Bug 15376 - vfs objects "zfsacl" causing NT_STATUS_INVALID_HANDLE on FreeBSD 13.2
Summary: vfs objects "zfsacl" causing NT_STATUS_INVALID_HANDLE on FreeBSD 13.2
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.18.2
Hardware: All FreeBSD
: P5 normal (vote)
Target Milestone: ---
Assignee: Samba QA Contact
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-05-25 10:20 UTC by Peter Eriksson
Modified: 2023-05-31 17:33 UTC (History)
0 users

See Also:


Attachments
Patch the only enables Samba O_PATH support on Linux systems (932 bytes, patch)
2023-05-29 14:51 UTC, Peter Eriksson
no flags Details
Patch that implements openat() O_PATH fix for FreeBSD instead of the proc_fds hack (10.32 KB, patch)
2023-05-31 05:51 UTC, Peter Eriksson
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Eriksson 2023-05-25 10:20:49 UTC
If using "vfs object" with "zfsacl" on FreeBSD 13.2 I get:

smb: \> mkdir qqq
NT_STATUS_INVALID_HANDLE making remote directory \qqq


If I up the "log level" to 10 then the same operation gives in the log file:

[2023/05/25 12:12:49.580439,  5, pid=82150, effective(1003258, 100001000), real(0, 0)] ../../source3/smbd/dosmode.c:181(unix_mode)
  unix_mode: unix_mode(qqq) returning 040700
  facl(ACE_GETACLCNT, .): Bad file descriptor check_parent_access_fsp: SMB_VFS_FGET_NT_ACL failed for . with error NT_STATUS_INVALID_HANDLE
[2023/05/25 12:12:49.580490,  5, pid=82150, effective(1003258, 100001000), real(0, 0)] ../../source3/smbd/open.c:4571(mkdir_internal)
  mkdir_internal: check_parent_access_fsp on directory . for path qqq returned NT_STATUS_INVALID_HANDLE

(Same problem with Samba 4.17.6, 4.17.8 & 4.18.2)

Works on FreeBSD 12.3/12.4
Comment 1 Peter Eriksson 2023-05-25 11:45:56 UTC
This seems to happen not just for MKDIR, but almost all operations working on files. "DIR" and "CD" works fine.
Comment 2 Peter Eriksson 2023-05-29 12:20:43 UTC
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
Comment 3 Peter Eriksson 2023-05-29 13:02:44 UTC
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 ...
Comment 4 Peter Eriksson 2023-05-29 14:51:43 UTC
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...
Comment 5 Jeremy Allison 2023-05-30 17:28:56 UTC
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.
Comment 6 Jeremy Allison 2023-05-30 17:50:24 UTC
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.
Comment 7 Peter Eriksson 2023-05-31 05:51:52 UTC
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 8 Jeremy Allison 2023-05-31 17:25:21 UTC
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.
Comment 9 Jeremy Allison 2023-05-31 17:26:07 UTC
Yeah I missed the 'int flags' arg, but I'm sure you get what I mean.
Comment 10 Jeremy Allison 2023-05-31 17:33:29 UTC
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
}