Bug 15099 - Setting fruit:resource = stream in vfs_fruit causes a panic.
Summary: Setting fruit:resource = stream in vfs_fruit causes a panic.
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: 4.16.1
Hardware: All All
: P5 major (vote)
Target Milestone: ---
Assignee: Jule Anger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-06-16 08:06 UTC by Mike Silva
Modified: 2022-06-28 06:52 UTC (History)
5 users (show)

See Also:


Attachments
Logs from reproducing the panic using a macOS 12 client (67.89 KB, application/zip)
2022-06-16 08:06 UTC, Mike Silva
no flags Details
Logs from reproducing the panic with an Ubuntu 22.04 client (1.98 KB, application/zip)
2022-06-16 08:07 UTC, Mike Silva
no flags Details
raw patch for master. (1.42 KB, patch)
2022-06-16 21:52 UTC, Jeremy Allison
jra: review? (slow)
Details
git-am fix for master. (6.74 KB, patch)
2022-06-18 01:00 UTC, Jeremy Allison
no flags Details
backport for 4.15 (7.05 KB, patch)
2022-06-20 16:19 UTC, Noel Power
jra: review-
Details
backport for 4.16 & 4.15 (6.99 KB, patch)
2022-06-20 16:19 UTC, Noel Power
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Silva 2022-06-16 08:06:13 UTC
Created attachment 17362 [details]
Logs from reproducing the panic using a macOS 12 client

Note: I see this bug in 4.15.5 and above, as well as 4.16.1.

Description:
Setting fruit:resource = stream in smb.conf, with streams_xattr also loaded causes a panic when accessing or trying to do file manipulation on shares, even when the filesystem in question supports sufficiently large xattributes. I can typically login to the share successfully. It's file manipulation that triggers the panic.

The panic is see is:
[2022/06/12 23:53:33.640745,  0] ../../source3/modules/vfs_streams_xattr.c:499(streams_xattr_unlink_internal)
  PANIC: assert failed at ../../source3/modules/vfs_streams_xattr.c(499): smb_fname->fsp != NULL
[2022/06/12 23:53:33.640992,  0] ../../lib/util/fault.c:172(smb_panic_log)

Setting fruit:resource = stream used to work in in 4.13.x releases of Samba, and I've had it configured that way without error for 2 years. In my case, the share is located on a btrfs volume, specifically formatted with a nodesize of 65536 set, to enable secondary data streams up to 64KB. (The max for a stock linux kernel.)

I have found that in the Samba 4.15.5 and above, if I set fruit:resource = xattr, there are no panics. This is a bit strange, as I understand both fruit:resource = stream and xattr options pass bits down to the next vfs_module in the stack to handle attributes/secondary data streams, in this case, vfs_streams_xattr.

I typically access the Samba shares from macOS clients, for both standard file shares and a Time Machine over SMB share. However, I've reproduced the issue connecting with a Ubuntu 22.04 and Windows clients, as well. The server in question is an Ubuntu 22.04 LTS on Raspberry Pi 4 (ARMv8) box. I've also reproduced it on a Ubuntu 22.04 amd64 server. But, I do not think this issue is likely specific to Ubuntu distributions of Linux. [Ubuntu 22.04 runs Samba 4.15.5, but the Ubuntu team also had me reproduce the issue on a daily build of Ubuntu 22.10, which runs Samba 4.16.1]

My smb.conf is mostly stock, with only these customizations:
aio read size = 1
aio write size = 1
block size = 4096
fruit:aapl = yes
fruit:copyfile = yes
fruit:encoding = native
fruit:metadata = stream
fruit:resource = stream
mdns name = mdns
min receivefile size = 1
strict allocate = yes
server multi channel support = yes
use sendfile = yes
vfs objects = catia fruit streams_xattr btrfs io_uring
kernel share modes = no
posix locking = no

I've been working on this issue with the Ubuntu server team. You can find the discussion of the bug with them, here:
https://bugs.launchpad.net/ubuntu/jammy/+source/samba/+bug/1977491

They asked that I contact you all, as it seems this is an issue upstream of them.

I will attach logs I have collected showing the panics.
Comment 1 Mike Silva 2022-06-16 08:07:35 UTC
Created attachment 17363 [details]
Logs from reproducing the panic with an Ubuntu 22.04 client
Comment 2 Jeremy Allison 2022-06-16 16:13:14 UTC
One for Ralph I think.
Comment 3 Jeremy Allison 2022-06-16 16:14:51 UTC
This is a regression due to the move to ensure that fsp (file handle based) calls are used instead of path based to prevent symlink races. It'll need some investigation.
Comment 4 Jeremy Allison 2022-06-16 16:54:16 UTC
(In reply to Mike Silva from comment #0)

> It's file manipulation that triggers the panic.

Can you give me a simple smbclient command (or set of commands) that reproduces the panic without needing a MacOSX client ?

That will greatly help in fixing and adding a regression test to ensure it stays fixed.

Thanks !
Comment 5 Mike Silva 2022-06-16 17:05:15 UTC
Sure, I'm installing smbclient now and I will experiment to lock down a reproducible case or two that triggers it.

I was going to ask about your regression suite, and how we might add something to check for this, after the issue had been identified and a patch issued, but happy to get cracking now.

That said, as indicated in the bug, I reproduced the panic using an Ubuntu Linux client just as easily as a Mac, interacting with the share via the relevant Finder/Gnome Files GUI. Though, I get that doesn't lend itself to easy automated testing. I don't know the smbclient commands yet, as I've yet to read the man page, but in one of my previous tests I was simply copying a file up to the server, and in another test attempting to delete another file from the server. It seems quite easy to trigger.
Comment 6 Jeremy Allison 2022-06-16 17:18:30 UTC
If you can reproduce it via GNOME -> libsmbclient by copying files around, then it should be easy to reproduce with smbclient, as both libsmbclient and smbclient use the same underlying cli_XXX() API internally. Let me know when you've got something that reproduces and I'll take a look !

Thanks.
Comment 7 Mike Silva 2022-06-16 20:26:19 UTC
The simplest possible test case is:

1) Use smbclient login to Samba server share.
2) put [filename]
3) rm [filename from #2]

Result:
Panic


I'll try some other smbclient commands or combinations of commands and see if I find other reliable means of reproducing the panic, but wanted to get you this ASAP.
Comment 8 Mike Silva 2022-06-16 21:01:15 UTC
I should have been more clear in my test case above about the result, as it may well effect how you script your testing.

Result:
Panic, with the file not deleted.
Comment 9 Jeremy Allison 2022-06-16 21:05:01 UTC
Great, that should be enough to reproduce, thanks !
Comment 10 Jeremy Allison 2022-06-16 21:20:05 UTC
Reproduces on master, thanks !
Comment 11 Jeremy Allison 2022-06-16 21:42:59 UTC
Looks like the code in source3/modules/vfs_fruit.c:fruit_unlinkat() is where the problem lies:

2271         /*
2272          * A request to delete the base file. Because 0 byte resource
2273          * fork streams are not listed by fruit_streaminfo,
2274          * delete_all_streams() can't remove 0 byte resource fork
2275          * streams, so we have to cleanup this here.
2276          */
2277         rsrc_smb_fname = synthetic_smb_fname(talloc_tos(),
2278                                              smb_fname->base_name,
2279                                              AFPRESOURCE_STREAM_NAME,
2280                                              NULL,
2281                                              smb_fname->twrp,
2282                                              smb_fname->flags);
2283         if (rsrc_smb_fname == NULL) {
2284                 return -1;
2285         }
2286 
2287         ret = fruit_unlink_rsrc(handle, dirfsp, rsrc_smb_fname, true);

synthetic_smb_fname() creates rsrc_smb_fname(), but doesn't open a handle for rsrc_smb_fname->fsp.

fruit_unlink_rsrc_stream() then calls:

2049         ret = SMB_VFS_NEXT_UNLINKAT(handle,
2050                         dirfsp,
2051                         smb_fname,
2052                         0);

and inside streams_xattr_unlinkat() we have:

 500         SMB_ASSERT(smb_fname->fsp != NULL);
 501         SMB_ASSERT(fsp_is_alternate_stream(smb_fname->fsp));
 502 
 503         ret = SMB_VFS_FREMOVEXATTR(smb_fname->fsp->base_fsp, xattr_name);

We already cope with this inside streams_xattr_stat() and streams_xattr_renameat() by opening a synthetic_pathref(), so I might try adding similar code inside streams_xattr_unlinkat().
Comment 12 Jeremy Allison 2022-06-16 21:52:34 UTC
Created attachment 17366 [details]
raw patch for master.

This (raw) patch fixes the problem here. Not fully tested yet (of course :-). Can you confirm it works for you ?
Comment 13 Mike Silva 2022-06-16 22:13:53 UTC
Let me hit the samba wiki to see how to do a build and give it a go.
Comment 14 Sergio Durigan Junior 2022-06-17 14:34:25 UTC
Mike,

I can backport the patch against the current Ubuntu samba package on Jammy and prepare a PPA with it.  This should be easier for you to test.

Let me know what you prefer.  Thanks.
Comment 15 Mike Silva 2022-06-17 15:00:15 UTC
(In reply to Sergio Durigan Junior from comment #14)

That would actually be super helpful. Thanks. While I actually like learning about and setting up build systems, I’m a little pegged for time at the moment and this will help me test before I’d have more time to do a build on the weekend.
Comment 16 Sergio Durigan Junior 2022-06-17 19:14:43 UTC
You mentioned that you were able to reproduce the bug using the samba package from Kinetic, so I backported the patch and uploaded the package to the following PPA:

https://launchpad.net/~sergiodj/+archive/ubuntu/sssd-bugfix/+packages

I decided to backport to Kinetic first because the version of samba there is closer to upstream.  It is still unclear whether Jammy's samba will need more patches to be backported or not.
Comment 17 Jeremy Allison 2022-06-17 19:50:58 UTC
(In reply to Sergio Durigan Junior from comment #16)

The only extra patches will be a regression test to ensure we keep this functionality working.
Comment 18 Mike Silva 2022-06-17 21:57:59 UTC
(In reply to Sergio Durigan Junior from comment #16)
Hi Sergio,

Thank you. Unfortunately, the link to the PPA you provided looks to be for another bug not to do with Samba. Could you double check, please, and provide me a corrected one?

I'm getting the latest daily of Kinetic installed now...
Comment 19 Sergio Durigan Junior 2022-06-17 22:01:43 UTC
Sorry, that's for another bug (on another package) that I'm working on.

The correct link is:

https://launchpad.net/~sergiodj/+archive/ubuntu/samba-bug1977491/+packages

I've just backported the same fix on top of the Jammy samba package and uploaded it to the PPA, in case you'd like to try it on Jammy as well.
Comment 20 Mike Silva 2022-06-17 22:22:58 UTC
(In reply to Sergio Durigan Junior from comment #19)
No worries. Appreciate the help.

Does this PPA include the previous patches you backported, as well?
Comment 21 Mike Silva 2022-06-17 22:41:43 UTC
(In reply to Sergio Durigan Junior from comment #19)
Oops. I didn't see Launchpad helpfully provides a diff right on the PPA page. Never mind.

It's also showing that the Jammy build is still in process, so as soon as that's done I'll get rolling with testing against Jammy, then Kinetic.
Comment 22 Sergio Durigan Junior 2022-06-17 23:09:10 UTC
(In reply to Mike Silva from comment #20)

No, I've decided to backport only the patch from comment #12.  I think it's worth giving it a try by itself first and see if it fixes the problems you were experiencing.  If needed, just let me know and I can easily re-add those patches and build a new package.
Comment 23 Sergio Durigan Junior 2022-06-17 23:31:52 UTC
FWIW, I was able to reproduce the bug locally (using Ubuntu Kinetic and Ubuntu Jammy) and also verify that the patch from comment 12 fixes the issue on both systems.
Comment 24 Jeremy Allison 2022-06-18 01:00:39 UTC
Created attachment 17371 [details]
git-am fix for master.

Fix (including regression test) I'm planning to submit to gitlab-ci.
Comment 26 Mike Silva 2022-06-18 05:57:17 UTC
I've confirmed that this patch fixes the bug, thanks to Sergio's PPA applying it to Ubuntu Jammy (22.04LTS) and Kinetic (22.10).

In addition to the simple test case from above, I also kicked the tires a bit more, using some automated processes I typically use on the shares. Not a single hiccup.

Thank you, Jeremey and Sergio.
Comment 27 Mike Silva 2022-06-18 06:03:43 UTC
A followup question for you, Jeremy. At this point, is there any really functional difference in my use case, given the VFS modules I use, between fruit:resource = stream and fruit:resource = xattr? That is, are they essentially passing any additional data streams down to vfs_streams_xattr identically?
Comment 28 Jeremy Allison 2022-06-19 20:16:24 UTC
(In reply to Mike Silva from comment #27)

fruit:resource = stream just means pass resource forks down to underlying streams.

POSIX / Linux doesn't have streams so underneath you need to have a streams module (you're using streams_xattr) to provide the streams functionality fruit is using.
Comment 29 Ralph Böhme 2022-06-20 04:51:20 UTC
(In reply to Mike Silva from comment #27)
Don't use fruit:resource=xattr unless running Solaris, cf man vfs_fruit.
Comment 30 Mike Silva 2022-06-20 06:12:19 UTC
Hi Ralph and Jeremy,

I assure you I've read the vfs_fruit, vfs_streams_xattrib, smb.conf, and xattr man pages. :-) That's what prompted the question.

In my case, the filesystem is btrfs. Which does support xattribs. Further, I formatted the btrfs filesystem with a nodesize of 64KB, to handle the max xattrib size Samba on Linux will throw at it.
See:
https://manpages.ubuntu.com/manpages/jammy/man7/xattr.7.html
paying particular attention to the "Filesystem differences" section and, "smbd max xattr size" here:
https://www.samba.org/samba/docs/current/man-html/smb.conf.5.html

The portion of the vfs_fruit man page Ralph is referring to states:
"xattr - use a xattr, requires a filesystem with large xattr support and a file IO API compatible with xattrs, this boils down to Solaris and derived platforms and ZFS"

The use of "this boils down to Solaris...and ZFS", thus does not appear to be meant to be an exhaustive list (and may with the passing of time have become an anachronism), as the default filesystem EXT4 on Ubuntu distributions is configured to handle xatrribs by default, and btrfs also handles them by default. In both cases, up to the limit that Samba itself will support, 64KB, per the smb.conf manpage section referenced above.

Moreover, what the max xattr size limit may be on Linux with EXT4, btrfs, and perhaps other filesystems seems dependent on whether one has customized the linux kernel config to handle larger xattr sizes than the 64KB, set by default in most distributions. One can find how their kernel is configured, with the following:
grep XATTR /usr/include/linux/limits.h
Those limits can, of course be changed.

So, back to both your responses to my question:
- In response to Jeremy, I get that fruit:resource = stream passes xattr handling to vfs_stream_xattr. But, I erroneously stated that fruit:resource = xattr passes xattr handling down to streams_xattr too. It doesn't. What I should have said, and meant to say, is functionally basically the same thing happens when fruit:resource = xattr passes the xattr stream down to the Linux kernel vfs, where they then get handled just fine by Ext4 or btrfs. In the case of vfs_streams the max limit of the stream size being bounded by what's set by in smb.conf for "smbd max xattr size", whereas with fruit:resource = xattr the limit is set by the linux kernel config.

This explains why fruit:resource = xattr worked for me and preserved additional data streams, even though there was a bug that meant fruit:resource = stream couldn't.

- In response to Ralph, as I showed above, it seems that the admonition in the vfs_fruit manpage is inarftully crafted or an anachronism, as these days xattribs are handled just fine by the Linux vfs within the limits set by the linux kernel. Yes, by default they are limited to a rather small 64KB per xattr, but that can be configured to be larger.

This then leads to the question:
Has vfs_streams_xattr outlived its usefulness as an experimental option. And, would it not be simpler to just have folks work with Alternative Data Streams via fruit:resource = xattr using whatever their kernel and filesystem allow? While also making the smb.conf and vfs_fruit man pages clearer, so that people are aware of the default limits, how to change them, and any gotchas mentioned in the xattr manpage https://manpages.ubuntu.com/manpages/jammy/man7/xattr.7.html
Comment 31 Samba QA Contact 2022-06-20 14:25:03 UTC
This bug was referenced in samba master:

238b2cbb8f352375c448d86b462f13752640e16b
808a7b8b76dbcaac1db0508fd410d0bcf702af7a
Comment 32 Noel Power 2022-06-20 16:19:31 UTC
Created attachment 17374 [details]
backport for 4.15
Comment 33 Noel Power 2022-06-20 16:19:56 UTC
Created attachment 17375 [details]
backport for 4.16 & 4.15
Comment 34 Jeremy Allison 2022-06-20 17:23:42 UTC
Comment on attachment 17374 [details]
backport for 4.15

Doesn't compile. You still have the master-only SMB_ASSERT(fsp_is_alternate_stream(smb_fname->fsp));
line.
I think the back-port for 4.16.x might work for 4.15 instead.
Comment 35 Noel Power 2022-06-20 20:07:17 UTC
(In reply to Jeremy Allison from comment #34)
I'm sorry, not sure what happened I backported both from the orig master patch and compiled and tested both, I can only assume I didn't to the git add after resolving the conflict & make error (oops)

Anyway, turns out you are right and the 4.16 applies for both, so I renamed it
Comment 36 Jeremy Allison 2022-06-20 21:41:24 UTC
Re-assigning to Jule for inclusion in 4.16.next, 4.15.next.
Comment 37 Jule Anger 2022-06-23 07:35:05 UTC
Pushed to autobuild-v4-{16,15}-test.
Comment 38 Samba QA Contact 2022-06-23 08:44:09 UTC
This bug was referenced in samba v4-16-test:

81dc0832eee7af5c7989c799c69b7845940b428e
58bdf100b2bfc852a5d7f499771395bf4062ec74
Comment 39 Samba QA Contact 2022-06-23 09:50:04 UTC
This bug was referenced in samba v4-15-test:

31d9de1405c8483187070492f479f4ed40e94651
86e9958156cc1df69bf8bafa8e28df7ce39a0982
Comment 40 Jule Anger 2022-06-23 10:12:00 UTC
Closing out bug report.

Thanks!
Comment 41 Samba QA Contact 2022-06-28 06:52:33 UTC
This bug was referenced in samba v4-15-stable (Release samba-4.15.8):

31d9de1405c8483187070492f479f4ed40e94651
86e9958156cc1df69bf8bafa8e28df7ce39a0982