Project

General

Profile

Bug #25394

Remove_ea_value() calls and its 64K limitation

Added by Caleb St. John over 1 year ago. Updated about 1 year ago.

Status:
Resolved
Priority:
Expected
Assignee:
Timur Bakeyev
Category:
OS
Target version:
Seen in:
Severity:
New
Reason for Closing:
Reason for Blocked:
Needs QA:
No
Needs Doc:
Yes
Needs Merging:
Yes
Needs Automation:
No
Support Suite Ticket:
n/a
Hardware Configuration:
ChangeLog Required:
No

Description

AFP_Resource extended attributes exist for files on Apple OS.
Copying these files from Apple OS to Windows works.
Copying these same files from Windows returns an error message as "Access Denied"

Samba's implementation includes the vfs_object of streams_xattr enabled by default per this article: https://www.samba.org/samba/docs/man/manpages/vfs_streams_xattr.8.html, however, it's still not working.

ReFS response 2.PNG (12.2 KB) ReFS response 2.PNG NT Status: FILE_SYSTEM_LIMITATION an odos, 08/07/2017 05:05 PM
12054

History

#1 Updated by Dru Lavigne over 1 year ago

  • Status changed from Untriaged to Unscreened

Ash: should this go to you or to John?

#2 Updated by Ash Gokhale over 1 year ago

  • Subject changed from AFP_Resource:Streams to AFP_Resource:Streams chokes samba writes
  • Status changed from Unscreened to Screened
  • Priority changed from No priority to Critical
  • Target version set to 11.1

reproduce trivially by constructing a file with a sufficiently large streams attribute as described in: https://docs.microsoft.com/en-us/sysinternals/downloads/streams

#3 Updated by Ash Gokhale over 1 year ago

Timur, John, what say you?

#4 Updated by Ash Gokhale over 1 year ago

This may be zfs extattr complaining about the size of the written object.Or smbd botching a steams-extattr vfs module passthrough operation .. Either way we need to understand the smbd codepath that returns the err.

#5 Updated by Timur Bakeyev over 1 year ago

  • Status changed from Screened to Unscreened

It would be nice to see debug info for this case. Few points to consider:

  • without enabled vfs_fruit with friends it's still should be possible to copy files to Samba share, just streams info would be omitted. But there should be no Access denied error.
  • There've been quite essential fixes to vfs_fruit in Samba 4.6.x, which is in the 11.0 only. So, customer may consider to upgrade, if any.
  • In 11.0 we generate proper order of vfs objects to enable AAP, which look like:
        vfs objects = zfs_space zfsacl catia fruit streams_xattr aio_pthread
    

    Still, so far you need to add catia and fruit manually, as well as their additional options through aux parameters.

#6 Updated by Ash Gokhale over 1 year ago

debugs are yours to peruse, (oob)

#7 Updated by Ash Gokhale over 1 year ago

Also in this error case AAP is not active ( the client is win10 based )

#8 Updated by an odos over 1 year ago

Ash Gokhale wrote:

Also in this error case AAP is not active ( the client is win10 based )

This is something that has been bugging me for a while. :-) Samba on FreeNAS throws an NT_STATUS_ACCESS_DENIED message when the size of an extended attribute exceeds 64KB. Sometimes it will also do you the favor of crashing your client as well. This is something I've seen repeatedly on the forums.

I actually asked about this on the Samba Mailing lists. See Ralph Böhme's response here: https://lists.samba.org/archive/samba/2017-August/210093.html

He stated:

"Yeah, iirc a built in buffer-size limit. We don't expect xattrs to be much larger, as no fs on Linux supports xattrs larger then iirc 64 KB. If you feel like it, you could write a VFS module that adds better support for this on FreeBSD, but what is the use case?"

It's clearly not a simple fix, but it would be a great feature to have. The size limit on ADS comes up periodically in the forums (users with avi files with subtitles / data stored in ADS), photoshop metadata stored in ADS, etc. At present, there is no good solution to this problem. I've been having users strip the streams and lose whatever data is stored there.

I believe that I was successful in writing larger xattrs in FreeBSD through the quick-and-dirty method mentioned in my email. ( cat <file with lots of data> | setextattr -i user 'DosStream.User.Stream:$DATA' file.txt ). I've personally written ones of up to 3MB in size, but have never tested the limits of what I can write. In testing I did the following:

  • Write a large "User DosStream.User.Stream:$DATA" xattr
  • In Powershell on windows client, ran command "Get-Item -Path <path to file> -stream *"

If the xattr was smaller than 64K I could see it in powershell. If it was more than 64K I could not see it. So the problem is probably Samba? One question that Ralph asked (about FreeBSD) was:

"Does it these day support the POSIX file IO API on xattrs like Solaris does?"

Which is something that I cannot answer, but you probably can. :-)

#9 Updated by an odos over 1 year ago

Ralph's response makes me wonder whether it's possible to bump up the buffer size limit in the FreeNAS samba fork rather than writing a custom VFS module. If nothing else, it'll probably break things in interesting ways. :-)

#10 Updated by Timur Bakeyev over 1 year ago

Well, how far should it be raised? Any fixed size buffer once wouldn't be enough.

But as I understood from the brief look into the code, the 64K limit is caused by the maximum packet size that can be sent at once. To fix the issue we'd need to rework corresponding part of the code and as xattr are used all around the Samba code - that could be tricky. But doable, of course.

#11 Updated by an odos over 1 year ago

Timur Bakeyev wrote:

Well, how far should it be raised? Any fixed size buffer once wouldn't be enough.

But as I understood from the brief look into the code, the 64K limit is caused by the maximum packet size that can be sent at once. To fix the issue we'd need to rework corresponding part of the code and as xattr are used all around the Samba code - that could be tricky. But doable, of course.

Well, I've heard that at CERN the ADS size can measure in terabytes, but they're probably not planning to use FreeNAS so we don't need to go that high. :-) Whenever I pull a number out of my head, it's usually 42. Perhaps look at the size of resource forks being generated by Adobe Photoshop. These are the most common problems I've seen.

Honestly, I haven't had to deal with oversized ADS myself. The cases where I've seen it are

1) resource forks on macs / associated metadata
2) media files

(1) is probably addressed by vfs_fruit, but there may still be some edge-cases where additional metadata gets written that won't be stored as double-files. Additionally, there may be some edge cases where users need to set "fruit:resource = xattr". Increasing the xattr size samba allows us to write will help keep FreeNAS from barfing on large resource forks. This also will give us a somewhat better option in cases where users can't veto apple_double files. The samba manpage says "the abstraction leak of exposing the internally created ._ files may have other unknown side effects." That doesn't sound good.

(2) is tricky because I don't have any media files with them. There were some references to software DVRs storing index information in them and possibly other metadata.

Do you think this is something that we could fix and upstream (perhaps with some checks for whether the samba server is running FreeBSD / ZFS)?

#12 Updated by Ash Gokhale over 1 year ago

Unfortunately PS on the mac writes 64kb + 0x106bytes to tag the fork and add tail-fins.

Increasing static buffers to this routine might let us live another day but it's weak motorcycle maintenance.
Sizing the buffer correctly shouldn't be _that hard. - Right?

I'm weary of letting AppleDouble files into the FS when we have extarr in ZFS that serves the purpose exaclty. What happens when a windows users copies files created by a mac? Do we need to wander after collecting ._$FILE after each possible operation? Teaching all the file server protocol translators about streams vs. resources is a dark art.

#13 Updated by an odos over 1 year ago

12054

Ash Gokhale wrote:

Unfortunately PS on the mac writes 64kb + 0x106bytes to tag the fork and add tail-fins.

Increasing static buffers to this routine might let us live another day but it's weak motorcycle maintenance.
Sizing the buffer correctly shouldn't be _that hard. - Right?

I'm weary of letting AppleDouble files into the FS when we have extarr in ZFS that serves the purpose exaclty. What happens when a windows users copies files created by a mac? Do we need to wander after collecting ._$FILE after each possible operation? Teaching all the file server protocol translators about streams vs. resources is a dark art.

It appears that Microsoft capped the size of alternate datastreams to 128K in ReFS. This may end up being a limit that Apple will have to work around as well. Perhaps hardcoding at some level above 128KB isn't a terrible solution since it's still better than what MS is doing? :-)

I believe that ADS isn't allowed on Azure, and so it appears MS is severely limiting the scope of what they're used for (Zone Identifier metadata, thumbnails, etc.).

Based on some wireshark traces on SMB2 traffic with an ReFS volume, it appears that the Windows server responds to large stream write attempts with "NT Status: (0xc0000427)". Wireshark flags this response as "unknown", but it actually corresponds to "STATUS_FILE_SYSTEM_LIMITATION". Windows translates this as "Error 0x80070299: The requested operation could not be completed due to a filesystem limitation". And then allows the user to skip the file.

At some point, apple's SMB client will probably need to properly handle this SMB2 response as well and not try to write large resource forks. I'm somewhat curious what the responses are from an ReFS volume to a Mac client trying to write a very large resource fork. Perhaps it's already gracefully handled?

So perhaps the fix is two part:
1) increase buffer sizes
2) fix streams_xattr to respond to write requests for overly large streams with "STATUS_FILE_SYSTEM_LIMITATION".

I'm not sure about (2). It'll require input from someone who knows more about SMB2 protocol / samba code. I'm just pointing out that MS is having to deal with the same problem so it might be worthwhile to see how they handle it as well (that might be an easier fix than rewrite the samba xattr code). Sorry if this is rambling.

#14 Updated by Timur Bakeyev over 1 year ago

As I understand, in fact we are talking about streams support, which are used to store secondary information regarding the main file, like previews, alternative representations, etc. It happened so, that we (ab)use xattr for storing the streams, using vfs_streams_xattr. As xattr used in other parts of Samba for different purposes, we hit the limitation, which is introduced in other parts of the code. That, possibly, should be fixed.

But, if we need to solve this problem immediately, there is an alternative implementation - http://samba.org.ru/samba/docs/man/manpages/vfs_streams_depot.8.html

That, possibly, has it's own penalties and still experimental, but it's an alternative.

#15 Updated by an odos over 1 year ago

Timur Bakeyev wrote:

That, possibly, has it's own penalties and still experimental, but it's an alternative.

There seems to be a pretty huge caveat to streams_depot:

vfs_streams_depot is totally unusable. While it can handle large streams and 
therefore resource forks, it stores them into directories named by major/minor 
of the device and inode of the original file. These files cannot be restored 
by standard unix backup tools and the linkage will also be lost if LVM f.e. 
changes the device node.

http://marc.info/?l=samba&m=132542069802160&w=2

#16 Updated by Ash Gokhale over 1 year ago

  • Status changed from Unscreened to Screened
  • Priority changed from Critical to Expected

#17 Updated by Ash Gokhale over 1 year ago

At some point, apple's SMB client will probably need to properly handle this SMB2 response as well and not try to write large resource forks. I'm somewhat curious what the responses are from an ReFS volume to a Mac client trying to write a very large resource fork. Perhaps it's already gracefully handled?

So perhaps the fix is two part:
1) increase buffer sizes
2) fix streams_xattr to respond to write requests for overly large streams with "STATUS_FILE_SYSTEM_LIMITATION".

I'm not sure about (2). It'll require input from someone who knows more about SMB2 protocol / samba code. >I'm just pointing out that MS is having to deal with the same problem so it might be worthwhile to see how >they handle it as well (that might be an easier fix than rewrite the samba xattr code). Sorry if this is >rambling.

Not rambling at all - everyone one at this card table seems to have a losing hand while apple is dealing -includng themselves.

I'm willing to push for a 128+2k buffer and stay with streams_xattr limit to ante up this hand. It's going to be fine until movies get 4k keyframes as thumbnails.

#18 Updated by an odos over 1 year ago

Ash Gokhale wrote:

At some point, apple's SMB client will probably need to properly handle this SMB2 response as well and not try to write large resource forks. I'm somewhat curious what the responses are from an ReFS volume to a Mac client trying to write a very large resource fork. Perhaps it's already gracefully handled?

So perhaps the fix is two part:
1) increase buffer sizes
2) fix streams_xattr to respond to write requests for overly large streams with "STATUS_FILE_SYSTEM_LIMITATION".

I'm not sure about (2). It'll require input from someone who knows more about SMB2 protocol / samba code. >I'm just pointing out that MS is having to deal with the same problem so it might be worthwhile to see how >they handle it as well (that might be an easier fix than rewrite the samba xattr code). Sorry if this is >rambling.

Not rambling at all - everyone one at this card table seems to have a losing hand while apple is dealing -includng themselves.

I'm willing to push for a 128+2k buffer and stay with streams_xattr limit to ante up this hand. It's going to be fine until movies get 4k keyframes as thumbnails.

I was able to fairly easily bump it up and write a few larger xattrs with a minor change:

root@TESTERERER:/usr/ports/net/samba46 # cat files/patch-source3__smbd__trans2.c
--- source3/smbd/trans2.c.orig  2017-04-27 04:33:47.000000000 -0500
+++ source3/smbd/trans2.c       2017-08-08 15:33:48.036945000 -0500
@@ -205,7 +205,7 @@
                      const char *ea_name, struct ea_struct *pea)
 {
        /* Get the value of this xattr. Max size is 64k. */
-       size_t attr_size = 256;
+       size_t attr_size = 524288;
        char *val = NULL;
        ssize_t sizeret;

I did absolutely zero testing for edge cases. Going too much higher causes problems as Timur indicated.

Honestly, I'm not sure how much we really want to abuse xattrs. Reading / writing to them is pretty slow. On my test VM, it took 31 seconds(!) to write a 24MB XATTR to a file using "setextattr". I believe that Samba and setextattr use extattr_set_fd(). Are there faster ways to interact with XATTRs on FreeBSD? What about zfs_setextattr()?

I believe this lack of performance at the xattr level is the root cause of the sort of o(n^2) delays we see when browsing directories with large amounts of files when "store dos attributes" is enabled. So there'd be an additional benefit if we could figure out a faster & better way to deal with xattrs. Perhaps through a vfs module like vfs_vxfs?

The easiest solution to the Apple problems, of course, may be to use vfs_fruit, as Timur stated at the beginning of all of this.

#19 Updated by Timur Bakeyev over 1 year ago

Guys, ash, odos. Can you get me the log.smbd with the debugging, set to maximum, that would expose this issue? I.e. I want to see the full traceback of the functions that are called during this operation(and failure).

I believe the problem is somewhere here, as you can see from the code in trans2.c:

/****************************************************************************
 Get one EA value. Fill in a struct ea_struct.
****************************************************************************/
NTSTATUS get_ea_value(TALLOC_CTX *mem_ctx, connection_struct *conn,
                      files_struct *fsp, const char *fname,
                      const char *ea_name, struct ea_struct *pea)
{
        /* Get the value of this xattr. Max size is 64k. */
        size_t attr_size = 256;
        char *val = NULL;
        ssize_t sizeret;

 again:

        val = talloc_realloc(mem_ctx, val, char, attr_size);
        if (!val) {
                return NT_STATUS_NO_MEMORY;
        }

        if (fsp && fsp->fh->fd != -1) {
                sizeret = SMB_VFS_FGETXATTR(fsp, ea_name, val, attr_size);
        } else {
                sizeret = SMB_VFS_GETXATTR(conn, fname, ea_name, val, attr_size);
        }

        if (sizeret == -1 && errno == ERANGE && attr_size != 65536) {
                attr_size = 65536;
                goto again;
        }

        if (sizeret == -1) {
                return map_nt_error_from_unix(errno);
        }

        DEBUG(10,("get_ea_value: EA %s is of length %u\n", ea_name, (unsigned int)sizeret));
        dump_data(10, (uint8_t *)val, sizeret);

        pea->flags = 0;
        if (strnequal(ea_name, "user.", 5)) {
                pea->name = talloc_strdup(mem_ctx, &ea_name[5]);
        } else {
                pea->name = talloc_strdup(mem_ctx, ea_name);
        }
        if (pea->name == NULL) {
                TALLOC_FREE(val);
                return NT_STATUS_NO_MEMORY;
        }
        pea->value.data = (unsigned char *)val;
        pea->value.length = (size_t)sizeret;
        return NT_STATUS_OK;
}

NTSTATUS get_ea_names_from_file(TALLOC_CTX *mem_ctx,
                                connection_struct *conn,
                                files_struct *fsp,
                                const struct smb_filename *smb_fname,
                                char ***pnames,
                                size_t *pnum_names)
{
        /* Get a list of all xattrs. Max namesize is 64k. */
        size_t ea_namelist_size = 1024;
        char *ea_namelist = NULL;

        char *p;
        char **names, **tmp;
        size_t num_names;
        ssize_t sizeret = -1;
        NTSTATUS status;

...
        while (ea_namelist_size <= 65536) {

                ea_namelist = talloc_realloc(
                        names, ea_namelist, char, ea_namelist_size);
                if (ea_namelist == NULL) {
                        DEBUG(0, ("talloc failed\n"));
                        TALLOC_FREE(names);
                        return NT_STATUS_NO_MEMORY;
                }

                if (fsp && fsp->fh->fd != -1) {
                        sizeret = SMB_VFS_FLISTXATTR(fsp, ea_namelist,
                                                     ea_namelist_size);
                } else {
                        sizeret = SMB_VFS_LISTXATTR(conn,
                                        smb_fname->base_name,
                                        ea_namelist,
                                        ea_namelist_size);
                }

                if ((sizeret == -1) && (errno == ERANGE)) {
                        ea_namelist_size *= 2;
                }
                else {
                        break;
                }
        }

        if (sizeret == -1) {
                TALLOC_FREE(names);
                return map_nt_error_from_unix(errno);
        }

        DEBUG(10, ("%s: ea_namelist size = %u\n",
                   __func__, (unsigned int)sizeret));

        if (sizeret == 0) {
                TALLOC_FREE(names);
                return NT_STATUS_OK;
        }
...
        if (pnames) {
                *pnames = names;
        } else {
                TALLOC_FREE(names);
        }
        *pnum_names = num_names;
        return NT_STATUS_OK;
}

/****************************************************************************
 Return a linked list of the total EA's. Plus the total size
****************************************************************************/

static NTSTATUS get_ea_list_from_file_path(TALLOC_CTX *mem_ctx,
                                connection_struct *conn,
                                files_struct *fsp,
                                const struct smb_filename *smb_fname,
                                size_t *pea_total_len,
                                struct ea_list **ea_list)
{
        /* Get a list of all xattrs. Max namesize is 64k. */
        size_t i, num_names;
        char **names;
        struct ea_list *ea_list_head = NULL;
        bool posix_pathnames = false;
        NTSTATUS status;

        *pea_total_len = 0;
        *ea_list = NULL;

        if (fsp) {
                posix_pathnames =
                        (fsp->fsp_name->flags & SMB_FILENAME_POSIX_PATH);
        } else {
                posix_pathnames = (smb_fname->flags & SMB_FILENAME_POSIX_PATH);
        }

        status = get_ea_names_from_file(talloc_tos(),
                                conn,
                                fsp,
                                smb_fname,
                                &names,
                                &num_names);

        if (!NT_STATUS_IS_OK(status)) {
                return status;
        }

        if (num_names == 0) {
                *ea_list = NULL;
                return NT_STATUS_OK;
        }

        for (i=0; i<num_names; i++) {
                struct ea_list *listp;
                fstring dos_ea_name;

                if (strnequal(names[i], "system.", 7)
                    || samba_private_attr_name(names[i]))
                        continue;

                /*
                 * Filter out any underlying POSIX EA names
                 * that a Windows client can't handle.
                 */
                if (!posix_pathnames &&
                                is_invalid_windows_ea_name(names[i])) {
                        continue;
                }

                listp = talloc(mem_ctx, struct ea_list);
                if (listp == NULL) {
                        return NT_STATUS_NO_MEMORY;
                }

                status = get_ea_value(listp,
                                        conn,
                                        fsp,
                                        smb_fname->base_name,
                                        names[i],
                                        &listp->ea);

                if (!NT_STATUS_IS_OK(status)) {
                        TALLOC_FREE(listp);
                        return status;
                }

...

        DEBUG(10, ("get_ea_list_from_file: total_len = %u\n",
                   (unsigned int)*pea_total_len));

        *ea_list = ea_list_head;
        return NT_STATUS_OK;
}

#20 Updated by Timur Bakeyev over 1 year ago

I think you are right, odoc, and we need to take an advantage that we are ZFS-only system by now and streamline certain generic code paths.

Somehow I though that zfsacl module also hooks up on the set/get/listxattr functions, but, apparently, not.

One quick test can be done with vfs_xattr_tdb and see, how it'll change the performance of the EA operations.

But that still will leave vfs_stream_xattr with explicit calls to:

         status = get_ea_value(talloc_tos(), handle->conn, NULL,
                              smb_fname->base_name, xattr_name, &ea);

Which will need to be modified. Well, something interesting could be developed around all this :)

#21 Updated by Timur Bakeyev over 1 year ago

I did some rough tests with EA API and got mixed feelings about them :)

Indeed, you are right that setting extended attribute with setextattr is, somehow, expensive operation, it can take minutes[!], before few megabytes got written into the attribute. I got 10 minutes result for 64Mb file.

But after looking into the code I realised that this is mostly due 1K buffer that is used for stdin input. My test program with 1M buffer managed to perform the same operation in few seconds. What is really nasty here is that API expects that you'll provide buffer with all the necessary data at once, so you have to pre-allocate such a buffer and can't write data in chunks. Think of few Gig stream... Filling up the buffer, using sbuf API seems, takes most of the time, the system call itself takes less than a second:

a.out: 1502245014: No error: 0
Reading into buffer
a.out: 1502245023: No error: 0
extattr_set_file() call
a.out: 1502245023: No error: 0

Reading from the attribute is very quick, for the 100M stream it took:

real    0m0,419s
user    0m0,000s
sys     0m0,417s

zfs_setextattr is wrapped by higher level API and, in general, won't save much time.

In general, due the way they implemented, EA, ACLs and streams are closely interconnected in Samba code and that may create problems with the performance, as we can see. Splitting that out may help to make things performing better.

#22 Updated by an odos over 1 year ago

  • File TwilightSparkleStreamLaser.ps1 added

Timur Bakeyev wrote:

Guys, ash, odos. Can you get me the log.smbd with the debugging, set to maximum, that would expose this issue? I.e. I want to see the full traceback of the functions that are called during this operation(and failure).

Timur, I'll get you the logs once I have a chance (maybe tomorrow). If you have a windows VM available, you can use the following powershell script to add streams of various sizes to files

$StreamTarget = "C:\users\theGibson\Desktop\workflow.png" 
$StreamPayload = "C:\users\theGibson\Desktop\my_sploitz.txt" 

$stream = Get-Content $StreamPayload
Add-Content -Path $StreamTarget -Value $stream -Stream 'TwilightSparkle'

Basically, open "powershell ise" on a recently modern windows system, paste my awesomely long and complex script into it :), then replace the target and payload with whatever you want to hit. This will add the payload as a "TwilightSparkle" stream that you can then use to cause errors on shares. The payload doesn't have to be a text file, just choose something sitting around your FS that is appropriately sized.

I've been doing this from Windows since I currently don't have a mac sitting around for testing and the ea code is basically the same whether I'm abusing streams from a Mac or a PC.

#23 Updated by an odos over 1 year ago

  • File StreamDetector.ps1 added

Another tool I wrote that might be useful to catch stream usage in the wild (to get an idea regarding size & content). Like any code I write, it mostly works. Modify & launch from Powershell ISE.

#24 Updated by an odos over 1 year ago

  • File log.smbd added

log.smbd with "log level = 10". FreeBSD with net/samba46 port installed and streams_xattr enabled.

smb4.conf as follows:

[global]
   netbios name = TESTERER
   guest account = awalker
   map to guest = Bad User
   log level = 10
   store dos attributes = yes
   ea support = yes
   map archive = no
   map readonly = no
   kernel change notify = no

[SHARE]
   path = "/mnt/DonkeyFarts/Vol1" 
   writeable = yes
   vfs objects = zfs_space streams_xattr
   guest ok = yes
   guest only = yes
   zfs_space:zpool-dsize = true

The attempt to write the stream starts at line 16295. Multiple streams on file (whoops) "TwilightSparkle" and "SecretStream".

A couple of quick notes:
The stream is getting truncated to "00" (line 16656).

[2017/08/09 08:27:59.973151, 10, pid=595, effective(1001, 0), real(0, 0), class=vfs] ../source3/modules/vfs_streams_xattr.c:160(streams_xattr_get_name)
  xattr_name: user.DosStream.TwilightSparkle:$DATA, stream_name: :TwilightSparkle
[2017/08/09 08:27:59.973201, 10, pid=595, effective(1001, 0), real(0, 0), class=vfs] ../source3/modules/vfs_streams_xattr.c:474(streams_xattr_open)
  get_ea_value returned NT_STATUS_NOT_FOUND
[2017/08/09 08:27:59.973218, 10, pid=595, effective(1001, 0), real(0, 0), class=vfs] ../source3/modules/vfs_streams_xattr.c:501(streams_xattr_open)
  creating or truncating attribute user.DosStream.TwilightSparkle:$DATA on file IRL 2016.txt
[2017/08/09 08:27:59.973302, 10, pid=595, effective(1001, 0), real(0, 0)] ../source3/smbd/open.c:689(fd_open)
  fd_open: name IRL 2016.txt:TwilightSparkle, flags = 05002 mode = 0644, fd = 38. 
[2017/08/09 08:27:59.973324, 10, pid=595, effective(1001, 0), real(0, 0), class=vfs] ../source3/modules/vfs_streams_xattr.c:234(streams_xattr_fstat)
  streams_xattr_fstat called for 38
[2017/08/09 08:27:59.973337, 10, pid=595, effective(1001, 0), real(0, 0), class=vfs] ../source3/modules/vfs_streams_xattr.c:160(streams_xattr_get_name)
  xattr_name: user.DosStream.TwilightSparkle:$DATA, stream_name: :TwilightSparkle
[2017/08/09 08:27:59.973387, 10, pid=595, effective(1001, 0), real(0, 0)] ../source3/smbd/trans2.c:234(get_ea_value)
  get_ea_value: EA user.DosStream.TwilightSparkle:$DATA is of length 1
[2017/08/09 08:27:59.973403, 10, pid=595, effective(1001, 0), real(0, 0)] ../lib/util/util.c:555(dump_data)
  [0000] 00                                              

This is different from cases with a small stream (line 16306):
[2017/08/09 08:27:59.970527, 10, pid=595, effective(1001, 0), real(0, 0)] ../source3/smbd/trans2.c:234(get_ea_value)
  get_ea_value: EA user.DosStream.SecretStream:$DATA is of length 41
[2017/08/09 08:27:59.970543, 10, pid=595, effective(1001, 0), real(0, 0)] ../lib/util/util.c:555(dump_data)
  [0000] 53 65 63 72 65 74 20 49   6E 66 6F 72 6D 61 74 69   Secret I nformati
  [0010] 6F 6E 0D 0A 53 65 63 72   65 74 20 49 6E 66 6F 72   on..Secr et Infor
  [0020] 6D 61 74 69 6F 6E 0D 0A   00                        mation.. .

NT_STATUS_ACCESS_DENIED appears to be generated here (line 17512)

[2017/08/09 08:27:59.985730, 10, pid=595, effective(1001, 0), real(0, 0), class=vfs] ../source3/modules/vfs_streams_xattr.c:954(streams_xattr_pwrite)
  streams_xattr_pwrite called for 131072 bytes
[2017/08/09 08:27:59.985764, 10, pid=595, effective(1001, 0), real(0, 0)] ../source3/smbd/fileio.c:132(real_write_file)
  real_write_file (IRL 2016.txt:TwilightSparkle): pos = 0, size = 131072, returned -1
[2017/08/09 08:27:59.985783,  2, pid=595, effective(1001, 0), real(0, 0)] ../source3/smbd/smb2_write.c:201(smb2_write_complete_internal)
  smb2_write failed: fnum 2107955480, file IRL 2016.txt:TwilightSparkle, length=131072 offset=0 nwritten=-1: NT_STATUS_ACCESS_DENIED
[2017/08/09 08:27:59.985798, 10, pid=595, effective(1001, 0), real(0, 0)] ../source3/smbd/smb2_write.c:379(smbd_smb2_write_send)
  smb2: write on file IRL 2016.txt:TwilightSparkle, offset 0, requested 131072, written = 4294967295
[2017/08/09 08:27:59.985814,  3, pid=595, effective(1001, 0), real(0, 0)] ../source3/smbd/smb2_server.c:3097(smbd_smb2_request_error_ex)
  smbd_smb2_request_error_ex: smbd_smb2_request_error_ex: idx[1] status[NT_STATUS_ACCESS_DENIED] || at ../source3/smbd/smb2_write.c:135
[2017/08/09 08:27:59.985828, 10, pid=595, effective(1001, 0), real(0, 0)] ../source3/smbd/smb2_server.c:2988(smbd_smb2_request_done_ex)
  smbd_smb2_request_done_ex: idx[1] status[NT_STATUS_ACCESS_DENIED] body[8] dyn[yes:1] at ../source3/smbd/smb2_server.c:3145

#25 Updated by an odos over 1 year ago

IIRC, NT_STATUS_ACCESS_DENIED does not always mean that there's a permissions error. I believe it's the default return from the function to map NT error codes from Unix error codes. I.e., if there's no explicit mapping for the Unix error code, then samba will return "NT_STATUS_ACCESS_DENIED".

#26 Updated by Ash Gokhale over 1 year ago

Timur; logs are available OOB.

#27 Updated by Timur Bakeyev over 1 year ago

IIRC, NT_STATUS_ACCESS_DENIED does not always mean that there's a permissions error. I believe it's the default return from the function to map NT error codes from Unix error codes. I.e., if there's no explicit mapping for the Unix error code, then samba will return "NT_STATUS_ACCESS_DENIED".

That's true, it comes from source3/lib/errmap_unix.c and:

        /* Default return */
        return NT_STATUS_ACCESS_DENIED;

But in general it doesn't matter, as I came to the bright idea that streams_xattr doesn't need to use get_ea_value in the first place, it can safely(?) be replaced by a similar bundled function based on the SMB_VFS_GETXATTR. I think we can do this.

#28 Updated by Timur Bakeyev over 1 year ago

Timur; logs are available OOB.

Thanks a lot, Ash! I'll take a look immediately!

#29 Updated by an odos over 1 year ago

Timur Bakeyev wrote:

IIRC, NT_STATUS_ACCESS_DENIED does not always mean that there's a permissions error. I believe it's the default return from the function to map NT error codes from Unix error codes. I.e., if there's no explicit mapping for the Unix error code, then samba will return "NT_STATUS_ACCESS_DENIED".

That's true, it comes from source3/lib/errmap_unix.c and:
[...]

But in general it doesn't matter, as I came to the bright idea that streams_xattr doesn't need to use get_ea_value in the first place, it can safely(?) be replaced by a similar bundled function based on the SMB_VFS_GETXATTR. I think we can do this.

I think that's what vfs_fruit does to read / parse the Netatalk AppleDouble metadata xattr (i.e. use SMB_VFS_GETXATTR rather than get_ea_value). It sounds like you're onto something. :-)

#30 Updated by Timur Bakeyev over 1 year ago

I think that's what vfs_fruit does to read / parse the Netatalk AppleDouble metadata xattr (i.e. use SMB_VFS_GETXATTR rather than get_ea_value). It sounds like you're onto something. :-)

Even streams_xattr uses it by itself in other places! It's just one(two) places where that crippled function is used. Possibly looked easier to re-used when was developed. But I see no reason to keep this call with it's size limitations.

#31 Updated by an odos over 1 year ago

Timur Bakeyev wrote:

I think that's what vfs_fruit does to read / parse the Netatalk AppleDouble metadata xattr (i.e. use SMB_VFS_GETXATTR rather than get_ea_value). It sounds like you're onto something. :-)

Even streams_xattr uses it by itself in other places! It's just one(two) places where that crippled function is used. Possibly looked easier to re-used when was developed. But I see no reason to keep this call with it's size limitations.

FYI in case you haven't seen it, there is a fairly large pending set of patches for vfs_streams_xattr. https://lists.samba.org/archive/samba-technical/2017-July/122034.html

#32 Updated by Timur Bakeyev over 1 year ago

Yes, I've seen the patches and hope to develop our fixes at the top of them(although they address different issues).

#33 Updated by Timur Bakeyev over 1 year ago

Jeremy Allison, one of the core Samba developers replied to this thread with:

Almost certainly malware. The primary use case for streams is
malware or CIA-exfiltration of your company data (I'm not joking,
the Wikileaks documents have the details).

#34 Updated by an odos over 1 year ago

Yeah, I saw that in my inbox, but that is only if we limit the conversation to what you'd typically see from Windows clients. I believe it is definitely desireable to be able store resource forks as xattrs rather than have Mac clients crap metadata as files all over shares. ;-)

#36 Updated by an odos over 1 year ago

Ash Gokhale wrote:

this is getting traction on the samba list
https://lists.samba.org/archive/samba/2017-August/210141.html
https://lists.samba.org/archive/samba/2017-August/210093.html

Well, yes and no. I'm the author of those emails. :-) But I did notice that a entry in the Samba bugtracker appeared briefly regarding the NT_STATUS_FILE_SYSTEM_LIMITATION response. I think it was subsequently either marked as "private" or deleted. Most likely the former as Ralph appeared to be interested in investigating it further. Hopefully, this will eventually result in Mac clients behaving somewhat better because ReFS is probably not going away in the server space. Unfortunately, the nasty task of fixing large xattr support on FreeBSD appears to have fallen squarely on poor Timur. :D

I was able to perform some "weak motorcycle maintenance" and increase the size of xattrs I could write, but real life / work keeps getting in the way of investigating the issue further. I now have a real Mac client, and so my ability to break things in interesting ways is improved. In your experience, are there any file types (for instance psd) that tend to have overly large resource forks?

#37 Updated by Ash Gokhale about 1 year ago

#38 Avatar?id=14398&size=24x24 Updated by Kris Moore about 1 year ago

Ping! Is this something you plan on issuing a Pull Request to merge in before 11.1?

#39 Updated by Ash Gokhale about 1 year ago

A branch of samba stable with 2mb XA's hasn't yet lost too much customer data: https://github.com/freenas/samba/tree/fix-25394-vs-stable Should I respin it as a PR for 4.7/master?

#40 Avatar?id=14398&size=24x24 Updated by Kris Moore about 1 year ago

Sounds good! Please do and we'll get this merged in time for 11.1-BETA1 next monday.

#41 Updated by Ash Gokhale about 1 year ago

wilco

#42 Updated by Ash Gokhale about 1 year ago

https://github.com/freenas/samba/pull/14

Please send Adobe a Christmas card with a 2Mb XA with an HDR 5K moving preview rickroll.

#43 Updated by Dru Lavigne about 1 year ago

  • Status changed from Screened to Needs Developer Review
  • Assignee changed from Ash Gokhale to Kris Moore

#44 Avatar?id=14398&size=24x24 Updated by Kris Moore about 1 year ago

  • Assignee changed from Kris Moore to Timur Bakeyev

Timur, Would you care to review this? The PR is still build testing.

#45 Updated by Timur Bakeyev about 1 year ago

  • Status changed from Needs Developer Review to Reviewed by Developer
  • Assignee changed from Timur Bakeyev to Ash Gokhale

In general that LGTM, but I consider this a hot fix and that a proper, more correct one would be written at some point.

I see potential memory allocation issues on a busy server with multiple simultaneous connections, but with adding DEBUG statement we can see and count such occurrences if any.

#46 Updated by Timur Bakeyev about 1 year ago

Meanwhile I've patched vfs_stream_xattr not to use get_ea_value at all. That doesn't cover all the issues with that function, but should address original problem in a more graceful way, by allocating exactly required buffer of any arbitrary size, if there is enough memory.

Requires testing though.

#47 Updated by Joe Maloney about 1 year ago

This will require an OS X client to write the large resource fork. Attaching notes from Andrew on how to reproduce for testing:

Andrew, [Oct 24, 2017 at 2:39:53 PM]:
You can write a large resource fork on a file as follows:

cat largefile > TEST/..namedfork/rsrc

Then copy it to the smb server. That generates a large resource fork on "TEST".

#48 Updated by Timur Bakeyev about 1 year ago

If we can pull that to the nightlies, we have chance to get that fix in 11.1.

https://github.com/freenas/samba/pull/15

#49 Updated by Dru Lavigne about 1 year ago

  • File deleted (log.smbd)

#50 Updated by Dru Lavigne about 1 year ago

  • Description updated (diff)

#51 Updated by Dru Lavigne about 1 year ago

  • Subject changed from AFP_Resource:Streams chokes samba writes to Remove_ea_value() calls and its 64K limitation

#52 Updated by Dru Lavigne about 1 year ago

  • Status changed from Reviewed by Developer to Resolved
  • Target version changed from 11.1 to 11.1-RC1

#53 Updated by Timur Bakeyev about 1 year ago

  • Status changed from Resolved to Unscreened

#54 Updated by Timur Bakeyev about 1 year ago

  • Status changed from Unscreened to Needs Developer Review
  • Assignee changed from Ash Gokhale to John Hixson

John, please, review my PR soon enough :)

#55 Updated by Timur Bakeyev about 1 year ago

As we are a bit tight on scheduler for 11.1 I've committed the PR without review from John's side. Sorry! In my defense I can say that I've tested the patched module with Windows and it worked OK in both directions, copying ADS of 8M size to and from FN and the date in the payload remained identical to the original files.

Still needs to be tested with MacOSX, so patched pushed to the master to get it in the nighlies.

The patch is still available for review in the commit https://github.com/freenas/samba/pull/15/commits/f0a2dc4acc76bff4cf98639d12ebe3cc857574fa

#56 Updated by Timur Bakeyev about 1 year ago

To test the module from Windows side I've used 8Mb binary file test.bin and a small text file test.txt. I've added binary file as a ADS to the text file:

C:\> type test.bin > test.txt:mystream

http://www.nirsoft.net/utils/alternate_data_streams.html allows you to view and extract streams from the files.

Then I've just copied test.txt to the FN share with streams_xattr enabled and on FN:

# lsextattr user test.txt
test.txt        DosStream.mystream:$DATA        DOSATTRIB
# getextattr -qq user 'DosStream.teststream:$DATA' test.txt > payload.bin
# ls -l payload.bin
-rw-r--r--  1 root  timur  8957653 26 окт.  05:01 payload.bin

That was exactly the size of the original payload file. Coping it to the same share and cmp revealed that they are identical.

Coping file from FN to Windows also preserved full size payload and with the mentioned tool extracted payload was the same as original binary file.

Note: Somehow using PowerShell to extract payload doubled the size of it, it seems due conversion of 8bit chars into 2byte UCS-2 chars.

#57 Updated by Dru Lavigne about 1 year ago

  • Status changed from Needs Developer Review to 47

#58 Updated by John Hixson about 1 year ago

  • Assignee changed from John Hixson to Timur Bakeyev

Timur Bakeyev wrote:

John, please, review my PR soon enough

Looks good to me.

#59 Updated by Joe Maloney about 1 year ago

  • File bookmarks.html.zip added

Current status:

I cannot reproduce the file yet although I think I have enough insight to know what to try next now.

I can reproduce the issue using bookmarks.html (20mb) provided by Andrew by copying to windows 2012 server share, and then copying from Windows server directly as a client to a TrueNAS running 11.0-u4.

I am going to try to reproduce with an adobe product if I can before testing the fix.

#60 Updated by Joe Maloney about 1 year ago

Also to verify the resource fork was copied to the windows share this was important.

Windows PowerShell
Copyright (C) 2014 Microsoft Corporation. All rights reserved.

PS C:\Users\Administrator> Get-Item

cmdlet Get-Item at command pipeline position 1
Supply values for the following parameters:
Path[0]: PS C:\Users\Administrator> cd ..
PS C:\Users> cd ..
PS C:\> cd .\Share
PS C:\Share> Get-Item .\bookmarks.html -stream *

   FileName: C:\Share\bookmarks.html

Stream                   Length
------                   ------
:$DATA                    26107
AFP_Resource           19624237
com.apple.lastuse...         16
com.apple.quarantine         57

PS C:\Share>

#62 Updated by Dru Lavigne about 1 year ago

  • Status changed from 47 to Resolved

#63 Updated by Joe Maloney about 1 year ago

  • Needs QA changed from Yes to No
  • QA Status Test Passes FreeNAS added
  • QA Status deleted (Not Tested)

I can confirm that I am able to write the affected file to shares with FreeNAS RC1.

#64 Updated by Dru Lavigne about 1 year ago

  • File deleted (bookmarks.html.zip)

#65 Updated by Dru Lavigne about 1 year ago

  • File deleted (TwilightSparkleStreamLaser.ps1)

#66 Updated by Dru Lavigne about 1 year ago

  • File deleted (StreamDetector.ps1)

Also available in: Atom PDF