This seems to happen not just for MKDIR, but almost all operations working on files. "DIR" and "CD" works fine.

Ah, I think I found it. It's the O_PATH support that is new in FreeBSD 13 that doesn't play well with the ACL functions - acl_get_fd_np() doesn't work for an O_PATH opened file. Test code that fails:

include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
#include <fcntl.h>
#include <sys/types.h>
#include <sys/acl.h>
#include <sys/stat.h>


int
main(int argc,
     char *argv[]) {
  int i, j, fd;
  acl_t a;
  char *path = ".";
  int flags = O_PATH;
  struct stat sb;
  
  
  for (i = 1; i < argc && argv[i][0] == '-'; i++) {
    for (j = 1; argv[i][j]; j++) {
      switch (argv[i][j]) {
      case 'n':
	flags = 0;
	break;
      }
    }
  }
  
  if (i < argc)
    path = argv[i];
  
  fd = open(path, O_RDONLY|flags);
  if (fd < 0) {
    fprintf(stderr, "%s: Error: %s: open: %s\n", argv[0], path, strerror(errno));
    exit(1);
  }

  if (fstat(fd, &sb) < 0) {
    fprintf(stderr, "%s: Error: %s: fstat: %s\n", argv[0], path, strerror(errno));
  }

  a = acl_get_fd_np(fd, ACL_TYPE_NFS4);
  if (!a) {
    fprintf(stderr, "%s: Error: %s: acl_get_fd_np: %s\n", argv[0], path, strerror(errno));
  }

  return 0;
}

Lpeter86@runur00:~ % ./t
./t: Error: .: acl_get_fd_np: Bad file descriptor
Lpeter86@runur00:~ % ./t -n

A workaround:

1. Edit /usr/include/fcntl.h and rename O_PATH to DISABLED_O_PATH
2. Build Samba
3. Undo the change to fcntl.h ...

Created attachment 17900 [details]
Patch the only enables Samba O_PATH support on Linux systems

Attachment contains a patch that only enables the O_PATH support for Linux systems.

Works but is kind of a big hammer...

Here is the code inside posixacl_sys_acl_get_fd() that does this for POSIX ACLs.

        if (!fsp->fsp_flags.is_pathref && (acl_type == ACL_TYPE_ACCESS)) {
                /* POSIX API only allows ACL_TYPE_ACCESS fetched on fd. */
                acl = acl_get_fd(fsp_get_io_fd(fsp));
        } else if (fsp->fsp_flags.have_proc_fds) {
                int fd = fsp_get_pathref_fd(fsp);
                const char *proc_fd_path = NULL;
                char buf[PATH_MAX];

                proc_fd_path = sys_proc_fd_path(fd, buf, sizeof(buf));
                if (proc_fd_path == NULL) {
                        return NULL;
                }

                acl = acl_get_file(proc_fd_path, acl_type);

fsp->fsp_flags.is_pathref = true

for fd's open with O_PATH.

I think zfsacl may need something similar for FreeBSD.

These are the current places where the
horrid hack is used:

git grep sys_proc_fd_path
source3/include/proto.h:const char *sys_proc_fd_path(int fd, char *buf, size_t bufsize);
source3/lib/system.c:const char *sys_proc_fd_path(int fd, char *buf, size_t bufsize)
source3/modules/vfs_btrfs.c:    p = sys_proc_fd_path(fsp_fd, buf, sizeof(buf));
source3/modules/vfs_default.c:          p = sys_proc_fd_path(fd, buf, sizeof(buf));
source3/modules/vfs_default.c:          p = sys_proc_fd_path(fd, buf, sizeof(buf));
source3/modules/vfs_default.c:          p = sys_proc_fd_path(fd, buf, sizeof(buf));
source3/modules/vfs_default.c:          p = sys_proc_fd_path(fd, buf, sizeof(buf));
source3/modules/vfs_default.c:          p = sys_proc_fd_path(fd, buf, sizeof(buf));
source3/modules/vfs_default.c:          p = sys_proc_fd_path(fd, buf, sizeof(buf));
source3/modules/vfs_default.c:          p = sys_proc_fd_path(fd, buf, sizeof(buf));
source3/modules/vfs_default.c:          p = sys_proc_fd_path(fd, buf, sizeof(buf));
source3/modules/vfs_gpfs.c:                     p = sys_proc_fd_path(fd, buf, sizeof(buf));
source3/modules/vfs_gpfs.c:             p = sys_proc_fd_path(fd, buf, sizeof(buf));
source3/modules/vfs_gpfs.c:             p = sys_proc_fd_path(fd, buf, sizeof(buf));
source3/modules/vfs_gpfs.c:             p = sys_proc_fd_path(fd, buf, sizeof(buf));
source3/modules/vfs_posixacl.c:         proc_fd_path = sys_proc_fd_path(fd, buf, sizeof(buf));
source3/modules/vfs_posixacl.c:         proc_fd_path = sys_proc_fd_path(fd, buf, sizeof(buf));
source3/modules/vfs_posixacl.c:         proc_fd_path = sys_proc_fd_path(fd, buf, sizeof(buf));
source3/smbd/open.c:    p = sys_proc_fd_path(old_fd, buf, sizeof(buf));

So these are all the places you're going to run into the
same problem with O_PATH opened fd's.

Created attachment 17901 [details]
Patch that implements openat() O_PATH fix for FreeBSD instead of the proc_fds hack

Here is a first cut at a patch that uses openat() to get a usable fd handle for systems that support it (FreeBSD) in order to avoid the fallback to using stored paths in smbd/open.c, modules/vfs_default & modules/vfs_zfsacl.

Comment on attachment 17901 [details]
Patch that implements openat() O_PATH fix for FreeBSD instead of the proc_fds hack

It works, but I hate the #ifdef wrapping :-). How about the:

#if defined(HAVE_OPENAT) && defined(O_EMPTY_PATH)
	new_fd = openat(old_fd, "", O_EMPTY_PATH|flags);
...
#endif

construct is wrapped into an inline function, something like:

------------------------------------------------------
inline int open_metadata_fd_from_pathref(int md_fd)
{
#if defined(HAVE_OPENAT) && defined(O_EMPTY_PATH)
        return openat(md_fd, "", O_EMPTY_PATH|flags);
#else
        return -1;
}
-----------------------------------------------------

That should allow open_metadata_fd_from_pathref() to
always be called, with the logic from a success return
always included, with the fsp->fsp_flags.have_proc_fds
as the first fallback.

Yeah I missed the 'int flags' arg, but I'm sure you get what I mean.

Or maybe not return -1 from the open_metadata_fd_from_pathref() call, as I guess that could be a valid error return if the openat(.. O_EMPTY_PATH) fails. So maybe have it look like:

inline int open_metadata_fd_from_pathref(int md_fd, int flags. int *_new_fd)
{
#if defined(HAVE_OPENAT) && defined(O_EMPTY_PATH)
        int new_fd
        new_fd = openat(old_fd, "", O_EMPTY_PATH|flags);
        if (new_fd == -1) {
                return errno;
        }
        *_new_fd = new_fd;
        return 0;
#else
        return ENOSYS;
#endif
}

Created attachment 17913 [details]
Patch that fixes pathref metadata access for FreeBSD

I've uploaded a new version of the Patch for FreeBSD 13+ that seems to work for Samba 4.18 (and compiles for 4.17 - I haven't tested it yet there) where I've put the #ifdefs and stuff inside a helper function in lib/system.c similar to the proc_fs stuff for Linux.

Re-assigning to Andrew for review. I think the conjunction of FreeBSD and ZFS fits squarely in his wheelhouse :-).

Sorry, just saw this. You won't be able to set ACL on an FD that's opened O_RDONLY. 

I think that rather than altering vfs_default.c, user should provide procfs equivalent via fdescfs mount (this is something KIB added to FreeBSD). 

I'll rebase my old MR for this on latest master. Target for maybe mid next week.

I use O_EMPTY_PATH fairly extensively in our internal FreeBSD samba branch. It's a tradeoff, by using O_EMPTY_PATH you can avoid an extra pass through VFS_LOOKUP in kernel for metadata ops (which can be a nice benefit in cases where name cache isn't being used -- like case insensitive ZFS datasets), but it also means that WRITE_DATA or READ_DATA is required for setacl and getacl respective (which isn't 100% correct for these ops).

I'll try to get my upstream freebsd phabricator request for openat2 RESOLVE_NO_SYMLINKS done as well by then (so that we can use Volker's work on dir listing).

(In reply to Andrew Walker from comment #13)

> Sorry, just saw this. You won't be able to set ACL on an FD that's opened O_RDONLY. 

Are you sure about that? I would have guessed so too, but my testing on FreeBSD 13.2 indicates otherwise. I can update ACLs on FD's open as O_RDONLY.



> I think that rather than altering vfs_default.c, user should provide procfs
> equivalent via fdescfs mount (this is something KIB added to FreeBSD). 

Yes, probably. However there is at least one bug with fdescfs that looks like it needs a stat() workaround for things to work as they should (or vfs_default will fail to open any directories).


From my testing it looks like after the fdescfs filesystem just has been mounted then the first attempt to access a FD opened via fdescfs misbehaves:

1. stat() works like lstat() - but only for the first access, If I stat() again then I get the information about the symlink target (correctly)...

2. acl fails with errno=78 (Function not implemented)

3. extattr_list_file fails with errno=45 (Operation not supported)

4. openat with O_DIRECTORY fails with errno=20 (Not a directory)

5. openat without O_DIRECTORY fails with errno=13 (Permission denied)

But if I just add a little stat() call of the fdescfs fd path before doing these operations then things work just fine... 

I'll upload my test program too.

Created attachment 17935 [details]
freebsd test program to verify fdescfs/O_PATH/acl/xattrs functionality

It sounds like you didn't mount the fsdescfs filesystem with the `nodup` option. c.f. man 5 fdescfs.

(In reply to Andrew Walker from comment #16)

nodup or no nodup doesn't seem to make any difference when I test it...?

I can still change the ACL without any problems...

Created attachment 18181 [details]
Patch to fix pathref handling for FreeBSD 13+

Updated version of the patch I use to make Samba usable on FreeBSD 13+ updated. Basically uses openat() and O_EMPTY_PATH to convert a pathref fd to a real one for the open() operation, and also for for various ACL and XATTR operations (and some others).

I can confirm that testing Peter's patch "Patch to fix pathref handling for FreeBSD 13+" on FreeBSD 14.0-RELEASE works and will propose that to be added to the FreeBSD samba 4.19.x tracking bug here: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=270383.

I have tested this within a jail and it works as well along with the zfsacl option. Please let me know if there's any other testing I can help with to push this fix in.

Created attachment 18524 [details]
Patch for vfs_zfsacl för Samba 4.19 on FreeBSD 13+

This is a simpler version of the patch that skips the openat()/O_EMPTY_PATH code and just does the same fallback as the code in vfs_default does (falls back to using the path stored in base_name in FSP). I'll also upload a version that works for Samba 4.20 and later.

Created attachment 18525 [details]
Patch for vfs_zfsacl för Samba 4.20+ on FreeBSD 13+

A version of the smaller patch for Samba 4.20/4.21

(In reply to Peter Eriksson from comment #20)

FTR: FreeBSD port maintainer here, I have updated the FreeBSD port of Samba 4.19 with Peter's simple patch and it works. It continues to use O_PATH with fdescfs.

Commit: https://github.com/freebsd/freebsd-ports/commit/24fbb52094816ac737d04a668f32dbaf471b41e8

@Andrew: can you take a look? As it is this is a mess of code changes, if I had a patchset with more isolated changes, ideally a WIP MR on gitlab, I'd be more then happy to drive this home.