Bug 15314 - streams_xattr is creating unexpected locks on folders
Summary: streams_xattr is creating unexpected locks on folders
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: 4.17.5
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jule Anger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-02-18 23:59 UTC by Chris P
Modified: 2023-03-09 09:22 UTC (History)
1 user (show)

See Also:


Attachments
Raw untested patch. (445 bytes, patch)
2023-02-21 19:20 UTC, Jeremy Allison
no flags Details
Raw tested patch for master. (1.07 KB, patch)
2023-02-28 01:11 UTC, Jeremy Allison
no flags Details
git-am fix for master. (6.89 KB, patch)
2023-02-28 19:36 UTC, Jeremy Allison
no flags Details
git-am fix for 4.18.next, 4.17.next. (7.43 KB, patch)
2023-03-03 18:45 UTC, Jeremy Allison
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chris P 2023-02-18 23:59:25 UTC
Hi Samba maintainers,

If streams_xattr is enabled and I write a new ADS (alternate data stream) to a file, the parent folder is seemingly locked and stays locked until smbd is restarted.

I'm on Samba 4.17.5 on Debian testing, and I'm testing with a Windows 11 22H2 client.

I can reproduce like this, in PowerShell. I have a r/w share mounted at Z:.

PS Z:\>mkdir test
PS Z:\>cd test
PS Z:\test> echo 1 > test.mkv
PS Z:\test> Set-Content .\test.mkv:asdf -Value "testing"

The ADS is created properly, and you can see it on the server side with getfattr.

But now the folder "Z:\test" is locked in the Windows File Explorer, and cannot be renamed. I turned up logging in Samba and can see the error NT_STATUS_ACCESS_DENIED whenever I try to rename it, specifically being returned by the internal function "can_rename". Restarting Samba resolves the issue.

Perhaps secondary to this, the lock or lease or whatever it may be that is blocking the rename is also not visible in smbstatus.

I can provide additional info or debugging if this is not easily reproduced. Thanks for your work on samba!
Comment 1 Chris P 2023-02-19 11:54:29 UTC
An update: it isn't just when writing a new ADS. If I even open a file in a folder at all (like, say, a text file in Notepad) in Win 11 22H2, the parent folder then cannot be renamed, even after the file is closed! It can be deleted, but not renamed. It's the same behavior: NT_STATUS_ACCESS_DENIED on the server side, the typical lock error in Windows, but nothing in Windows has a handle on any files in the tree (at least that I can see with Process Explorer).

If I restart smbd without streams_xattr, as in, no vfs objects at all, I cannot reproduce this. It reproduces as soon as I add streams_xattr back in.

I did a little bit of digging in the code and added some debug prints; the code path that it's always hitting is here:

https://github.com/samba-team/samba/blob/master/source3/smbd/smb2_reply.c#L1156

can_rename fails when it gets a true response from file_find_subpath. If I print the locals for each loop inside file_find_subpath it looks as though there is a dangling fsp for the parent folder that was not closed, as it's not visible in smbstatus and not open on any machine. But this is coming from an outsider's perspective with no experience on this code base, so do with that what you will!

I suspect there is some kind of access pattern that is exposing this bug. It may relate to the antimalware service on Windows 11, but I cannot say for sure.
Comment 2 Chris P 2023-02-20 09:37:19 UTC
Second update: I bisected by building release versions back to 4.15.9, which did not exhibit the bug. It was introduced with 4.17.0 and has appeared in each release since.
Comment 3 Jeremy Allison 2023-02-21 19:20:27 UTC
Created attachment 17763 [details]
Raw untested patch.

Can you try this raw and utterly untested patch. Looks like file_find_subpath() is treating fsp's opened for traversal the same as fsp's opened by the client.

This patch will allow file_find_subpath() to ignore traversal fsp's.

If it works we'll still need regression tests etc., but this might show the way.
Comment 4 Chris P 2023-02-22 02:47:14 UTC
Yep, just built master with your patch and it resolves the issue in all my test cases.
Comment 5 Chris P 2023-02-22 02:48:37 UTC
I do wonder why, though, this only triggers when streams_xattr is set. Shouldn't this code path have the same issue regardless of VFS? Are traversal fsp's cleaned up somehow in the default case?
Comment 6 Jeremy Allison 2023-02-22 19:24:52 UTC
(In reply to Chris P from comment #5)

Yes, I was wondering that myself :-). I'm guessing it's something to do with how streams_xattr opens the containing directory to get an fd for EA access, but without spending a lot of time going into it I'm not sure right now. The fix is obviously good (it mirrors the logic in all of the other fsp enumeration calls) so  once I'm back working I'll create a regression test and then submit the fix to gitlab.
Comment 7 Ralph Böhme 2023-02-22 20:17:53 UTC
(In reply to Jeremy Allison from comment #6)
Fwiw, been following along and my educated guess would be that your fix is only paving over the issue and the real problem is that we're somewhere leaking an internal fsp. If I had to guess where to look first, I would start with the generic streams code first, my gut feeling tells me streams_xattr is likely not the culprit.
Comment 8 Jeremy Allison 2023-02-22 23:28:42 UTC
(In reply to Ralph Böhme from comment #7)

Sure, we may be leaking an internal fsp inside the streams code.

However, the fix itself is the correct thing to do in this case - it matches the logic inside file_find_dif(), file_find_di_first() and file_find_di_next().

Once school vacation week is over I'll take a look and see if I can find the specific open fsp that's not being closed.
Comment 9 Jeremy Allison 2023-02-27 19:50:21 UTC
OK, I just tested this with current master and cannot reproduce.
Comment 10 Jeremy Allison 2023-02-27 20:03:40 UTC
Spoke too soon. I can reproduce the first reported problem in master using the:

Set-Content .\test.txt:asdf -Value "testing"

command. But not the second (open via notepad causes parent folder to disallow rename).
Comment 11 Jeremy Allison 2023-02-27 22:32:11 UTC
openat_pathref_fsp() is leaking the base_fsp for a stream name somehow. Still investigating.
Comment 12 Jeremy Allison 2023-02-27 23:23:09 UTC
Hmmm. Actually looks like openat_pathref_fullname() is where the bug is. Still investigating.
Comment 13 Jeremy Allison 2023-02-27 23:48:28 UTC
Ah, think I've found it...
Comment 14 Jeremy Allison 2023-02-28 01:02:35 UTC
The leak is here (in master) source3/smbd/filename.c:filename_convert_dirfsp_nosymlink()

1336         base_fsp = smb_fname_rel->fsp;
1337         smb_fname_fsp_unlink(smb_fname_rel);
1338         SET_STAT_INVALID(smb_fname_rel->st);
...
1376         if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND)) {
1377                 /*
1378                  * Creating a new stream
1379                  *
1380                  * We should save the already-open base fsp for
1381                  * create_file_unixpath() somehow.
1382                  */
1383                 smb_fname = full_path_from_dirfsp_atname(
1384                         mem_ctx, smb_dirname->fsp, smb_fname_rel);
1385                 if (smb_fname == NULL) {
1386                         status = NT_STATUS_NO_MEMORY;
1387                         goto fail;
1388                 }
1389                 goto done;
1390         }

When When open_stream_pathref_fsp() returns NT_STATUS_OBJECT_NAME_NOT_FOUND, smb_fname_rel->fsp is set to zero, leaving base_fsp leaking. I'm trying to figure out the right way to cleanly close this.
Comment 15 Jeremy Allison 2023-02-28 01:11:09 UTC
Created attachment 17780 [details]
Raw tested patch for master.

This one fixes it without leaving a file descriptor leak in place. Now for the regression test.
Comment 16 Jeremy Allison 2023-02-28 04:33:01 UTC
Reproducer using smbclient:

$ touch file.txt
$ smbclient //server/share_with_streams_xattr_vfs -Uuser%pass
smb: \> mkdir test
smb: \> put file.txt test\file.txt
putting file file.txt as \test\file.txt (536.6 kb/s) (average 536.6 kb/s)
smb: \> get test\file.txt:abcf
NT_STATUS_OBJECT_NAME_NOT_FOUND opening remote file \test\file.txt:abcf
smb: \> rename test test1
NT_STATUS_ACCESS_DENIED renaming files \test -> \test1 

The line get test\file.txt:abcf causes the fd and fsp leak when filename_convert_dirfsp_nosymlink() goes down the NT_STATUS_OBJECT_NAME_NOT_FOUND path when opening a non-existent stream name.
Comment 17 Jeremy Allison 2023-02-28 19:36:06 UTC
Created attachment 17784 [details]
git-am fix for master.

Full fix including regression test. Currently in gitlab-ci.
Comment 19 Jeremy Allison 2023-02-28 22:06:03 UTC
Official MR: https://gitlab.com/samba-team/samba/-/merge_requests/2953

Passes ci.
Comment 20 Samba QA Contact 2023-03-03 16:38:04 UTC
This bug was referenced in samba master:

5a3db5105bd8360b245cd35810002740ccff605c
c54bec26ad23b0121b2ddfbf04bc81050f27e6e1
3f84a6df4546e0f1e62dfbcd0b823ea29499a787
Comment 21 Jeremy Allison 2023-03-03 18:45:44 UTC
Created attachment 17790 [details]
git-am fix for 4.18.next, 4.17.next.

Applies cleanly to 4.18.next, 4.17.next.
Comment 22 Jeremy Allison 2023-03-07 17:59:30 UTC
Comment on attachment 17790 [details]
git-am fix for 4.18.next, 4.17.next.

Ping ! Can we get this into 4.18.final and 4.17.next please ?
Comment 23 Ralph Böhme 2023-03-07 20:34:03 UTC
Reassigning to Jule for inclusion in 4.17 and 4.18.
Comment 24 Jule Anger 2023-03-08 09:02:00 UTC
Pushed to autobuild-v4-{18,17}-test.
Comment 25 Samba QA Contact 2023-03-08 10:12:04 UTC
This bug was referenced in samba v4-17-test:

1caac94128e66884c7a844ddb3c6ef1f02d20bdc
460bc1897a3031728a505e660155f55a0762e5c8
ec6a057e6908408c7d64f6da7e5b11503d14a144
Comment 26 Samba QA Contact 2023-03-08 11:17:12 UTC
This bug was referenced in samba v4-18-test:

f2c9d59e5a3c66156a20d297c8660ab2609bedb5
3fb8f2c579cf13fd7d0367ace97d8d2ff5d2c5ac
800f4f9cc9dba727cdca44b3f799cfa83f5f0854
Comment 27 Jule Anger 2023-03-08 12:04:52 UTC
Closing out bug report.

Thanks!
Comment 28 Samba QA Contact 2023-03-08 12:29:29 UTC
This bug was referenced in samba v4-18-stable (Release samba-4.18.0):

f2c9d59e5a3c66156a20d297c8660ab2609bedb5
3fb8f2c579cf13fd7d0367ace97d8d2ff5d2c5ac
800f4f9cc9dba727cdca44b3f799cfa83f5f0854
Comment 29 Samba QA Contact 2023-03-09 09:22:52 UTC
This bug was referenced in samba v4-17-stable (Release samba-4.17.6):

1caac94128e66884c7a844ddb3c6ef1f02d20bdc
460bc1897a3031728a505e660155f55a0762e5c8
ec6a057e6908408c7d64f6da7e5b11503d14a144