Summary: | net/samba419: Function not implemented facl(ACE_GETACLCNT, filename) with vfs_zfsacl | ||
---|---|---|---|
Product: | Ports & Packages | Reporter: | Yoshiaki Kasahara <ykasap> |
Component: | Individual Port(s) | Assignee: | FreeBSD Samba Team <samba> |
Status: | In Progress --- | ||
Severity: | Affects Only Me | CC: | 0mp, crest, dpy, eborisch+FreeBSD, freebsd-48b9, freebsd, kib, michaelo, pen, site-bugs.freebsd.org, vvd |
Priority: | --- | Flags: | bugzilla:
maintainer-feedback?
(mikael) |
Version: | Latest | ||
Hardware: | amd64 | ||
OS: | Any | ||
Attachments: |
Description
Yoshiaki Kasahara
2024-03-22 03:34:41 UTC
I've tried your smb4.conf and I have no problem with a win10 client. Thank you for checking. I may be doing something wrong with my setup, but I'm unsure where to look. facl() in libsunacl.a hides the actual errno produced by acl_get_fd_np(), so maybe I'll try to patch that to see what is happening more closely... What if you remove "zfsacl" from "vfs objects"? Even after removing "zfsacl" from "vfs objects," I couldn't modify the file on the share, so I guess the problem is not in zfsacl but somewhere else. The same configuration worked with samba416. I'm more confused. Anyway, I think this PR should be closed. Sorry for the noise. I'm still trying to find the reason why the same smb4.conf works with samba416 and not with samba419. I emptied "vfs objects," but still, I cannot modify files. I noticed that when I try to overwrite a text file with notepad, "The handle is invalid" error occurred. The log with debug level=10 shows: [2024/04/04 11:43:22.915701, 1, pid=80375, effective(1001, 1001), real(0, 0)] ../../source3/smbd/open.c:1508(open_file) open_file: Could not set fd to blocking: Bad file descriptor [2024/04/04 11:43:22.915720, 10, pid=80375, effective(1001, 1001), real(0, 0), class=locking] ../../source3/locking/posix.c:482(delete_lock_ref_count) delete_lock_ref_count for file tmp/test2/a.txt [2024/04/04 11:43:22.915741, 10, pid=80375, effective(1001, 1001), real(0, 0)] ../../source3/smbd/open.c:6465(create_file_unixpath) create_file_unixpath: NT_STATUS_INVALID_HANDLE I don't understand why open_file() fails like this. There is something wrong with file descriptor handling. Then, I noticed that "files/0100-Fix-pathref-handling-for-FreeBSD-13plus.patch" modified proc_fd_patterns in source3/lib/system.c: --- source3/lib/system.c 2023-01-18 16:32:24.174553200 +0100 +++ source3/lib/system.c 2023-06-19 23:35:30.132465000 +0200 @@ -1022,6 +1022,8 @@ } proc_fd_patterns[] = { /* Linux */ { "/proc/self/fd/%d", "/proc/self/fd/0" }, + /* FreeBSD */ + { "/compat/linux/dev/fd/%d", "/compat/linux/dev/fd/0" }, { NULL, NULL }, }; Linux emulation is enabled on my server, so I have /compat/linux/dev/fd mounted. I unmounted /compat/linux/dev/fd and /compat/linux/dev (because /compat/linux/dev contains another "fd" directory), and samba419 began to work properly. Could you please review this patch more closely? I don't like this patch because it silently introduces (broken) dependency with the Linux emulator. After reading https://bugzilla.samba.org/show_bug.cgi?id=15376, mounting fdescfs seems expected, but it is not working well on my system. I tried the attached test program in the bug report, and the result was erratic, as mentioned in the comment. Sometimes, acl and extattr were not working properly, consistent with my issues with samba419. After I added -s option to stat() the fdescfd path, the test seemed to work. Then, I found bug #272127. The test still failed just after mounting fdescfs on my system. I tried the rdlnk option and acl and extattr errors disappeared, but open_file still failed with "Bad file descriptor". Created attachment 249703 [details]
v0
Can you test the attached patch, it should restore the 4.16 behavior
With the patch, samba419 seems to work properly. There is no facl() error logged now. Created attachment 249849 [details]
v1
The previous patch broke acls on zfs.
Can you give this one a shot? It also update samba to 4.19.6
Thanks.
With the new patch, I can rename a file and change DOS attributes (no facl or extattr error), but I cannot overwrite an existing file. The error message is the same. [2024/04/10 14:07:03.246827, 1] ../../source3/smbd/open.c:1497(open_file) open_file: Could not set fd to blocking: Bad file descriptor *** Bug 278159 has been marked as a duplicate of this bug. *** (In reply to Yoshiaki Kasahara from comment #5) I've been struggling with the same error. I'll try disabling Linux as well today and see if it fixes the situation for me as well. Mikael's patch worked for me, with no problems whatsoever anymore. Of course, it'd be good if the path wasn't hardcoded but instead followed the same logic as the `samba_server` rc script. (In reply to Yoshiaki Kasahara from comment #10) This I can explain, but need also explain a greater problem: The patch is good for those code spots which need to interact with fdescfs, but does not solve the actual problem. Either Michael or me have to go on and apply this patch. I have put some effor today to undstand what is happening here given that the patch is applied: * Client tries to write to a file * Sample issues the following in the logs: [2025/01/03 17:58:11.788935, 10, pid=24454, effective(722, 121), real(0, 0)] ../../source3/smbd/open.c:5939(create_file_unixpath){"create_file_unixpath: access_mask = 0x120196 file_attributes = 0x0 share_access = 0x3 create_disposition = 0x3 create_options = 0x400040 oplock_request = 0x100 private_flags = 0x0 ea_list = 0x0, sd = 0x0, fname = test/log/test.txt [2025/01/03 17:58:11.791894, 4, pid=24454, effective(722, 121), real(0, 0)] ../../source3/smbd/open.c:4136(open_file_ntcreate)\n",128},{" calling open_file with flags=0x2 flags2=0x204 mode=0764, access_mask = 0x120196, open_access_mask = 0x120196 Memorize create_disposition = 0x3 (FILE_OPEN_IF), flags2=0x204 (from create_disposition contains O_CREAT (0x200). * Samba checks for access rights and now is ready to open and serve the file. Note that the new VFS is using O_PATH/O_EMPTY_PATH (https://wiki.samba.org/index.php/The_New_VFS) to efficiently access metadata, etc. * Performs these now: openat(31,"test.txt",O_RDONLY|O_NOFOLLOW|O_PATH,00) = 25 (0x19) openat(25,"",O_RDWR|O_NONBLOCK|O_CREAT|0x2000000,01006) ERR#22 'Invalid argument' 0x2000000 is O_EMPTY_PATH Reason for failure: 6de3cf14c47d97b423ae25d5bd1d80b896ecd9e6. Might be totally legit. For me, O_CREAT does not sense here because O_PATH only operates on existing file/dir entries. * It falls back to fdescfs: openat(AT_FDCWD,"/var/run/samba4/fd/25",O_RDONLY|O_PATH,00) = 32 (0x20). No idea why this is necessary fcntl(32,F_GETFL,) = 12582912 (0xc00000) fcntl(32,F_SETFL,O_RDONLY|O_PATH|0x800000) ERR#9 'Bad file descriptor' This is your "open_file: Could not set fd to blocking: Bad file descriptor". Why does it fail? Well, look at The_New_VFS: > The file itself is not opened, and other file operations (e.g., read(2), write(2), fchmod(2), fchown(2), fgetxattr(2), ioctl(2), mmap(2)) fail with the error EBADF. One can fantastically reproduce these with Python which calls all low level APIs for us: > $ cat empty-path.py.bak > import os > fd = os.open("/tmp/test.txt", os.O_RDONLY|os.O_NOFOLLOW|os.O_PATH) > print(fd) > fd = os.open("", os.O_RDWR|os.O_NONBLOCK|os.O_CREAT|0x2000000, dir_fd=fd) > print(fd) 3 Traceback (most recent call last): File "/net/home/osipovmi/empty-path.py.bak", line 4, in <module> fd = os.open("", os.O_RDWR|os.O_NONBLOCK|os.O_CREAT|0x2000000, dir_fd=fd) OSError: [Errno 22] Invalid argument: '' > $ cat empty-path.py.bak > import os > fd = os.open("/tmp/test.txt", os.O_RDONLY|os.O_NOFOLLOW|os.O_PATH) > print(fd) > fd = os.open("", os.O_RDWR|os.O_NONBLOCK|0x2000000, dir_fd=fd) > print(fd) 3 4 and the fdescfs issue: > $ cat empty-path.py > import os > import fcntl > > fd = os.open("/tmp/test.txt", os.O_RDONLY|os.O_NOFOLLOW|os.O_PATH) > print(fd) > flags = fcntl.fcntl(fd, fcntl.F_GETFL) > print(flags) > flags &= ~os.O_NONBLOCK > fcntl = fcntl.fcntl(fd, fcntl.F_SETFL, flags) 3 12582912 Traceback (most recent call last): File "/net/home/osipovmi/empty-path.py", line 12, in <module> fcntl = fcntl.fcntl(fd, fcntl.F_SETFL, flags) OSError: [Errno 9] Bad file descriptor Remove O_PATH: 3 0 Note: I don't use ZFS and I don't consider it to be a FS problem and relying on the Linux compat layer does not make sense for natively complied software. Unfortunately, I have no clue how to proceed here, it is simply usable for automated clients. FTR: fcntl is invoked by https://github.com/samba-team/samba/blob/204b0f2d2f7bcdfac41b96f499920c3980088665/source3/smbd/open.c#L1488C10-L1488C38 Peter Eriksson, since you are the author of the actual patch, can you have a look at my analysis? (In reply to Antti Rasinen from comment #13) As far as I can see we need to use lp_pid_directory() for that. Open review: https://reviews.freebsd.org/D48313 (In reply to Michael Osipov from comment #14) I do not quite follow the discussion. Are there any problems with the open(2) API? For instance, O_EMPTY_PATH only makes sense for openat(2), and kernel rightfully reports EINVAL for open( ..., O_EMPTY_PATH). OTOH, it is not hard to make openat(..., O_PATH | O_CREAT) to work since it has a clear semantic. Is the later (O_CREAT | O_PATH) needed for samba? (In reply to Michael Osipov from comment #16) I've tried various ways of getting ZFS ACLs handling correct in Samba on FreeBSD and I'm currently using a different solution that uses O_PATH and stuff - but that is a bigger patch. It doesn't work (as is) for Samba 4.20 and later though. I haven't had time to rework it for those versions though... I'll upload that patch here too. Created attachment 256497 [details]
Patch to fix pathref handling in Samba 4.19
(In reply to Konstantin Belousov from comment #19) Let me investigate it a bit futher tomorrow and give you an answer. I have meanwhile modified Peter's patch for the bad file descriptor and my usecase works now. Though, I can not sure wether this patch is logically correct even if it solves the problem. Created attachment 256506 [details]
Patch to fix handling of pathref fsp's in vfs_zfsacl.c (only) i Samba 4.19
Here is a more simple patch that just fixes the handling of pathref fsp's in vfs_zfsacl - it does it the same way the "fall-thru" case is done in vfs_default.c for other filesystem operations.
Ie:
If the "FSP" contains a normal "io" fd, use that with facl().
If the "FSP" contains a pathref fd, and there is a procfs, use that to translate the fd into a path that can be used with acl().
Else use the stored base_name path in the FSP with acl().
The "bigger" patch also attempts to use openat() with O_EMPTY_PATH to convert a pathref fd into a normal IO fd before falling back to the base_name. It also attempts some operations directly on the pathref fd since some can actually succeed without having to convert them into paths or io fd's. And covers more that just just vfs_zfsacl code.
Each variant has it's own "problem" though:
1. The procfs way requires the use of the Linux emulator and having the procfs mounted in a specific way. Whe I tested that previously I also noticed some strange behavior where sometime filesystem metadata would be stale. I haven't verified that behaviour lately though (was on FreeBSD 13.2 I think).
2. The problem with the fallback case is that there is a potential for a race condition.
3. The openat() way is more complex and when modifying files/directories it also requires that we can actually open the file/directory in question - not just operate on the directory entry...
I'll also upload a variant of the smaller patch that works for Samba 4.20 and 4.21
Created attachment 256507 [details]
Patch to fix handling of pathref fsp's in vfs_zfsacl.c (only) i Samba 4.20+
A version of the smaller patch for Samba 4.20 and 4.21
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=39b0576274424e915fc1208c5cb50ca7880964dc commit 39b0576274424e915fc1208c5cb50ca7880964dc Author: Mikael Urankar <mikael@FreeBSD.org> AuthorDate: 2024-04-09 12:35:28 +0000 Commit: Michael Osipov <michaelo@FreeBSD.org> CommitDate: 2025-01-08 13:48:04 +0000 net/samba419: Fix procfd search patterns for FreeBSD PR: 277878 Approved by: jrm (mentor) Tested by: michaelo, Antti Rasinen MFH: 2025Q1 Differential Revision: https://reviews.freebsd.org/D48313 net/samba419/Makefile | 2 +- net/samba419/files/0100-Fix-pathref-handling-for-FreeBSD-13plus.patch | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) A commit in branch 2025Q1 references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=6c175b587b97b281c3eb85fb6647cb73b714a217 commit 6c175b587b97b281c3eb85fb6647cb73b714a217 Author: Mikael Urankar <mikael@FreeBSD.org> AuthorDate: 2024-04-09 12:35:28 +0000 Commit: Michael Osipov <michaelo@FreeBSD.org> CommitDate: 2025-01-08 13:48:46 +0000 net/samba419: Fix procfd search patterns for FreeBSD PR: 277878 Approved by: jrm (mentor) Tested by: michaelo, Antti Rasinen MFH: 2025Q1 Differential Revision: https://reviews.freebsd.org/D48313 (cherry picked from commit 39b0576274424e915fc1208c5cb50ca7880964dc) net/samba419/Makefile | 2 +- net/samba419/files/0100-Fix-pathref-handling-for-FreeBSD-13plus.patch | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (In reply to commit-hook from comment #26) This patch broke file editing, only creation and deletion works: $ smbclient //server/share Password for [WORKGROUP\user]: Try "help" to get a list of possible commands. smb: \> put MOVED putting file MOVED as \MOVED (3249,3 kb/s) (average 3249,3 kb/s) smb: \> put MOVED NT_STATUS_INVALID_HANDLE opening remote file \MOVED smb: \> rm MOVED smb: \> put MOVED putting file MOVED as \MOVED (4019,8 kb/s) (average 4148,1 kb/s) smb: \> put MOVED NT_STATUS_INVALID_HANDLE opening remote file \MOVED smb: \> rm MOVED On server remove patch, rebuild and restart samba. On client: $ smbclient //server/share Password for [WORKGROUP\root]: Try "help" to get a list of possible commands. smb: \> put MOVED putting file MOVED as \MOVED (3644,1 kb/s) (average 3644,1 kb/s) smb: \> put MOVED putting file MOVED as \MOVED (3512,8 kb/s) (average 3577,3 kb/s) smb: \> put MOVED putting file MOVED as \MOVED (4238,3 kb/s) (average 4238,3 kb/s) smb: \> rm MOVED Wendoze clients can't edit files too - create and delete only. Same share with and without "vfs objects = zfsacl", public shares and shares with password. (In reply to Vladimir Druzenko from comment #27) It is not the patch, the reason for that is explained here: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=277878#c14 I need to go through some stuff to understand the case better, but you can apply this intermediate patch and let me know wether it works for you: > diff --git a/net/samba419/files/0100-Fix-pathref-handling-for-FreeBSD-13plus.patch b/net/samba419/files/0100-Fix-pathref-handling-for-FreeBSD-13plus.patch > index b2a51efb7..8fa78037a 100644 > @@ -91,7 +91,7 @@ https://bugzilla.samba.org/show_bug.cgi?id=15376 > +#if defined(HAVE_OPENAT) && defined(O_EMPTY_PATH) > + /* This works for FreeBSD 13+ atleast */ > + > -+ tfd = openat(fd, "", O_EMPTY_PATH|flags); > ++ tfd = openat(fd, "", (O_EMPTY_PATH|flags) & ~O_CREAT); > + if (tfd < 0) { > + return errno; > + } (In reply to Michael Osipov from comment #28) Both patches? (In reply to Vladimir Druzenko from comment #29) The patch from Mikael is already in main and quarterly, my one-off not yet. Please try it and let me know. (In reply to Michael Osipov from comment #30) Ok, build in progress. (In reply to Michael Osipov from comment #28) This patch fixed the issue: smb: \> put MOVED putting file MOVED as \MOVED (2599,5 kb/s) (average 2599,5 kb/s) smb: \> put MOVED putting file MOVED as \MOVED (4104,4 kb/s) (average 3183,0 kb/s) smb: \> put MOVED putting file MOVED as \MOVED (3610,4 kb/s) (average 3313,8 kb/s) smb: \> put MOVED putting file MOVED as \MOVED (4148,1 kb/s) (average 3489,2 kb/s) smb: \> rm MOVED Thanks! Commit it? (In reply to Vladimir Druzenko from comment #32) I first need to process Konstantin's comment, make a few tests to better understand O_PATH/O_EMPTY_PATH and align with Peter. (In reply to Konstantin Belousov from comment #19) I will get back to the behavior of O_EMPTY_PATH, for now I don't see a problem. But I can see that (O_CREAT | O_PATH) is a valid usecase and might appear in Samba, it is not supported in FreeBSD. I tried the same code on RHEL8 and it just works. I will create a PR for have this combination supported. As soon as I find other irregularities I will file separate issues. Thank you! (In reply to Michael Osipov from comment #34) cc0d806f63e833b9e011c0665905b2208b436c8b (In reply to Konstantin Belousov from comment #35) Oh wow, insane what a coincidence! Thank you! (In reply to Peter Eriksson from comment #23) Disclaimer: I have tested vfs_default only because I don't have a reasonable ZFS-based ACL setup so someone with that setup needs to copy/confirm my results on ZFS. The simple patch works correctly with reopening the path-only FD (O_PATH) by using the fdescfs mounted by the samba_server rc script (confirmed with truss and a C# app on Windows). The sophisticated patch using O_EMPTY_PATH does not work reliably because some of the semantics are not properly applied, read: It needs more investagation, testing, fixing. What I'd like to do is to replace the current sophisticated patch with Peter's simple patch since it works reliably in main and quarterly since Samba 4.19 as of now is not properly usable. As for the problems you have mentioned: 1. the procfs way does not require the Linux emulator, it happily uses fdescfs in Samba's PID dir with 'nodup' option. As for the strage behavior: file a PR 2. Here the same: is it us or a Samba issue? In any case a PR. 3. Basically answered by my statement about the sophisticated patch. Since the manpage of fdescfs(5) says that reopening and O_EMPTY_PATH are equivalent I'd stick with fdescfs(5). Review coming right up and waiting for comments on the vfs_zfsacl module. vvd: Please try the review as well as soon as it is there. (In reply to Antti Rasinen from comment #13) Here is the fix: https://reviews.freebsd.org/D48416 (In reply to Michael Osipov from comment #38) Patch from https://reviews.freebsd.org/D48399 work for me. Not related but add support for dns/bind920 in net/samba419/files/0001-Compact-and-simplify-modules-build-and-config-genera.patch: -+for bind_version in (910, 911, 912, 914, 916, 918): ++for bind_version in (910, 911, 912, 914, 916, 918, 920): (In reply to Vladimir Druzenko from comment #40) Likewise; with D48399 Time Machine is working again! Relevant settings for the record: [global] fruit:aapl = yes fruit:model = MacSamba fruit:advertise_fullsync = true fruit:posix_rename = yes fruit:metadata = stream fruit:veto_appledouble = no fruit:nfs_aces = no fruit:wipe_intentionally_left_blank_rfork = yes fruit:delete_empty_adfiles = yes [sharename] vfs objects = catia fruit streams_xattr zfsacl fruit:time machine = yes A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=24fbb52094816ac737d04a668f32dbaf471b41e8 commit 24fbb52094816ac737d04a668f32dbaf471b41e8 Author: Peter Eriksson <pen@lysator.liu.se> AuthorDate: 2025-01-09 17:56:33 +0000 Commit: Michael Osipov <michaelo@FreeBSD.org> CommitDate: 2025-01-12 20:54:04 +0000 net/samba419: Use a simple approach to reconcile O_PATH and vfs_zfsacl The current, sophisticated approach does not fully cover all usecases and flag twiddling around O_EMPTY_PATH, resort to use working fdescfs(5) magic for now. PR: 277878 Tested by: michaelo, vvd, Peter Eriksson Approved by: jrm (mentor), vvd, mikael (samba) MFH: 2025Q1 Differential Revision: https://reviews.freebsd.org/D48399 net/samba419/Makefile | 2 +- ...0-Fix-pathref-handling-for-FreeBSD-13plus.patch | 604 ++++++--------------- 2 files changed, 157 insertions(+), 449 deletions(-) A commit in branch 2025Q1 references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=aa8c5beb655f79c21e6ec7837b4b669919fcb89f commit aa8c5beb655f79c21e6ec7837b4b669919fcb89f Author: Peter Eriksson <pen@lysator.liu.se> AuthorDate: 2025-01-09 17:56:33 +0000 Commit: Michael Osipov <michaelo@FreeBSD.org> CommitDate: 2025-01-12 20:57:10 +0000 net/samba419: Use a simple approach to reconcile O_PATH and vfs_zfsacl The current, sophisticated approach does not fully cover all usecases and flag twiddling around O_EMPTY_PATH, resort to use working fdescfs(5) magic for now. PR: 277878 Tested by: michaelo, vvd, Peter Eriksson Approved by: jrm (mentor), vvd, mikael (samba) MFH: 2025Q1 Differential Revision: https://reviews.freebsd.org/D48399 (cherry picked from commit 24fbb52094816ac737d04a668f32dbaf471b41e8) net/samba419/Makefile | 2 +- ...0-Fix-pathref-handling-for-FreeBSD-13plus.patch | 604 ++++++--------------- 2 files changed, 157 insertions(+), 449 deletions(-) (In reply to Vladimir Druzenko from comment #40) Not rather: > $ git diff -U0 > diff --git a/net/samba419/files/0001-Compact-and-simplify-modules-build-and-config-genera.patch b/net/samba419/files/0001-Compact-and-simplify-modules-build-and-config-genera.patch > index b4bc56519f7e..d34395bf6f22 100644 > --- a/net/samba419/files/0001-Compact-and-simplify-modules-build-and-config-genera.patch > +++ b/net/samba419/files/0001-Compact-and-simplify-modules-build-and-config-genera.patch > @@ -171 +171 @@ index ab0a241b937..3743753504c 100644 > -+for bind_version in (910, 911, 912, 914, 916, 918): > ++for bind_version in (910, 911, 912, 914, 916, 918, 920): > @@ -239 +239 @@ index ab0a241b937..3743753504c 100644 > -+ cflags='-DBIND_VERSION=918', > ++ cflags='-DBIND_VERSION=920', > @@ -286 +286 @@ index 0b40e03e370..bf7415ff88a 100644 > -+ cflags='-DBIND_VERSION=918', > ++ cflags='-DBIND_VERSION=920', I cannot reasonably test this. Don't know how with a simple setup. (In reply to eborisch+FreeBSD from comment #41) Does this also cover: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=281360 Last two reviews applied. If no one objects I will close this issue tomorrow. |