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.
Created attachment 17363 [details] Logs from reproducing the panic with an Ubuntu 22.04 client
One for Ralph I think.
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.
(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 !
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.
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.
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.
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.
Great, that should be enough to reproduce, thanks !
Reproduces on master, thanks !
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().
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 ?
Let me hit the samba wiki to see how to do a build and give it a go.
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.
(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.
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.
(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.
(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...
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.
(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?
(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.
(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.
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.
Created attachment 17371 [details] git-am fix for master. Fix (including regression test) I'm planning to submit to gitlab-ci.
https://gitlab.com/samba-team/samba/-/merge_requests/2584
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.
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?
(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.
(In reply to Mike Silva from comment #27) Don't use fruit:resource=xattr unless running Solaris, cf man vfs_fruit.
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
This bug was referenced in samba master: 238b2cbb8f352375c448d86b462f13752640e16b 808a7b8b76dbcaac1db0508fd410d0bcf702af7a
Created attachment 17374 [details] backport for 4.15
Created attachment 17375 [details] backport for 4.16 & 4.15
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.
(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
Re-assigning to Jule for inclusion in 4.16.next, 4.15.next.
Pushed to autobuild-v4-{16,15}-test.
This bug was referenced in samba v4-16-test: 81dc0832eee7af5c7989c799c69b7845940b428e 58bdf100b2bfc852a5d7f499771395bf4062ec74
This bug was referenced in samba v4-15-test: 31d9de1405c8483187070492f479f4ed40e94651 86e9958156cc1df69bf8bafa8e28df7ce39a0982
Closing out bug report. Thanks!
This bug was referenced in samba v4-15-stable (Release samba-4.15.8): 31d9de1405c8483187070492f479f4ed40e94651 86e9958156cc1df69bf8bafa8e28df7ce39a0982
The title of this bug was erroneous. The root cause is having vfs_streams_xattr enabled, even if fruit is not in use.
This bug was referenced in samba v4-16-stable (Release samba-4.16.3): 81dc0832eee7af5c7989c799c69b7845940b428e 58bdf100b2bfc852a5d7f499771395bf4062ec74