From 2a98b53556edd4fd7f2168bc8a429ff4fe48993b Mon Sep 17 00:00:00 2001 From: Andrew Walker Date: Fri, 6 Jan 2023 09:44:56 -0800 Subject: [PATCH] Simplify get/set NFS4 ACL (#113) This removes an extra memory allocation / free from the NFS4 ACL xattr handler. Initially this was written rather quickly in the alpha cycle of SCALE and implemented in a way to ensure that xattr was exactly matching format used internally in samba's vfs_acl_xattr module. Since this time a more efficient conversion between the Samba format and various other ones was added for the purpose of inclusion in the Kernel NFS server. This change simplifies conversion between internal NFS ACL and external xattr representation, but has no impact on userspace and kernel consumers of this xattr (format does not change). Signed-off-by: Andrew Walker --- module/Kbuild.in | 1 - module/Makefile.in | 9 -- module/os/linux/zfs/nfs41acl.x | 74 ----------- module/os/linux/zfs/zpl_xattr.c | 212 +++++++++++++------------------- 4 files changed, 83 insertions(+), 213 deletions(-) delete mode 100644 module/os/linux/zfs/nfs41acl.x diff --git a/module/Kbuild.in b/module/Kbuild.in index f1f4ff4f14d6..a9003fa8a2cb 100644 --- a/module/Kbuild.in +++ b/module/Kbuild.in @@ -438,7 +438,6 @@ ZFS_OBJS_OS := \ abd_os.o \ arc_os.o \ mmp_os.o \ - nfs41acl_xdr.o \ policy.o \ qat.o \ qat_compress.o \ diff --git a/module/Makefile.in b/module/Makefile.in index 757dfc088ddf..d42d9cc1f804 100644 --- a/module/Makefile.in +++ b/module/Makefile.in @@ -53,15 +53,6 @@ FMAKE = env -u MAKEFLAGS make $(FMAKEFLAGS) modules-Linux: mkdir -p $(sort $(dir $(spl-objs) $(spl-))) mkdir -p $(sort $(dir $(zfs-objs) $(zfs-))) - @# Generate the nfs41acl XDR header. - rpcgen -h "@abs_srcdir@/os/linux/zfs/nfs41acl.x" > "@abs_srcdir@/os/linux/zfs/nfs41acl.h" - sed -i "/\/c\\#include \" "@abs_srcdir@/os/linux/zfs/nfs41acl.h" - sed -i "9i #include " "@abs_srcdir@/os/linux/zfs/nfs41acl.h" - @# Generate the nfs41acl XDR code. - rpcgen -c "@abs_srcdir@/os/linux/zfs/nfs41acl.x" > "@abs_srcdir@/os/linux/zfs/nfs41acl_xdr.c" - sed -i "5i #pragma GCC diagnostic push" "@abs_srcdir@/os/linux/zfs/nfs41acl_xdr.c" - sed -i "6i #pragma GCC diagnostic ignored \"-Wunused-variable\"" "@abs_srcdir@/os/linux/zfs/nfs41acl_xdr.c" - echo "#pragma GCC diagnostic pop" >> "@abs_srcdir@/os/linux/zfs/nfs41acl_xdr.c" @# Build the kernel modules. $(MAKE) -C @LINUX_OBJ@ $(if @KERNEL_CC@,CC=@KERNEL_CC@) \ $(if @KERNEL_LD@,LD=@KERNEL_LD@) $(if @KERNEL_LLVM@,LLVM=@KERNEL_LLVM@) \ diff --git a/module/os/linux/zfs/nfs41acl.x b/module/os/linux/zfs/nfs41acl.x deleted file mode 100644 index 1df04e8b8af8..000000000000 --- a/module/os/linux/zfs/nfs41acl.x +++ /dev/null @@ -1,74 +0,0 @@ -const ACE4_ACCESS_ALLOWED_ACE_TYPE = 0x00000000; -const ACE4_ACCESS_DENIED_ACE_TYPE = 0x00000001; -const ACE4_SYSTEM_AUDIT_ACE_TYPE = 0x00000002; -const ACE4_SYSTEM_ALARM_ACE_TYPE = 0x00000003; - -typedef u_int acetype4; - -const ACE4_FILE_INHERIT_ACE = 0x00000001; -const ACE4_DIRECTORY_INHERIT_ACE = 0x00000002; -const ACE4_NO_PROPAGATE_INHERIT_ACE = 0x00000004; -const ACE4_INHERIT_ONLY_ACE = 0x00000008; -const ACE4_SUCCESSFUL_ACCESS_ACE_FLAG = 0x00000010; -const ACE4_FAILED_ACCESS_ACE_FLAG = 0x00000020; -const ACE4_IDENTIFIER_GROUP = 0x00000040; -const ACE4_INHERITED_ACE = 0x00000080; - -typedef u_int aceflag4; - -const ACEI4_SPECIAL_WHO = 0x00000001; - -typedef u_int aceiflag4; - -const ACE4_SPECIAL_OWNER = 1; -const ACE4_SPECIAL_GROUP = 2; -const ACE4_SPECIAL_EVERYONE = 3; -const ACE4_SPECIAL_INTERACTIVE = 4; -const ACE4_SPECIAL_NETWORK = 5; -const ACE4_SPECIAL_DIALUP = 6; -const ACE4_SPECIAL_BATCH = 7; -const ACE4_SPECIAL_ANONYMOUS = 8; -const ACE4_SPECIAL_AUTHENTICATED = 9; -const ACE4_SPECIAL_SERVICE = 10; - -const ACE4_READ_DATA = 0x00000001; -const ACE4_LIST_DIRECTORY = 0x00000001; -const ACE4_WRITE_DATA = 0x00000002; -const ACE4_ADD_FILE = 0x00000002; -const ACE4_APPEND_DATA = 0x00000004; -const ACE4_ADD_SUBDIRECTORY = 0x00000004; -const ACE4_READ_NAMED_ATTRS = 0x00000008; -const ACE4_WRITE_NAMED_ATTRS = 0x00000010; -const ACE4_EXECUTE = 0x00000020; -const ACE4_DELETE_CHILD = 0x00000040; -const ACE4_READ_ATTRIBUTES = 0x00000080; -const ACE4_WRITE_ATTRIBUTES = 0x00000100; -const ACE4_WRITE_RETENTION = 0x00000200; -const ACE4_WRITE_RETENTION_HOLD = 0x00000400; - -const ACE4_DELETE = 0x00010000; -const ACE4_READ_ACL = 0x00020000; -const ACE4_WRITE_ACL = 0x00040000; -const ACE4_WRITE_OWNER = 0x00080000; -const ACE4_SYNCHRONIZE = 0x00100000; - -typedef u_int acemask4; - -struct nfsace4i { - acetype4 type; - aceflag4 flag; - aceiflag4 iflag; - acemask4 access_mask; - u_int who; -}; - -const ACL4_AUTO_INHERIT = 0x00000001; -const ACL4_PROTECTED = 0x00000002; -const ACL4_DEFAULTED = 0x00000004; - -typedef u_int aclflag4; - -struct nfsacl41i { - aclflag4 na41_flag; - nfsace4i na41_aces<>; -}; diff --git a/module/os/linux/zfs/zpl_xattr.c b/module/os/linux/zfs/zpl_xattr.c index 326c8215b83d..67f93ea8e8ba 100644 --- a/module/os/linux/zfs/zpl_xattr.c +++ b/module/os/linux/zfs/zpl_xattr.c @@ -85,8 +85,6 @@ #include #include #include -#include -#include "nfs41acl.h" #define NFS41ACL_XATTR "system.nfs4_acl_xdr" @@ -1590,6 +1588,10 @@ zpl_permission(struct inode *ip, int mask) return (ret); } +#define ACEI4_SPECIAL_WHO 1 +#define ACE4_SPECIAL_OWNER 1 +#define ACE4_SPECIAL_GROUP 2 +#define ACE4_SPECIAL_EVERYONE 3 #define NFS41ACL_MAX_ACES 128 #define NFS41_FLAGS (ACE_DIRECTORY_INHERIT_ACE| \ ACE_FILE_INHERIT_ACE| \ @@ -1601,23 +1603,14 @@ zpl_permission(struct inode *ip, int mask) /* * Macros for sanity checks related to XDR and ACL buffer sizes */ -#define ACE4SIZE (sizeof (nfsace4i)) -#define ACLBASE (sizeof (nfsacl41i)) -#define XDRBASE (2 * sizeof (uint_t)) +#define ACE4ELEM 5 +#define ACE4SIZE (ACE4ELEM * sizeof (u32)) +#define XDRBASE (2 * sizeof (u32)) -#define ACES_TO_SIZE(x, y) (x + (y * ACE4SIZE)) -#define SIZE_TO_ACES(x, y) ((y - x) / ACE4SIZE) -#define SIZE_IS_VALID(x, y) ((x >= ACES_TO_SIZE(y, 0)) && \ - (((x - y) % ACE4SIZE) == 0)) - -#define ACES_TO_ACLSIZE(x) (ACES_TO_SIZE(ACLBASE, x)) -#define ACES_TO_XDRSIZE(x) (ACES_TO_SIZE(XDRBASE, x)) - -#define ACLSIZE_TO_ACES(x) (SIZE_TO_ACES(ACLBASE, x)) -#define XDRSIZE_TO_ACES(x) (SIZE_TO_ACES(XDRBASE, x)) - -#define ACLSIZE_IS_VALID(x) (SIZE_IS_VALID(x, ACLBASE)) -#define XDRSIZE_IS_VALID(x) (SIZE_IS_VALID(x, XDRBASE)) +#define ACES_TO_XDRSIZE(x) (XDRBASE + (x * ACE4SIZE)) +#define XDRSIZE_TO_ACES(x) ((x - XDRBASE) / ACE4SIZE) +#define XDRSIZE_IS_VALID(x) ((x >= XDRBASE) && \ + (((x - XDRBASE) % ACE4SIZE) == 0)) static int __zpl_xattr_nfs41acl_list(struct inode *ip, char *list, size_t list_size, @@ -1637,31 +1630,29 @@ __zpl_xattr_nfs41acl_list(struct inode *ip, char *list, size_t list_size, ZPL_XATTR_LIST_WRAPPER(zpl_xattr_nfs41acl_list); static int -acep_to_nfsace4i(const ace_t *acep, nfsace4i *nacep) +acep_to_nfsace4i(const ace_t *acep, u32 *xattrbuf) { - nacep->type = acep->a_type; - nacep->flag = acep->a_flags & NFS41_FLAGS; - nacep->access_mask = acep->a_access_mask; + u32 who = 0, iflag = 0; switch (acep->a_flags & ACE_TYPE_FLAGS) { case ACE_OWNER: - nacep->iflag |= ACEI4_SPECIAL_WHO; - nacep->who = ACE4_SPECIAL_OWNER; + iflag = ACEI4_SPECIAL_WHO; + who = ACE4_SPECIAL_OWNER; break; case ACE_GROUP|ACE_IDENTIFIER_GROUP: - nacep->iflag |= ACEI4_SPECIAL_WHO; - nacep->who = ACE4_SPECIAL_GROUP; + iflag = ACEI4_SPECIAL_WHO; + who = ACE4_SPECIAL_GROUP; break; case ACE_EVERYONE: - nacep->iflag |= ACEI4_SPECIAL_WHO; - nacep->who = ACE4_SPECIAL_EVERYONE; + iflag = ACEI4_SPECIAL_WHO; + who = ACE4_SPECIAL_EVERYONE; break; case ACE_IDENTIFIER_GROUP: case 0: - nacep->who = acep->a_who; + who = acep->a_who; break; default: @@ -1670,49 +1661,48 @@ acep_to_nfsace4i(const ace_t *acep, nfsace4i *nacep) return (-EINVAL); } + *xattrbuf++ = htonl(acep->a_type); + *xattrbuf++ = htonl(acep->a_flags & NFS41_FLAGS); + *xattrbuf++ = htonl(iflag); + *xattrbuf++ = htonl(acep->a_access_mask); + *xattrbuf++ = htonl(who); + return (0); } static int -zfsacl_to_nfsacl41i(const vsecattr_t vsecp, nfsacl41i **_nacl, size_t *_size) +zfsacl_to_nfsacl41i(const vsecattr_t vsecp, u32 *xattrbuf) { - nfsacl41i *nacl = NULL; - nfsace4i *nacep = NULL; + int i, error = 0; ace_t *acep = NULL; - int i, error; - size_t acl_size; - - acl_size = ACES_TO_ACLSIZE(vsecp.vsa_aclcnt); + *xattrbuf++ = htonl(vsecp.vsa_aclflags); + *xattrbuf++ = htonl(vsecp.vsa_aclcnt); - nacl = kmem_alloc(acl_size, KM_SLEEP); - nacl->na41_aces.na41_aces_len = vsecp.vsa_aclcnt; - nacl->na41_flag = vsecp.vsa_aclflags; - nacep = (nfsace4i *)((char *)nacl + sizeof (nfsacl41i)); - nacl->na41_aces.na41_aces_val = nacep; - - for (i = 0; i < nacl->na41_aces.na41_aces_len; i++) { - nacep = &nacl->na41_aces.na41_aces_val[i]; + for (i = 0; i < vsecp.vsa_aclcnt; i++, xattrbuf += ACE4ELEM) { acep = vsecp.vsa_aclentp + (i * sizeof (ace_t)); - error = acep_to_nfsace4i(acep, nacep); - if (error) { - kmem_free(nacl, acl_size); - return (error); - } + + error = acep_to_nfsace4i(acep, xattrbuf); + if (error) + break; } - *_size = acl_size; - *_nacl = nacl; - return (0); + + return (error); } static int -nfsace4i_to_acep(const nfsace4i *nacep, ace_t *acep) +nfsace4i_to_acep(const u32 *xattrbuf, ace_t *acep) { - acep->a_type = nacep->type; - acep->a_flags = nacep->flag & NFS41_FLAGS; - acep->a_access_mask = nacep->access_mask; - if (nacep->iflag & ACEI4_SPECIAL_WHO) { - switch (nacep->who) { + u32 iflag, id; + + acep->a_type = ntohl(*(xattrbuf++)); + acep->a_flags = ntohl(*(xattrbuf++)) & NFS41_FLAGS; + iflag = ntohl(*(xattrbuf++)); + acep->a_access_mask = ntohl(*(xattrbuf++)); + id = ntohl(*(xattrbuf++)); + + if (iflag & ACEI4_SPECIAL_WHO) { + switch (id) { case ACE4_SPECIAL_OWNER: acep->a_flags |= ACE_OWNER; acep->a_who = -1; @@ -1729,51 +1719,57 @@ nfsace4i_to_acep(const nfsace4i *nacep, ace_t *acep) break; default: - dprintf("Unknown id 0x%08x\n", nacep->who); + dprintf("Unknown id 0x%08x\n", id); return (-EINVAL); } } else { - acep->a_who = nacep->who; + acep->a_who = id; } return (0); } static int -nfsacl41i_to_zfsacl(const nfsacl41i *nacl, vsecattr_t *_vsecp) +nfsacl41i_to_zfsacl(const u32 *xattrbuf, size_t bufsz, vsecattr_t *vsecp) { - int i, error = 0; - vsecattr_t vsecp; - - vsecp.vsa_aclcnt = nacl->na41_aces.na41_aces_len; - vsecp.vsa_aclflags = nacl->na41_flag & ACL_FLAGS_ALL; - vsecp.vsa_aclentsz = vsecp.vsa_aclcnt * sizeof (ace_t); - vsecp.vsa_mask = (VSA_ACE | VSA_ACE_ACLFLAGS); - vsecp.vsa_aclentp = kmem_alloc(vsecp.vsa_aclentsz, KM_SLEEP); - - for (i = 0; i < vsecp.vsa_aclcnt; i++) { - ace_t *acep = vsecp.vsa_aclentp + (i * sizeof (ace_t)); - nfsace4i *nacep = &nacl->na41_aces.na41_aces_val[i]; - error = nfsace4i_to_acep(nacep, acep); + int error; + int i; + + vsecp->vsa_aclflags = ntohl(*(xattrbuf++)); + vsecp->vsa_aclcnt = ntohl(*(xattrbuf++)); + bufsz -= (2 * sizeof (u32)); + vsecp->vsa_aclentsz = vsecp->vsa_aclcnt * sizeof (ace_t); + + if (bufsz != (vsecp->vsa_aclcnt * ACE4SIZE)) { + dprintf("Embedded ACL count [%d] is larger than " + "can fit in provided buffer size: %zu\n", + vsecp->vsa_aclcnt, bufsz); + return (-ERANGE); + } + + vsecp->vsa_aclentp = kmem_alloc(vsecp->vsa_aclentsz, KM_SLEEP); + + for (i = 0; i < vsecp->vsa_aclcnt; i++, xattrbuf += ACE4ELEM) { + ace_t *acep = vsecp->vsa_aclentp + (i * sizeof (ace_t)); + + error = nfsace4i_to_acep(xattrbuf, acep); if (error) { + kmem_free(vsecp->vsa_aclentp, vsecp->vsa_aclentsz); return (error); } } - *_vsecp = vsecp; - return (error); + + return (0); } static int __zpl_xattr_nfs41acl_get(struct inode *ip, const char *name, void *buffer, size_t size) { - vsecattr_t vsecp; + vsecattr_t vsecp = {0}; cred_t *cr = CRED(); int ret, fl; - size_t acl_size = 0, xdr_size = 0; - XDR xdr = {0}; - boolean_t ok; - nfsacl41i *nacl = NULL; + size_t xdr_size; /* xattr_resolve_name will do this for us if this is defined */ #ifndef HAVE_XATTR_HANDLER_NAME @@ -1827,22 +1823,9 @@ __zpl_xattr_nfs41acl_get(struct inode *ip, const char *name, goto nfs4acl_get_out; } - ret = zfsacl_to_nfsacl41i(vsecp, &nacl, &acl_size); - if (ret) { - ret = -ENOMEM; - goto nfs4acl_get_out; - } - - xdrmem_create(&xdr, (char *)buffer, xdr_size, XDR_ENCODE); - ok = xdr_nfsacl41i(&xdr, nacl); - if (!ok) { - ret = -ENOMEM; - kmem_free(nacl, acl_size); - goto nfs4acl_get_out; - } - - kmem_free(nacl, acl_size); - ret = xdr_size; + ret = zfsacl_to_nfsacl41i(vsecp, (u32 *)buffer); + if (ret == 0) + ret = xdr_size; nfs4acl_get_out: kmem_free(vsecp.vsa_aclentp, vsecp.vsa_aclentsz); @@ -1858,13 +1841,8 @@ __zpl_xattr_nfs41acl_set(zidmap_t *mnt_ns, { (void) mnt_ns; cred_t *cr = CRED(); - vsecattr_t vsecp; - char *bufp = NULL; - nfsacl41i *nacl = NULL; - boolean_t ok; - XDR xdr = {0}; - size_t acl_size = 0; int error, fl, naces; + vsecattr_t vsecp = { .vsa_mask = (VSA_ACE | VSA_ACE_ACLFLAGS) }; if (ITOZSB(ip)->z_acl_type != ZFS_ACLTYPE_NFSV4) return (-EOPNOTSUPP); @@ -1889,34 +1867,10 @@ __zpl_xattr_nfs41acl_set(zidmap_t *mnt_ns, if (!XDRSIZE_IS_VALID(size)) { return (-EINVAL); } - bufp = (char *)value; - acl_size = ACES_TO_ACLSIZE(naces); - nacl = kmem_alloc(sizeof (nfsacl41i), KM_SLEEP); - /* - * NULL may still be returned with KM_SLEEP set. - * In principal, checks for SIZE of xattr are - * sufficient to protect, but check for NULL anyway. - */ - if (nacl == NULL) { - return (-ENOMEM); - } - xdrmem_create(&xdr, bufp, acl_size, XDR_DECODE); - ok = xdr_nfsacl41i(&xdr, nacl); - if (!ok) { - kmem_free(nacl, sizeof (nfsacl41i)); - return (-ENOMEM); - } - error = nfsacl41i_to_zfsacl(nacl, &vsecp); - if (error) { - kmem_free(nacl, sizeof (nfsacl41i)); + error = nfsacl41i_to_zfsacl((u32 *)value, size, &vsecp); + if (error) return (error); - } - - /* XDR_DECODE allocates memory for the array of aces */ - kmem_free(nacl->na41_aces.na41_aces_val, - (nacl->na41_aces.na41_aces_len * ACE4SIZE)); - kmem_free(nacl, sizeof (nfsacl41i)); crhold(cr); fl = capable(CAP_DAC_OVERRIDE) ? ATTR_NOACLCHECK : 0;