Samba aborts when deleting a file and "vfs_streams_xattr" is used.

[Mike Silva]

[ Impact ]

Samba users who create shares that use vfs_fruit and the option "fruit:resource = stream" will face a crash with SIGABRT when trying to delete a file.

[ Test Plan ]

Thanks to Mike Silva for coming up with a simple reproducer for this bug.

$ lxc launch ubuntu-daily:jammy samba-bug1977491
$ lxc shell samba-bug1977491
# apt update
# apt install -y samba smbclient
# mkdir /testshare
# chown ubuntu:ubuntu /testshare
# $EDITOR /etc/samba/smb.conf

Add the following snippet inside the "[global]" section of the smb.conf file:

fruit:resource = stream
vfs objects = fruit streams_xattr

# cat >> /etc/samba/smb.conf << _EOF_
[testshare]
   path = /testshare
   browseable = yes
   read only = no
   guest ok = yes
   writable = yes
_EOF_
# systemctl restart smbd.service
# smbpasswd -a ubuntu

(Choose an easy password here)

# cat > testfile << _EOF_
this is a test file
_EOF_
# smbclient '\\127.0.0.1\testshare' -U ubuntu
...
smb: \> put testfile
smb: \> rm testfile

You should see an error like this:

NT_STATUS_CONNECTION_DISCONNECTED deleting remote file \test
NT_STATUS_CONNECTION_DISCONNECTED listing \test
smb: \> SMBecho failed (NT_STATUS_CONNECTION_DISCONNECTED). The connection is disconnected now

And if you inspect the file /var/log/samba/log.samba-bug1977491, you should see a panic message:

[2022/06/17 23:30:01.597926, 0] ../../lib/util/fault.c:183(smb_panic_log)
  PANIC (pid 2649): assert failed: smb_fname->fsp != NULL in 4.15.5-Ubuntu
[2022/06/17 23:30:01.603078, 0] ../../lib/util/fault.c:287(log_stack_trace)
  BACKTRACE: 29 stack frames:
   #0 /lib/x86_64-linux-gnu/libsamba-util.so.0(log_stack_trace+0x34) [0x7f077cad6f44]
   #1 /lib/x86_64-linux-gnu/libsamba-util.so.0(smb_panic+0xd) [0x7f077cad74fd]
   #2 /usr/lib/x86_64-linux-gnu/samba/vfs/streams_xattr.so(+0x49a8) [0x7f077796e9a8]
   #3 /usr/lib/x86_64-linux-gnu/samba/vfs/fruit.so(+0xcf32) [0x7f0777953f32]
   #4 /usr/lib/x86_64-linux-gnu/samba/vfs/fruit.so(+0xd392) [0x7f0777954392]
   #5 /usr/lib/x86_64-linux-gnu/samba/vfs/catia.so(+0x6f1d) [0x7f077793ff1d]
... backtrace truncated ...

N.B.: You might also want to test with "fruit:time machine = yes" for completeness.

[ Where problems could occur ]

The patch that fixes the issue has been recently proposed upstream, and as such it hasn't been extensively tested by the Samba community in general. Both the reporter, the Samba developers and I tried to make sure that different scenarios are working correctly with the patch applied, but there is a small risk that some corner hasn't been covered.

Even if that is indeed the case, it is hard to talk about "regressions" here when the previous behaviour was a SIGABRT crash. The Samba developers were very responsive regarding this issue, so I'm sure that, if a problem happens, we can work together to come up with a solution fairly quickly.

[ Original Description ]

In Ubuntu 22.04 LTS, there is currently a regression in Samba 4.15.5 and a new issue, both of which cause connection issues with macOS clients. The regression causes segfaults with macOS client connections. The new issue, breaks durable handles, which are crucial to Samba's support of Time Machine over SMB (using vfs_fruit).

For the regression:
https://bugzilla.samba.org/show_bug.cgi?id=14900
For the new issue:
https://bugzilla.samba.org/show_bug.cgi?id=15022

This regression was fixed in Samba 4.15.6, see:
https://www.samba.org/samba/history/samba-4.15.6.html

The new issue in 4.15.7, see:
https://www.samba.org/samba/history/samba-4.15.7.html

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Mike Silva (mikesilva)]

Just to be clear on these two issues, they make macOS clients connecting to Samba unreliable, and even if you can connect nearly all file transfers fail.

[Sergio Durigan Junior (sergiodj)]

Thanks for reporting the bug and helping to make Ubuntu better.

I don't have access to a proprietary MacOS system, so it is really hard for me to reproduce the bug and test the patches you linked above. I would like to ask for you help to do that.

I've prepared a PPA with samba and the patches backported from upstream:

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

Can you please give it a try and report back with the results?

Also, I noticed that the second issue you mentioned (upstream bug 15022) is fixed by an 18-commit series. Although the patches apply cleanly against our version of samba in Jammy, this can be a bit tricky to SRU. I'm thinking whether it'd make sense to consider doing an MRE instead. But let's worry about this after we verify that the patches really fix the issue you're having.

Thanks.

[Mike Silva (mikesilva)]

I’ll have some time to give it a go over the weekend and report back. Thanks.

[Mike Silva (mikesilva)]

Nope. I tried 22.04, installing Samba with the patches from your PPA, and while I can connect to the server, as soon as I try to access a share mount point, Finder on my Mac crashes.

I don't see anything terribly useful in either logs on my Mac or /var/log/samba. I'll attach a zip if you want to peruse, but there's zero useful info.

[Mike Silva (mikesilva)]

I realized I made the stupid error of not using smbpasswd to instantiate my samba user on my fresh 22.04 install, so I ran another test and got some useful output. Attached is a zip of /var/logs/samba on 22.04, running your patch.

[This is a fresh install of 22.04, but the directories that Samba is sharing are on a separate drive from the OS, thus remain unchanged from what Samba on the two previous Ubuntu versions were able to share successfully.]

I was running atop at the time, to see what was going on. And, as soon as my Mac tried to mount the share after successfully login into the samba server, I saw a bunch of smb processes start as though smbd was forking out of control.

My smb.conf is pretty stock, using probably 98% of the default configuration as installed by the package. With the notable exception of the following:
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

The volume with the data I'm sharing is on btrfs, purposely created with nodesize= 65536 [eg 64KB], so that it can handle more xattr bits.

Finally, I'm using the same smb.conf that worked absolutely flawlessly with Samba in Ubuntu 21.10 and 20.04 before that. And, I've combed through the lines I've added, line by line to make sure that all of the configuration options I've selected are still supported in Samba 4.15.x, which is shipped in Ubuntu 22.04.

[Mike Silva (mikesilva)]

I did some further testing, using a Ubuntu 22.04 VM on my Intel Mac to connect to the same Samba share on the physical Ubuntu 22.04 server (which I haven't previously mentioned is running on a Raspberry Pi 4.)

In the test case:

1) I successfully logged in and copied about a GB of an Ubuntu gzip up to the physical server from the VM.
2) I then tried to delete the gzip bits from the share.

Result:
The same panic as when my Mac is trying to use the share. Thus, while your patch has made it possible to reliably connect to the server from my Mac, and the durable handles patch is definitely required for Time Machine support, it seems there may be another underlying issue that does not seem to be platform dependent. Rather, judging from the logs, it looks like there is an additional issue with vfs_streams_xattr that didn't exist in the previous version of Samba that shipped with Ubuntu 21.10 and 20.04.

I will attach the logs with the panic information here.

[Sergio Durigan Junior (sergiodj)]

Thanks for the extra information and investigation, Mike.

Out of curiosity, are you able to reproduce this bug using the samba package that's currently in Kinetic (development version)? The package has recently been updated to 4.16 and I'd like to know if this bug is still present in that version.

[Mike Silva (mikesilva)]

How can I easily accomplish installing that on 22.04?

[Sergio Durigan Junior (sergiodj)]

If you're OK with upgrading your system to a development release, then you can perform:

# do-release-upgrade -d

It should guide you through the steps and upgrade your system. After rebooting the machine/VM, you will be using the development release of Ubuntu.

I strongly suggest you do this on a VM and not on your main system.

[Mike Silva (mikesilva)]

Thanks, for the suggestions. I'm setting up a VM now for further testing. I'll let you know.

However, while I wait I did a little experiment.

I made the following change to my smb.conf:
fruit:resource = xattr

But, I left streams_xattr in the same position in the vfs configuration line, rather than removing it.

My server now works. Which is a bit bizarre, because though I have not delved into the code, it is my understanding that both "resource = xattr" and "resource = stream" shovel the alternative data stream bits down to the next vfs module in the stack to be handled, which in both cases would be "streams_xattr" coming after "fruit".

[Mike Silva (mikesilva)]

I ran tests against the 22.10 daily for 6/13/22 in a VM, attaching the same drive I use for my Samba shares on the physical server, and reproduced the same behavior as above.

With my smb.conf configured as in comment #6 above, I could log in, but as soon as I tried to use the share by coping 1GB of bits up, the same panic happened. I will attach the log below.

With my smb.conf configured with fruit:resource = xattr, then I could connect, copy, and delete bits without issue.

Again, I haven't done a code deep dive, but very late last night I did a cursory search for recent changes to samba, vfs_fruit, vfs_streams_xattr, vfs_btrfs, and vfs_io_uring that seemed to touch how samba handles xattrs and streams. I did notice that about 4 months ago (don't quote me), there was a push to update vfs_fruit to fix a fairly severe CVE to do with boundary conditions. Perhaps, the fix(es) for that were such that fruit:resouce = xattr is properly handling extended attributes up to the stock Linux kernel max for ext4/btrfs of 64KB, but now fruit:resource = stream is not? Don't quote me on this...just spitballing. :-)

[Sergio Durigan Junior (sergiodj)]

Thanks a lot for the extensive investigation, Mike.

Since you couldn't reproduce the bug using the latest version of Samba from Kinetic, then I believe we need to resort to upstream's knowledge now. We're currently shipping samba 4.16.1 on Kinetic, which is pretty recent: upstream released 4.16.2 yesterday (June 13). The list of fixes from 4.16.2 doesn't look like something that could help with this issue: https://www.samba.org/samba/history/samba-4.16.2.html.

As I don't have access to a proprietary MacOS system to test and try to reproduce this problem, I would like to ask you to open a bug against upstream and see if they can help pinpoint the issue here. When you open the bug, please leave a link to it here in the comments so that we can track it as well.

Thanks.

[Mike Silva (mikesilva)]

Actually, as I mentioned, I can reproduce the bug on Kinetic in the same way it reproduces on Jammy.

If you use fruit:resouce = stream, you hit the bug and get a panic.
If you use fruit:resource = xattr, no bug.
And it doesn't matter what OS the client machine is using, which was the point of testing it using a Linux client too.

Further, I don't particularly get why you keep mentioning that macOS is "proprietary". Canonical is shipping Samba, which enables Ubuntu machines to be a File, Print, and AD/DC machine for *many* different OSes, many of them "proprietary". It is a reimplementation of "proprietary", yet now openly documented protocols: CIFS & SMB. As a fellow engineer, I'd say it's on Canonical to therefore ship said software making a best effort to insure it actually works. That, therefore, doesn't absolve Canonical or upstream from having a lab, test harness with requisite example hardware, and QA process to make a reasonable effort to see that the bits shipped work.

My Mac connects just fine to a "proprietary" Windows server over Microsoft and Apple's implementation of SMB. Which means Apple, implemented the openly documented SMB protocol, better than upstream and Canonical. And, if I reach out to either Microsoft or Apple, because my Linux box(es) can't connect to their OSes over SMB, they don't scoff because Linux is "open". They investigate, and fix the problem.

Not trying to be a dick, but there's an attitudinal problem here in many open source projects and distributions, that leads to nonsense resource allocation decisions, that in turn lead to crappy bugs like this that wouldn't ship, if the attitude didn't make people think there is an excuse for insufficient effort and care.

I'll contact upstream. But you guys have a bug, you're perhaps inheriting from them...perhaps not..., because nobody upstream or at Canonical is making a de minimis effort to properly QA the software to see that it's fit for stated purpose.

[Mike Silva (mikesilva)]

I've reported this issue upstream to the Samba team. You can find the bug with them, here:
https://bugzilla.samba.org/show_bug.cgi?id=15099

[Mike Silva (mikesilva)]

Sergio,

One change for the test case:
You'll want to ensure that fruit:time machine = yes is configured on a test share. This will turn on Time Machine compatibility, which will by default enable durable handles and several other options that it would be good to exercise, even if the test client is not running macOS.

In that respect, it may be good to have a test share that has that enabled, and one configured as you have above without, so that the regression suite tests against fruit being enabled with and without all of the additional bells and whistles to enable Time Machine.

[Mike Silva (mikesilva)]

Seems to be good on Jammy, I've done a little bit more than than the simple cp then rm test to 'kick the tires' a bit more, since I've had so much trouble since the upgrade.

I'll now get the daily of Kinetic installed and the patch applied to give it a go there. Since the bug Jeremy at Samba.org fixed is so 'obvious', I'm guessing we'll be fine on Kinetic too, but better to find issues in the dev stage than after release.

After I finish with Kinetic testing, I'll return here and the Samba bugzilla to deliver overall testing results.

[Mike Silva (mikesilva)]

Looks like the patchset in the PPA fixes the bug in both Jammy and Kinetic.

[Sergio Durigan Junior (sergiodj)]

Thanks for testing the patches.

I am working on the SRU now; I should put an MP up for review by EOD.

[Mike Silva (mikesilva)]

Sergio, thanks for updating the bug title and description to be more apt.

[Launchpad Janitor (janitor)]

This bug was fixed in the package samba - 2:4.16.1+dfsg-8ubuntu2

---------------
samba (2:4.16.1+dfsg-8ubuntu2) kinetic; urgency=medium

  * Fix abort when deleting a file and "fruit:resource = stream" is
    used. (LP: #1977491)
    - d/p/lp1977491-dont-crash-on-vfs_fruit-resource-stream-01.patch:
      Add test that shows smbd crashing when deleting a file while using
      vfs_fruit with "fruit:resource = stream".
    - d/p/lp1977491-dont-crash-on-vfs_fruit-resource-stream-02.patch:
      Handle file deleting when "fruit:resource = stream" is used.

 -- Sergio Durigan Junior <email address hidden> Mon, 20 Jun 2022 19:09:25 -0400

[Mike Silva (mikesilva)]

Sergio,

Should the regression test recommendation require a share with fruit:time machine = yes, rather than just being another optional test case? (Regardless, of whether the client running the test against the server is macOS or not, since we've seen that's not a dependency.) I never tested a share without that set. Did you?

[Sergio Durigan Junior (sergiodj)]

On Thursday, June 23 2022, Mike Silva wrote:

> Should the regression test recommendation require a share with
> fruit:time machine = yes, rather than just being another optional test
> case? (Regardless, of whether the client running the test against the
> server is macOS or not, since we've seen that's not a dependency.) I
> never tested a share without that set. Did you?

Yes, I did test with and without the "fruit:time machine = yes" setting.
Both scenarios fail as expected, and I believe it's worth test them
separately.

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

[Mike Silva (mikesilva)]

Yup, I agree. Sorry, I thought your test instructions weren’t making both cases a requirement.

[Robie Basak (racb)]

> Even if that is indeed the case, it is hard to talk about "regressions" here when the previous behaviour was a SIGABRT crash.

Only in the case of using vfs_fruit, though, right? What if someone is using vfs_streams_xattr (the module being patched here) with a Windows client to store an NTFS-like alternate data stream? I think that is expected to work (and not crash) despite this bug, right?

Can I suggest that this use of the module is tested if possible please, as part of your Test Plan, to make sure it hasn't regressed?

[Robie Basak (racb)]

Hello Mike, or anyone else affected,

Accepted samba into jammy-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/samba/2:4.15.5~dfsg-0ubuntu5.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-jammy to verification-done-jammy. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-jammy. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Mike Silva (mikesilva)]

Robie,

I'll have some time this evening or this weekend to test the package.

With regards to your initial question, I think there is some confusion from my earlier comment. What I meant in my query to Sergio was that I thought the following two tests cases should be run:
1) with the vfs streams_xattr configured
2) with vfs_streams_xattr AND vfs_fruit configured

This is because fruit is passing xattribs and Alternate Data Streams [ADSes] down to vfs_streams_xattr, when configured to use streams. Thus, it would be good to insure that any deltas in vfs_streams_xattr doesn't break the interaction with vfs_fruit.

I noted in an earlier comment here or in the linked Samba bugzilla that this particular bug was reproducible even when using a Linux client that wasn't trying to manipulate xattribs or ADSes. Therefore, and given the nature of the bug (see the Samba bugzilla), I'd expect a Windows client doing simple file operations, manipulating xattribs, or Alternate Data Streams [ADS] would likewise fail, in both test cases 1 & 2. It's perfectly valid for a Windows client to mount a share running vfs_fruit, and if it does, it is my understanding xattribs and ADSes are still being passed from fruit to streams_xattr.

Sergio pruned the main test case in the bug down to the absolute minimum to reproduce the bug. But he agreed with me above, that it would be good to have some regression tests that verify changes to both vfs_fruit or streams_xattr do not break streams_xattr alone or when layered under fruit, by running test case 1 & 2.

And, I think that you bring up an excellent point that for a regression suite it would be worth having additional test cases that not only do simple file operations with all OS clients, but also specifically run tests 1 & 2 with both a macOS AND Windows client manipulating xattribs or ADSes on the share too, as I believe that both Windows and macOS are the only client OSes that natively use ADSes.

[Mike Silva (mikesilva)]

adding...

It might be good to update the bug title and body to indicate this is really a bug in streams_xattr.

With the added detail that it breaks all client OSes doing simple file operations when streams_xattr is enabled, but additionally will break both macOS clients and Windows clients manipulating xattribs or ADSes. As people running those client OSes are most likely...perhaps the only people...to have shares configured as in test case 1 & 2. Thus, changing the bug title and body will help clarify the primary issue, while also letting macOS and Windows users know this is a bug and patch that is relevant to their use cases.

[Mike Silva (mikesilva)]

I have tested the following configurations that used to fail , with Jammy Proposed, and found the bug does not reproduce.

From macOS client
1) Fruit enabled, streams enabled, file with no Alternate Data Stream created/deleted = Pass
2) Fruit enabled, streams enabled, file with ADS created/deleted = Pass
3) Fruit disabled, streams enabled, file with ADS created/deleted = Pass

From Ubuntu Jammy client
1) Fruit enabled, streams enabled, no Alternate Data Stream created/deleted = Pass
2) Fruit enabled, streams enabled, file with ADS created/deleted = Pass
3) Fruit disabled, streams enabled, file with ADS created/deleted = Pass

From Windows 11 client
) Fruit enabled, streams enabled, no Alternate Data Stream created/deleted = Pass
2) Fruit enabled, streams enabled, file with ADS created/deleted = Pass
3) Fruit disabled, streams enabled, file with ADS created/deleted = Pass

For posterity, because ADS is such a niche hellscape with each client OS using them a bit differently, here are the means with which I tested in an appropriate manner to each client OS and with an eye to being able to script in regression testing.

macOS
------------
- mount your test share with smbclient
- use commandline touch or text editor of choice to create a test text file, without ADS, then delete it.

- use commandline touch text editor of choice to create test text file, then add an ADS
 - after flat text file created and saved to share from touch or editor, you need to add the ADS, this is done by using the xattr command, like so:
  1) First verify the saved file has no ADSes, with the command:
   ls -l ./
   If ls returns the filename without showing an @, there is no ADS.
  2) Now we will use xattr to write an ADS to the file:
   xattr -w TestADS 'BlahBlah' testtext.txt
  3) Verify the ads has been written with:
   ls -l ./
  4) Verify the contents of the ADS with:
   xattr - p TestADS testfile.txt
  5) Verify the output is the string you input to the ADS stream.
  6) Delete the test file with:
   rm ./*
- Alternatively, use a GUI based tool like TextEdit on the Mac to create and save a file. This will necessarily create and save the file with various ADSes macOS uses. ADSes with complex names and values. Or, which could include binary data.

Linux
-----
- mount your test share with mount.cifs, specifying the user_xattr option.
- use commandline touch or text editor of choice to create a test text file, without ADS, then delete it.

- use commandline touch text editor of choice to create test text file, with the intention of adding ADS. mount.cifs allows extended attributes of the type .user by default.
  1) First verify the saved file has no ADSes, with the command:
   getfattr -R ./
   no output means there are no ADSes.
  2) Now we will use setfattr to write an ADS to the file:
   sudo setfattr -n user.TestADS -v 'BlahBlah' testfilewADSes.txt
  3) Verify the ads has been written with:
   getfattr -R ./
   Which should result output showing the ADS user.TestADS
  4) Verify the contents of the ADS with:
   getfattr -n user.TestADS testfile.txt
  5) Verify the output is the string you input to the ADS stream.
   BlahBlah
  6) Delete the files with
   rm ./*

...

[Mike Silva (mikesilva)]

I changed the bug title to reflect reality.

[Paride Legovini (paride)]

Hello Mike,

Given that you performed verification using the package from jammy-proposed, and that the bug doesn't reproduce in all the configurations you tested, I'm changing the "verification-needed" tags of this bug report to "verification-done".

Next time feel free to do this yourself after performing SRU verification so to speed up the process.

Thanks!

[Mike Silva (mikesilva)]

Thank you, I didn't see that.

Is the server-todo flag to indicate it needs to be integrated into a future server release? Or, is that about testing on the server flavor specifically, because, I am running server.

[Sergio Durigan Junior (sergiodj)]

The server-todo tag is an internal tag that we use in order to track our work items. Since this bug has been successfully verified (thanks), I will remove it.

[Launchpad Janitor (janitor)]

This bug was fixed in the package samba - 2:4.15.5~dfsg-0ubuntu5.1

---------------
samba (2:4.15.5~dfsg-0ubuntu5.1) jammy; urgency=medium

  * Fix abort when deleting a file and "fruit:resource = stream" is
    used. (LP: #1977491)
    - d/p/lp1977491-dont-crash-on-vfs_fruit-resource-stream-01.patch:
      Add test that shows smbd crashing when deleting a file while using
      vfs_fruit with "fruit:resource = stream".
    - d/p/lp1977491-dont-crash-on-vfs_fruit-resource-stream-02.patch:
      Handle file deleting when "fruit:resource = stream" is used.

 -- Sergio Durigan Junior <email address hidden> Tue, 21 Jun 2022 16:31:40 -0400

[Brian Murray (brian-murray)]

The verification of the Stable Release Update for samba has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.