> On 2016/05/23, 15:06, "James Simmons" <jsimmons@xxxxxxxxxxxxx> wrote:
> >
> >> Currently, getxattr() and setxattr() check for the xattr names
> >> "system.posix_acl_{access,default}" and perform in-place UID / GID
> >> namespace mappings in the xattr values. Filesystems then again check for
> >> the same xattr names to handle those attributes, almost always using the
> >> standard posix_acl_{access,default}_xattr_handler handlers. This is
> >> unnecessary overhead; move the namespace conversion into the xattr
> >> handlers instead.
> >>
> >> For filesystems that use the POSIX ACL xattr handlers, no change is
> >> required. Filesystems that don't use those handlers (cifs and lustre)
> >> need to take care of the namespace mapping themselves now.
> >>
> >> The only user left of the posix_acl_fix_xattr_{from,to}_user helpers is
> >> lustre, so this patch moves them into lustre.
> >>
> >> Signed-off-by: Andreas Gruenbacher <agruenba@xxxxxxxxxx>
> >> ---
> >>
> >> I'm reasonably confident about the core and cifs changes in this patch.
> >> The lustre code is pretty weird though, so could I please get a careful
> >> review on the changes there?
> >
> >Nak on the Lustre changes. POSIX ACLs are also handled in mdc_request.c.
> >Besides POSIX ACLs lustre has created a extended ACL as well that is
> >grouped in with posix ACL handling. This extended ACL is like POSIX acls
> >except it also contains the state (deleted, modified, ...) of the ACL.
> >Besides normal local access control list handling Lustre manages remote
> >access control list handling. You can read about this handling is in
> >llite_rmtacl.c. This code was written long before I became involved with
> >lustre so the exact details are fuzzy to me.
>
> James,
> the remote ACL code never found any usage in the field and can be
> deleted. Please see http://review.whamcloud.com/19789 for details.
That is a huge cleanup which will make Greg very happy. The tools and test
are going to be updated so the landing has to be just right so we
don't have a flood of test failures. Gruenbacher with the 19789 patch
Dilger pointed out Lustre's POSIX ACL code just becomes ordinary which
means we can use the default POSIS xattr handler. You wouldn't have to
keep posix_acl.c around with these changes.
> Cheers, Andreas
>
> > The guts of this are handled is located at:
> >
> >drivers/staging/lustre/lustre/obdclass/acl.c
> >
> >P.S
> >
> > A you probably have noticed Lustre has had an uptick in development
> >which means the code is now changing all the time in staging. How should
> >we handle the changes you are working in your own trees verses what is
> >happing in staging. For example I'm looking at moving lustre to the
> >xattr_handles. Should I push it to you and wait until it works it way
> >into Greg's tree. What do the merge schedules look like. Lastly the
> >a_refcount for the POSIX acl changed with your xattr updates which now
> >causes lustre to crash :-(
> >
> >> Thanks,
> >> Andreas
> >>
> >> drivers/staging/lustre/lustre/llite/Makefile | 1 +
> >> .../staging/lustre/lustre/llite/llite_internal.h | 3 ++
> >> drivers/staging/lustre/lustre/llite/posix_acl.c | 62 ++++++++++++++++++++++
> >> drivers/staging/lustre/lustre/llite/xattr.c | 8 ++-
> >> fs/cifs/cifssmb.c | 41 ++++++++++----
> >> fs/posix_acl.c | 62 +---------------------
> >> fs/xattr.c | 7 ---
> >> include/linux/posix_acl_xattr.h | 12 -----
> >> 8 files changed, 107 insertions(+), 89 deletions(-)
> >> create mode 100644 drivers/staging/lustre/lustre/llite/posix_acl.c
> >>
> >> diff --git a/drivers/staging/lustre/lustre/llite/Makefile b/drivers/staging/lustre/lustre/llite/Makefile
> >> index 2ce10ff..67125f8 100644
> >> --- a/drivers/staging/lustre/lustre/llite/Makefile
> >> +++ b/drivers/staging/lustre/lustre/llite/Makefile
> >> @@ -7,5 +7,6 @@ lustre-y := dcache.o dir.o file.o llite_close.o llite_lib.o llite_nfs.o \
> >> glimpse.o lcommon_cl.o lcommon_misc.o \
> >> vvp_dev.o vvp_page.o vvp_lock.o vvp_io.o vvp_object.o vvp_req.o \
> >> lproc_llite.o
> >> +lustre-$(CONFIG_FS_POSIX_ACL) += posix_acl.o
> >>
> >> llite_lloop-y := lloop.o
> >> diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h b/drivers/staging/lustre/lustre/llite/llite_internal.h
> >> index ce1f949..d454dfb 100644
> >> --- a/drivers/staging/lustre/lustre/llite/llite_internal.h
> >> +++ b/drivers/staging/lustre/lustre/llite/llite_internal.h
> >> @@ -1402,4 +1402,7 @@ int cl_local_size(struct inode *inode);
> >> __u64 cl_fid_build_ino(const struct lu_fid *fid, int api32);
> >> __u32 cl_fid_build_gen(const struct lu_fid *fid);
> >>
> >> +void posix_acl_fix_xattr_from_user(void *value, size_t size);
> >> +void posix_acl_fix_xattr_to_user(void *value, size_t size);
> >> +
> >> #endif /* LLITE_INTERNAL_H */
> >> diff --git a/drivers/staging/lustre/lustre/llite/posix_acl.c b/drivers/staging/lustre/lustre/llite/posix_acl.c
> >> new file mode 100644
> >> index 0000000..4fabd0f
> >> --- /dev/null
> >> +++ b/drivers/staging/lustre/lustre/llite/posix_acl.c
> >> @@ -0,0 +1,62 @@
> >> +#include <linux/kernel.h>
> >> +#include <linux/fs.h>
> >> +#include <linux/posix_acl_xattr.h>
> >> +#include <linux/user_namespace.h>
> >> +
> >> +/*
> >> + * Fix up the uids and gids in posix acl extended attributes in place.
> >> + */
> >> +static void posix_acl_fix_xattr_userns(
> >> + struct user_namespace *to, struct user_namespace *from,
> >> + void *value, size_t size)
> >> +{
> >> + posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
> >> + posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;
> >> + int count;
> >> + kuid_t uid;
> >> + kgid_t gid;
> >> +
> >> + if (!value)
> >> + return;
> >> + if (size < sizeof(posix_acl_xattr_header))
> >> + return;
> >> + if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
> >> + return;
> >> +
> >> + count = posix_acl_xattr_count(size);
> >> + if (count < 0)
> >> + return;
> >> + if (count == 0)
> >> + return;
> >> +
> >> + for (end = entry + count; entry != end; entry++) {
> >> + switch(le16_to_cpu(entry->e_tag)) {
> >> + case ACL_USER:
> >> + uid = make_kuid(from, le32_to_cpu(entry->e_id));
> >> + entry->e_id = cpu_to_le32(from_kuid(to, uid));
> >> + break;
> >> + case ACL_GROUP:
> >> + gid = make_kgid(from, le32_to_cpu(entry->e_id));
> >> + entry->e_id = cpu_to_le32(from_kgid(to, gid));
> >> + break;
> >> + default:
> >> + break;
> >> + }
> >> + }
> >> +}
> >> +
> >> +void posix_acl_fix_xattr_from_user(void *value, size_t size)
> >> +{
> >> + struct user_namespace *user_ns = current_user_ns();
> >> + if (user_ns == &init_user_ns)
> >> + return;
> >> + posix_acl_fix_xattr_userns(&init_user_ns, user_ns, value, size);
> >> +}
> >> +
> >> +void posix_acl_fix_xattr_to_user(void *value, size_t size)
> >> +{
> >> + struct user_namespace *user_ns = current_user_ns();
> >> + if (user_ns == &init_user_ns)
> >> + return;
> >> + posix_acl_fix_xattr_userns(user_ns, &init_user_ns, value, size);
> >> +}
> >> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
> >> index ed4de04..c721b44 100644
> >> --- a/drivers/staging/lustre/lustre/llite/xattr.c
> >> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> >> @@ -144,6 +144,9 @@ int ll_setxattr_common(struct inode *inode, const char *name,
> >> return -EOPNOTSUPP;
> >>
> >> #ifdef CONFIG_FS_POSIX_ACL
> >> + if (xattr_type == XATTR_ACL_ACCESS_T ||
> >> + xattr_type == XATTR_ACL_DEFAULT_T)
> >> + posix_acl_fix_xattr_from_user((void *)value, size);
> >> if (sbi->ll_flags & LL_SBI_RMT_CLIENT &&
> >> (xattr_type == XATTR_ACL_ACCESS_T ||
> >> xattr_type == XATTR_ACL_DEFAULT_T)) {
> >> @@ -348,7 +351,7 @@ int ll_getxattr_common(struct inode *inode, const char *name,
> >> if (!acl)
> >> return -ENODATA;
> >>
> >> - rc = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);
> >> + rc = posix_acl_to_xattr(current_user_ns(), acl, buffer, size);
> >> posix_acl_release(acl);
> >> return rc;
> >> }
> >> @@ -436,6 +439,9 @@ getxattr_nocache:
> >> goto out;
> >> }
> >> }
> >> + if (rc >= 0 && (xattr_type == XATTR_ACL_ACCESS_T ||
> >> + xattr_type == XATTR_ACL_DEFAULT_T))
> >> + posix_acl_fix_xattr_to_user(buffer, rc);
> >> #endif
> >>
> >> out_xattr:
> >> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> >> index d47197e..9dc001f 100644
> >> --- a/fs/cifs/cifssmb.c
> >> +++ b/fs/cifs/cifssmb.c
> >> @@ -3337,10 +3337,25 @@ CIFSSMB_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
> >> static void cifs_convert_ace(posix_acl_xattr_entry *ace,
> >> struct cifs_posix_ace *cifs_ace)
> >> {
> >> + u32 cifs_id, id = -1;
> >> +
> >> /* u8 cifs fields do not need le conversion */
> >> ace->e_perm = cpu_to_le16(cifs_ace->cifs_e_perm);
> >> ace->e_tag = cpu_to_le16(cifs_ace->cifs_e_tag);
> >> - ace->e_id = cpu_to_le32(le64_to_cpu(cifs_ace->cifs_uid));
> >> + switch(cifs_ace->cifs_e_tag) {
> >> + case ACL_USER:
> >> + cifs_id = le64_to_cpu(cifs_ace->cifs_uid);
> >> + id = from_kuid(current_user_ns(),
> >> + make_kuid(&init_user_ns, cifs_id));
> >> + break;
> >> +
> >> + case ACL_GROUP:
> >> + cifs_id = le64_to_cpu(cifs_ace->cifs_uid);
> >> + id = from_kgid(current_user_ns(),
> >> + make_kgid(&init_user_ns, cifs_id));
> >> + break;
> >> + }
> >> + ace->e_id = cpu_to_le32(id);
> >> /*
> >> cifs_dbg(FYI, "perm %d tag %d id %d\n",
> >> ace->e_perm, ace->e_tag, ace->e_id);
> >> @@ -3408,21 +3423,29 @@ static int cifs_copy_posix_acl(char *trgt, char *src, const int buflen,
> >> static __u16 convert_ace_to_cifs_ace(struct cifs_posix_ace *cifs_ace,
> >> const posix_acl_xattr_entry *local_ace)
> >> {
> >> - __u16 rc = 0; /* 0 = ACL converted ok */
> >> + u32 cifs_id = -1, id;
> >>
> >> cifs_ace->cifs_e_perm = le16_to_cpu(local_ace->e_perm);
> >> cifs_ace->cifs_e_tag = le16_to_cpu(local_ace->e_tag);
> >> - /* BB is there a better way to handle the large uid? */
> >> - if (local_ace->e_id == cpu_to_le32(-1)) {
> >> - /* Probably no need to le convert -1 on any arch but can not hurt */
> >> - cifs_ace->cifs_uid = cpu_to_le64(-1);
> >> - } else
> >> - cifs_ace->cifs_uid = cpu_to_le64(le32_to_cpu(local_ace->e_id));
> >> + switch(cifs_ace->cifs_e_tag) {
> >> + case ACL_USER:
> >> + id = le32_to_cpu(local_ace->e_id);
> >> + cifs_id = from_kuid(&init_user_ns,
> >> + make_kuid(current_user_ns(), id));
> >> + break;
> >> +
> >> + case ACL_GROUP:
> >> + id = le32_to_cpu(local_ace->e_id);
> >> + cifs_id = from_kgid(&init_user_ns,
> >> + make_kgid(current_user_ns(), id));
> >> + break;
> >> + }
> >> + cifs_ace->cifs_uid = cpu_to_le64(cifs_id);
> >> /*
> >> cifs_dbg(FYI, "perm %d tag %d id %d\n",
> >> ace->e_perm, ace->e_tag, ace->e_id);
> >> */
> >> - return rc;
> >> + return 0;
> >> }
> >>
> >> /* Convert ACL from local Linux POSIX xattr to CIFS POSIX ACL wire format */
> >> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> >> index 2c60f17..fca281c 100644
> >> --- a/fs/posix_acl.c
> >> +++ b/fs/posix_acl.c
> >> @@ -627,64 +627,6 @@ no_mem:
> >> EXPORT_SYMBOL_GPL(posix_acl_create);
> >>
> >> /*
> >> - * Fix up the uids and gids in posix acl extended attributes in place.
> >> - */
> >> -static void posix_acl_fix_xattr_userns(
> >> - struct user_namespace *to, struct user_namespace *from,
> >> - void *value, size_t size)
> >> -{
> >> - posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
> >> - posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;
> >> - int count;
> >> - kuid_t uid;
> >> - kgid_t gid;
> >> -
> >> - if (!value)
> >> - return;
> >> - if (size < sizeof(posix_acl_xattr_header))
> >> - return;
> >> - if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
> >> - return;
> >> -
> >> - count = posix_acl_xattr_count(size);
> >> - if (count < 0)
> >> - return;
> >> - if (count == 0)
> >> - return;
> >> -
> >> - for (end = entry + count; entry != end; entry++) {
> >> - switch(le16_to_cpu(entry->e_tag)) {
> >> - case ACL_USER:
> >> - uid = make_kuid(from, le32_to_cpu(entry->e_id));
> >> - entry->e_id = cpu_to_le32(from_kuid(to, uid));
> >> - break;
> >> - case ACL_GROUP:
> >> - gid = make_kgid(from, le32_to_cpu(entry->e_id));
> >> - entry->e_id = cpu_to_le32(from_kgid(to, gid));
> >> - break;
> >> - default:
> >> - break;
> >> - }
> >> - }
> >> -}
> >> -
> >> -void posix_acl_fix_xattr_from_user(void *value, size_t size)
> >> -{
> >> - struct user_namespace *user_ns = current_user_ns();
> >> - if (user_ns == &init_user_ns)
> >> - return;
> >> - posix_acl_fix_xattr_userns(&init_user_ns, user_ns, value, size);
> >> -}
> >> -
> >> -void posix_acl_fix_xattr_to_user(void *value, size_t size)
> >> -{
> >> - struct user_namespace *user_ns = current_user_ns();
> >> - if (user_ns == &init_user_ns)
> >> - return;
> >> - posix_acl_fix_xattr_userns(user_ns, &init_user_ns, value, size);
> >> -}
> >> -
> >> -/*
> >> * Convert from extended attribute to in-memory representation.
> >> */
> >> struct posix_acl *
> >> @@ -814,7 +756,7 @@ posix_acl_xattr_get(const struct xattr_handler *handler,
> >> if (acl == NULL)
> >> return -ENODATA;
> >>
> >> - error = posix_acl_to_xattr(&init_user_ns, acl, value, size);
> >> + error = posix_acl_to_xattr(current_user_ns(), acl, value, size);
> >> posix_acl_release(acl);
> >>
> >> return error;
> >> @@ -840,7 +782,7 @@ posix_acl_xattr_set(const struct xattr_handler *handler,
> >> return -EPERM;
> >>
> >> if (value) {
> >> - acl = posix_acl_from_xattr(&init_user_ns, value, size);
> >> + acl = posix_acl_from_xattr(current_user_ns(), value, size);
> >> if (IS_ERR(acl))
> >> return PTR_ERR(acl);
> >>
> >> diff --git a/fs/xattr.c b/fs/xattr.c
> >> index b11945e..b648b05 100644
> >> --- a/fs/xattr.c
> >> +++ b/fs/xattr.c
> >> @@ -20,7 +20,6 @@
> >> #include <linux/fsnotify.h>
> >> #include <linux/audit.h>
> >> #include <linux/vmalloc.h>
> >> -#include <linux/posix_acl_xattr.h>
> >>
> >> #include <asm/uaccess.h>
> >>
> >> @@ -329,9 +328,6 @@ setxattr(struct dentry *d, const char __user *name, const void __user *value,
> >> error = -EFAULT;
> >> goto out;
> >> }
> >> - if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
> >> - (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
> >> - posix_acl_fix_xattr_from_user(kvalue, size);
> >> }
> >>
> >> error = vfs_setxattr(d, kname, kvalue, size, flags);
> >> @@ -426,9 +422,6 @@ getxattr(struct dentry *d, const char __user *name, void __user *value,
> >>
> >> error = vfs_getxattr(d, kname, kvalue, size);
> >> if (error > 0) {
> >> - if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
> >> - (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
> >> - posix_acl_fix_xattr_to_user(kvalue, size);
> >> if (size && copy_to_user(value, kvalue, error))
> >> error = -EFAULT;
> >> } else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
> >> diff --git a/include/linux/posix_acl_xattr.h b/include/linux/posix_acl_xattr.h
> >> index e5e8ec4..9789aba 100644
> >> --- a/include/linux/posix_acl_xattr.h
> >> +++ b/include/linux/posix_acl_xattr.h
> >> @@ -48,18 +48,6 @@ posix_acl_xattr_count(size_t size)
> >> return size / sizeof(posix_acl_xattr_entry);
> >> }
> >>
> >> -#ifdef CONFIG_FS_POSIX_ACL
> >> -void posix_acl_fix_xattr_from_user(void *value, size_t size);
> >> -void posix_acl_fix_xattr_to_user(void *value, size_t size);
> >> -#else
> >> -static inline void posix_acl_fix_xattr_from_user(void *value, size_t size)
> >> -{
> >> -}
> >> -static inline void posix_acl_fix_xattr_to_user(void *value, size_t size)
> >> -{
> >> -}
> >> -#endif
> >> -
> >> struct posix_acl *posix_acl_from_xattr(struct user_namespace *user_ns,
> >> const void *value, size_t size);
> >> int posix_acl_to_xattr(struct user_namespace *user_ns,
> >> --
> >> 2.5.5
> >>
> >> _______________________________________________
> >> lustre-devel mailing list
> >> lustre-devel@xxxxxxxxxxxxxxxx
> >> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
> >>
> >_______________________________________________
> >lustre-devel mailing list
> >lustre-devel@xxxxxxxxxxxxxxxx
> >http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
> >
>
>
>
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
