Bug 277878

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 Flags
v0
none
v1
none
Patch to fix pathref handling in Samba 4.19
none
Patch to fix handling of pathref fsp's in vfs_zfsacl.c (only) i Samba 4.19
none
Patch to fix handling of pathref fsp's in vfs_zfsacl.c (only) i Samba 4.20+ none

Description Yoshiaki Kasahara 2024-03-22 03:34:41 UTC
I tried to update my Samba server on my FreeBSD 14.0-STABLE server from net/samba416 to net/samba419 (from samba416-4.16.11_4 to samba419-4.19.5_1) with pkg.
After the update, some operations from my Windows 11 client started to fail.
I can create a file but cannot change the filename or modify the file contents.

The "log.hostname" log file reports a lot of similar errors concatenated without line breaks like this (while testing with "tmp/test.txt" on the share):

  facl(ACE_GETACLCNT, .): Function not implemented facl(ACE_GETACLCNT, .): Function not implemented facl(ACE_GETACLCNT, .): Function not implemented facl(ACE_GETACLCNT, .): Function not implemented facl(ACE_GETACLCNT, .): Function not implemented facl(ACE_GETACLCNT, tmp): Function not implemented facl(ACE_GETACLCNT, tmp): Function not implemented facl(ACE_GETACLCNT, .): Function not implemented facl(ACE_GETACLCNT, .): Function not implemented facl(ACE_GETACLCNT, .): Function not implemented facl(ACE_GETACLCNT, tmp): Function not implemented facl(ACE_GETACLCNT, tmp): Function not implemented facl(ACE_GETACLCNT, tmp): Function not implemented facl(ACE_GETACLCNT, tmp): Function not implemented facl(ACE_GETACLCNT, tmp): Function not implemented facl(ACE_GETACLCNT, tmp): Function not implemented facl(ACE_GETACLCNT, tmp/test.txt): Function not implemented facl(ACE_GETACLCNT, .): Function not implemented facl(ACE_GETACLCNT, tmp/test.txt): Function not implemented facl(ACE_GETACLCNT, tmp): Function not implemented facl(ACE_GETACLCNT, .): Function not implemented facl(ACE_GETACLCNT, tmp/test.txt): Function not implemented facl(ACE_GETACLCNT, tmp/test.txt): Function not implemented facl(ACE_GETACLCNT, tmp/test.txt): Function not implemented facl(ACE_GETACLCNT, tmp/test.txt): Function not implemented facl(ACE_GETACLCNT, tmp/test.txt): Function not implemented facl(ACE_GETACLCNT, tmp/test.txt): Function not implemented facl(ACE_GETACLCNT, tmp/test.txt): Function not implemented facl(ACE_GETACLCNT, tmp/test.txt): Function not implemented facl(ACE_GETACLCNT, tmp/test.txt): Function not implemented open_file: Could not set fd to blocking: Bad file descriptor

I'm using vfs_zfsacl with a ZFS dataset for the share.
An essential part of my smb4.conf is as follows (some network configuration with IP addresses is omitted):

------------
[global]
        log file = /var/log/samba4/log.%m
        log level = 1

        security = user
        local master = no
        invalid users = root    
        dns proxy = no
        mdns name = mdns

        min protocol = SMB3
        smb encrypt = required
        unix extensions = no

[homes]
        strict allocate = no

        path = /usr/home/%S/samba
        writable = yes
        valid users = %S
        browseable = no
        guest ok = no
        read only = no
        follow symlinks = yes
        wide links = yes

        ea support = yes

        vfs objects = zfsacl streams_xattr
        nfs4:mode = simple
        nfs4:acedup = merge

        streams_xattr:prefix = user.
        streams_xattr:store_stream_type = no

        map archive = no
        dos filemode = yes
        map acl inherit = yes
------------

I use the same smb4.conf for samba416 and have no such problems. vfs_zfsacl.c in net/samba419 is patched by files/0100-Fix-pathref-handling-for-FreeBSD-13plus.patch, which does not exist in net/samba416.
I believe the patched part emitted these error messages. 

% uname -a
FreeBSD elvenbow.nc.kyushu-u.ac.jp 14.0-STABLE FreeBSD 14.0-STABLE #4 stable/14-n266915-8d22744f5be1: Thu Feb 29 14:25:53 JST 2024     root@elvenbow.nc.kyushu-u.ac.jp:/usr/obj/usr/src/amd64.amd64/sys/ELVENBOW amd64
Comment 1 Mikael Urankar freebsd_committer freebsd_triage 2024-03-22 08:42:58 UTC
I've tried your smb4.conf and I have no problem with a win10 client.
Comment 2 Yoshiaki Kasahara 2024-03-22 09:57:21 UTC
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...
Comment 3 Vladimir Druzenko freebsd_committer freebsd_triage 2024-03-22 22:04:18 UTC
What if you remove "zfsacl" from "vfs objects"?
Comment 4 Yoshiaki Kasahara 2024-03-26 08:04:57 UTC
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.
Comment 5 Yoshiaki Kasahara 2024-04-04 04:05:20 UTC
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.
Comment 6 Yoshiaki Kasahara 2024-04-04 08:03:43 UTC
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".
Comment 7 Mikael Urankar freebsd_committer freebsd_triage 2024-04-04 11:29:58 UTC
Created attachment 249703 [details]
v0

Can you test the attached patch, it should restore the 4.16 behavior
Comment 8 Yoshiaki Kasahara 2024-04-05 02:11:24 UTC
With the patch, samba419 seems to work properly.
There is no facl() error logged now.
Comment 9 Mikael Urankar freebsd_committer freebsd_triage 2024-04-09 12:38:09 UTC
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.
Comment 10 Yoshiaki Kasahara 2024-04-10 05:19:07 UTC
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
Comment 11 Duncan 2024-05-22 04:50:32 UTC
*** Bug 278159 has been marked as a duplicate of this bug. ***
Comment 12 Antti Rasinen 2024-11-27 13:19:44 UTC
(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.
Comment 13 Antti Rasinen 2024-11-27 19:20:53 UTC
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.
Comment 14 Michael Osipov freebsd_committer freebsd_triage 2025-01-03 19:06:42 UTC
(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.
Comment 16 Michael Osipov freebsd_committer freebsd_triage 2025-01-03 19:41:15 UTC
Peter Eriksson, since you are the author of the actual patch, can you have a look at my analysis?
Comment 17 Michael Osipov freebsd_committer freebsd_triage 2025-01-03 19:59:18 UTC
(In reply to Antti Rasinen from comment #13)

As far as I can see we need to use lp_pid_directory() for that.
Comment 18 Michael Osipov freebsd_committer freebsd_triage 2025-01-03 20:22:02 UTC
Open review: https://reviews.freebsd.org/D48313
Comment 19 Konstantin Belousov freebsd_committer freebsd_triage 2025-01-04 03:51:11 UTC
(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?
Comment 20 Peter Eriksson 2025-01-07 09:17:34 UTC
(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.
Comment 21 Peter Eriksson 2025-01-07 09:18:21 UTC
Created attachment 256497 [details]
Patch to fix pathref handling in Samba 4.19
Comment 22 Michael Osipov freebsd_committer freebsd_triage 2025-01-07 17:38:42 UTC
(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.
Comment 23 Peter Eriksson 2025-01-07 19:00:22 UTC
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
Comment 24 Peter Eriksson 2025-01-07 19:01:12 UTC
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
Comment 25 commit-hook freebsd_committer freebsd_triage 2025-01-08 13:49:16 UTC
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(-)
Comment 26 commit-hook freebsd_committer freebsd_triage 2025-01-08 13:50:20 UTC
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(-)
Comment 27 Vladimir Druzenko freebsd_committer freebsd_triage 2025-01-09 10:13:33 UTC
(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.
Comment 28 Michael Osipov freebsd_committer freebsd_triage 2025-01-09 10:17:50 UTC
(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;
>  +  }
Comment 29 Vladimir Druzenko freebsd_committer freebsd_triage 2025-01-09 10:43:48 UTC
(In reply to Michael Osipov from comment #28)
Both patches?
Comment 30 Michael Osipov freebsd_committer freebsd_triage 2025-01-09 10:45:41 UTC
(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.
Comment 31 Vladimir Druzenko freebsd_committer freebsd_triage 2025-01-09 10:53:15 UTC
(In reply to Michael Osipov from comment #30)
Ok, build in progress.
Comment 32 Vladimir Druzenko freebsd_committer freebsd_triage 2025-01-09 11:03:43 UTC
(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?
Comment 33 Michael Osipov freebsd_committer freebsd_triage 2025-01-09 11:04:49 UTC
(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.
Comment 34 Michael Osipov freebsd_committer freebsd_triage 2025-01-09 16:32:18 UTC
(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!
Comment 35 Konstantin Belousov freebsd_committer freebsd_triage 2025-01-09 16:48:30 UTC
(In reply to Michael Osipov from comment #34)
cc0d806f63e833b9e011c0665905b2208b436c8b
Comment 36 Michael Osipov freebsd_committer freebsd_triage 2025-01-09 16:57:23 UTC
(In reply to Konstantin Belousov from comment #35)

Oh wow, insane what a coincidence! Thank you!
Comment 37 Michael Osipov freebsd_committer freebsd_triage 2025-01-09 17:50:47 UTC
(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.
Comment 38 Michael Osipov freebsd_committer freebsd_triage 2025-01-09 18:02:36 UTC
Review: https://reviews.freebsd.org/D48399
Comment 39 Michael Osipov freebsd_committer freebsd_triage 2025-01-10 14:17:27 UTC
(In reply to Antti Rasinen from comment #13)

Here is the fix: https://reviews.freebsd.org/D48416
Comment 40 Vladimir Druzenko freebsd_committer freebsd_triage 2025-01-11 13:07:29 UTC
(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):
Comment 41 eborisch+FreeBSD 2025-01-12 19:55:39 UTC
(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
Comment 42 commit-hook freebsd_committer freebsd_triage 2025-01-12 20:55:14 UTC
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(-)
Comment 43 commit-hook freebsd_committer freebsd_triage 2025-01-12 20:58:17 UTC
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(-)
Comment 44 Michael Osipov freebsd_committer freebsd_triage 2025-01-12 21:19:02 UTC
(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.
Comment 45 Michael Osipov freebsd_committer freebsd_triage 2025-01-12 21:24:28 UTC
(In reply to eborisch+FreeBSD from comment #41)

Does this also cover: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=281360
Comment 46 Michael Osipov freebsd_committer freebsd_triage 2025-01-12 21:25:18 UTC
Last two reviews applied. If no one objects I will close this issue tomorrow.