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!
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.
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.
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.
Yep, just built master with your patch and it resolves the issue in all my test cases.
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?
(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.
(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.
(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.
OK, I just tested this with current master and cannot reproduce.
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).
openat_pathref_fsp() is leaking the base_fsp for a stream name somehow. Still investigating.
Hmmm. Actually looks like openat_pathref_fullname() is where the bug is. Still investigating.
Ah, think I've found it...
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.
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.
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.
Created attachment 17784 [details] git-am fix for master. Full fix including regression test. Currently in gitlab-ci.
https://gitlab.com/samba-team/devel/samba/-/merge_requests/new?merge_request%5Bsource_branch%5D=jra-fix-bug-15314 https://gitlab.com/samba-team/devel/samba/-/pipelines/791753884
Official MR: https://gitlab.com/samba-team/samba/-/merge_requests/2953 Passes ci.
This bug was referenced in samba master: 5a3db5105bd8360b245cd35810002740ccff605c c54bec26ad23b0121b2ddfbf04bc81050f27e6e1 3f84a6df4546e0f1e62dfbcd0b823ea29499a787
Created attachment 17790 [details] git-am fix for 4.18.next, 4.17.next. Applies cleanly to 4.18.next, 4.17.next.
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 ?
Reassigning to Jule for inclusion in 4.17 and 4.18.
Pushed to autobuild-v4-{18,17}-test.
This bug was referenced in samba v4-17-test: 1caac94128e66884c7a844ddb3c6ef1f02d20bdc 460bc1897a3031728a505e660155f55a0762e5c8 ec6a057e6908408c7d64f6da7e5b11503d14a144
This bug was referenced in samba v4-18-test: f2c9d59e5a3c66156a20d297c8660ab2609bedb5 3fb8f2c579cf13fd7d0367ace97d8d2ff5d2c5ac 800f4f9cc9dba727cdca44b3f799cfa83f5f0854
Closing out bug report. Thanks!
This bug was referenced in samba v4-18-stable (Release samba-4.18.0): f2c9d59e5a3c66156a20d297c8660ab2609bedb5 3fb8f2c579cf13fd7d0367ace97d8d2ff5d2c5ac 800f4f9cc9dba727cdca44b3f799cfa83f5f0854
This bug was referenced in samba v4-17-stable (Release samba-4.17.6): 1caac94128e66884c7a844ddb3c6ef1f02d20bdc 460bc1897a3031728a505e660155f55a0762e5c8 ec6a057e6908408c7d64f6da7e5b11503d14a144