| Message ID | 1464008989-3812-1-git-send-email-agruenba@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> 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@redhat.com> > --- > > 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. 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@lists.lustre.org > http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org > -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-05-23 23:06 GMT+02:00 James Simmons <jsimmons@infradead.org>: > 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. The guts of this are handled > is located at: > > drivers/staging/lustre/lustre/obdclass/acl.c I'm not sure we're talking about the same thing here. As far as I can see, the only code path from getxattr(2) and settxattr(2) into lustre is through ll_getxattr and ll_setxattr. So far, the vfs performed the uid/gid mappings before calling into the filesystem. With this patch, the filesystem performs this mapping instead. In the lustre case, pretty much directly after entering ll_getxattr and immediately before leaving ll_setxattr. The rest of the code has and still does operate in the "init uid/gid namespace". Did I miss anything? > 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. Okay, great. > Should I push it to you and wait until it works it way > into Greg's tree. Changing lustre to use xattr handlers does not dependent on any of my pending xattr changes, only the other way around. Please copy me for review and merge through Greg's tree as usual. Once I have your changes on a public branch, I can merge that into my xattr inode operation removal branch. > What do the merge schedules look like. I hope the {get,set,remove}xattr inode operations will go away in the next merge window, so it would be good to have the lustre xattr handler convestion on a public branch relatively soon if possible. > Lastly the a_refcount for the POSIX acl changed with your xattr updates which now > causes lustre to crash :-( You mean commit b8a7a3a6 "posix_acl: Inode acl caching fixes"? It seems a "forget_cached_acl(inode, type);" before returning the acl in ll_get_acl is missing -- but lustre still shouldn't crash because of that. Thanks, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Reviewed by: Steve French <steve.french@primarydata.com> (for the non-lustre pieces of the patch) On Mon, May 23, 2016 at 8:09 AM, Andreas Gruenbacher <agruenba@redhat.com> 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@redhat.com> > --- > > 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? > > 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 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 23, 2016 at 03:09:49PM +0200, Andreas Gruenbacher wrote: > 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? Lustre is in the staging tree because it's an unrescuable piece of junk. Please just ignore it and move the proper parts of the kernel forward. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, May 23, 2016 at 03:09:49PM +0200, Andreas Gruenbacher 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. Please, are you sure that the changes in posix_acl_xattr_get() and posix_acl_xattr_set() are safe ? you are reading into current user namespace, from a first view this is not safe unless I'm missing something... they should map into init_user_ns... Please Cc the user namespace maintainers before. Thank you! > 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@redhat.com> > --- > > 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? > > 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 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 24, 2016 at 5:41 PM, Djalal Harouni <tixxdz@gmail.com> wrote: > On Mon, May 23, 2016 at 03:09:49PM +0200, Andreas Gruenbacher 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. > > Please, are you sure that the changes in posix_acl_xattr_get() and > posix_acl_xattr_set() are safe ? you are reading into current user > namespace, from a first view this is not safe unless I'm missing > something... they should map into init_user_ns... Yes, moving the namespace conversion from the VFS into those functions so that we don't have to check for those attributes and parse them twice is exactly the point of this patch. > Please Cc the user namespace maintainers before. Thank you! Eric, Andy, anyone else? Thanks, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016/05/23, 15:06, "James Simmons" <jsimmons@infradead.org> 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@redhat.com> >> --- >> >> 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. 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@lists.lustre.org >> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org >> >_______________________________________________ >lustre-devel mailing list >lustre-devel@lists.lustre.org >http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org >
> On 2016/05/23, 15:06, "James Simmons" <jsimmons@infradead.org> 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@redhat.com> > >> --- > >> > >> 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@lists.lustre.org > >> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org > >> > >_______________________________________________ > >lustre-devel mailing list > >lustre-devel@lists.lustre.org > >http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org > > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On May 24, 2016, at 2:47 AM, Christoph Hellwig <hch@infradead.org> wrote: > > On Mon, May 23, 2016 at 03:09:49PM +0200, Andreas Gruenbacher wrote: >> 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? > > Lustre is in the staging tree because it's an unrescuable piece of junk. > Please just ignore it and move the proper parts of the kernel forward. Don't be shy Christoph, tell us how you really feel about Lustre... :-) Fortunately, that "piece of junk" is working fine and is running on most of the supercomputers in the world (9 of the top 10, 70%+ of the top 100, ...) with several sites having 50PB+ of storage, and 1TB/s+ to 20000+ clients, in a single coherent POSIX filesystem. It's true that the staging client isn't getting cleaned up as fast as anyone likes, but that is because it is complex software in use at many sites for many years and we can't just stop to completely rewrite the code and hope it still works. Even XFS took many years to get rid of the IRIX wrapping layers, and still does a lot of things outside of the VFS/VM (stats, memory allocation, IO submission, etc.) because the VFS/VM doesn't work the way XFS wants it to (or vice versa). This is not a slight against XFS, just a sign that non-trivial filesystems take a long time to change while maintaining existing functionality. This is mainly a difference in how code is added to the kernel today vs. how it was added back then. Cheers, Andreas
On Tue, May 24, 2016 at 10:35 PM, James Simmons <jsimmons@infradead.org> wrote: > 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. Change http://review.whamcloud.com/19789 doesn't switch Lustre over to using xattr handlers. As long as Lustre doesn't use those (i.e., use posix_acl_access_xattr_handler and posix_acl_default_xattr_handler), it will still need the uid/gid namespace conversion code in ll_getxattr and ll_setxattr for this patch to be correct. The merge conflict between this patch and http://review.whamcloud.com/19789 is easily resolved though. Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Andreas Gruenbacher <agruenba@redhat.com> writes: > On Tue, May 24, 2016 at 5:41 PM, Djalal Harouni <tixxdz@gmail.com> wrote: >> On Mon, May 23, 2016 at 03:09:49PM +0200, Andreas Gruenbacher 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. >> >> Please, are you sure that the changes in posix_acl_xattr_get() and >> posix_acl_xattr_set() are safe ? you are reading into current user >> namespace, from a first view this is not safe unless I'm missing >> something... they should map into init_user_ns... > > Yes, moving the namespace conversion from the VFS into those functions > so that we don't have to check for those attributes and parse them > twice is exactly the point of this patch. In general I am in favor of cleaning up the xattr and acl code in the kernel. However I am not certain that your changes succeed in getting us where we want to go. My feel is that what we want to do is to update the cached acl when we write it from userspace, to update the disk with the new acl when the inode is sync'd to disk, and let the vfs handle the translation from the cached posix acl to the vfs getxattr and setxattr system calls. In the long term anything else is madness. Currently posix acl reads and updates bypass the vfs acl cache for the inode and that just looks wrong. The reason that fixup happens in a separate pass from everything else today is that when I was wrote posix_acl_fix_xattr_to_user and posix_acl_xattr_from_user a number of filesystems had a very strange structure that completely bypassed any code conversion routines and made some strange assumptions. I don't remember which filesystems those were but it was neither lustre, nor cifs, nor nfs that were the problem cases when I was looking. I don't see your patch addressing that problem so either someone has already fixed those issues or they have been overlooked. There is a complication that is comming shortly (next merge window unless major unfixable bugs show up between now and them). There is a new field s_user_ns that is being introduced for filesystems to record their owner and their default translation rules to kuids. That will make the hard coded &init_user_ns values in your patch wrong. >> Please Cc the user namespace maintainers before. Thank you! > > Eric, Andy, anyone else? Serge Hallyn has a pending patch that adds a similar translation to the security.capability xattr. Which is one more case of where caching and translation at the VFS layer are makes sense. All of that said I am definitely in favor of cleaning up this area of code. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 26, 2016 at 1:30 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: > Andreas Gruenbacher <agruenba@redhat.com> writes: > >> On Tue, May 24, 2016 at 5:41 PM, Djalal Harouni <tixxdz@gmail.com> wrote: >>> On Mon, May 23, 2016 at 03:09:49PM +0200, Andreas Gruenbacher 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. >>> >>> Please, are you sure that the changes in posix_acl_xattr_get() and >>> posix_acl_xattr_set() are safe ? you are reading into current user >>> namespace, from a first view this is not safe unless I'm missing >>> something... they should map into init_user_ns... >> >> Yes, moving the namespace conversion from the VFS into those functions >> so that we don't have to check for those attributes and parse them >> twice is exactly the point of this patch. > > In general I am in favor of cleaning up the xattr and acl code in the > kernel. However I am not certain that your changes succeed in getting > us where we want to go. > > My feel is that what we want to do is to update the cached acl when we > write it from userspace, to update the disk with the new acl when the > inode is sync'd to disk, and let the vfs handle the translation from > the cached posix acl to the vfs getxattr and setxattr system calls. > In the long term anything else is madness. > > Currently posix acl reads and updates bypass the vfs acl cache for the > inode and that just looks wrong. Not all filesystems cache acls in the inode (i_acl and i_default_acl), so there has to be some way to "bypass the cache" if you want to put it that way. All normal filesystems that cache ACLs locally implement the get_acl and set_acl inode operations. They put posix_acl_access_xattr_handler and posix_acl_default_xattr_handler into s_xattr and use the generic_{get,set,remove}xattr inode operations to hook things up appropriately. The xattr handlers convert to/from xattrs and struct posix_acl and call the get_acl and set_acl inode operations. That's where the namespace conversion is supposed to happen as well; this avoids special-casing it in the VFS. The only filesystems that don't do things that way are CIFS and Lustre. Doing the appropriate namespace mapping in CIFS is easy (see this patch). So it's only Lustre that's left with the original in-place xattr conversion. Another pending cleanup (https://lwn.net/Articles/688390/) gets rid of the {get,set,remove}xattr inode operations in favor of using s_xattr directly. > The reason that fixup happens in a separate pass from everything else > today is that when I wrote posix_acl_fix_xattr_to_user and > posix_acl_xattr_from_user a number of filesystems had a very strange > structure that completely bypassed any code conversion routines and > made some strange assumptions. I don't remember which filesystems those > were but it was neither lustre, nor cifs, nor nfs that were the problem > cases when I was looking. I don't see your patch addressing that > problem so either someone has already fixed those issues or they have > been overlooked. There have been cleanups in this area before, so I assume things were fixed. In any case, I didn't blindly hack this together, I've actually checked all the filesystems. > There is a complication that is comming shortly (next merge window > unless major unfixable bugs show up between now and them). There is a > new field s_user_ns that is being introduced for filesystems to record > their owner and their default translation rules to kuids. That will > make the hard coded &init_user_ns values in your patch wrong. Okay, that conflict should be easy enough to resolve. Where's that code? >>> Please Cc the user namespace maintainers before. Thank you! >> >> Eric, Andy, anyone else? > > Serge Hallyn has a pending patch that adds a similar translation to > the security.capability xattr. Which is one more case of where caching > and translation at the VFS layer are makes sense. No. Just like with this patch, that mapping really needs to go into the appropriate xattr handler. Again, where's that code, please? Thanks, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Andreas Gruenbacher <agruenba@redhat.com> writes: > On Thu, May 26, 2016 at 1:30 AM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> Andreas Gruenbacher <agruenba@redhat.com> writes: >> >>> On Tue, May 24, 2016 at 5:41 PM, Djalal Harouni <tixxdz@gmail.com> wrote: >>>> On Mon, May 23, 2016 at 03:09:49PM +0200, Andreas Gruenbacher 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. >>>> >>>> Please, are you sure that the changes in posix_acl_xattr_get() and >>>> posix_acl_xattr_set() are safe ? you are reading into current user >>>> namespace, from a first view this is not safe unless I'm missing >>>> something... they should map into init_user_ns... >>> >>> Yes, moving the namespace conversion from the VFS into those functions >>> so that we don't have to check for those attributes and parse them >>> twice is exactly the point of this patch. >> >> In general I am in favor of cleaning up the xattr and acl code in the >> kernel. However I am not certain that your changes succeed in getting >> us where we want to go. >> >> My feel is that what we want to do is to update the cached acl when we >> write it from userspace, to update the disk with the new acl when the >> inode is sync'd to disk, and let the vfs handle the translation from >> the cached posix acl to the vfs getxattr and setxattr system calls. >> In the long term anything else is madness. >> >> Currently posix acl reads and updates bypass the vfs acl cache for the >> inode and that just looks wrong. > > Not all filesystems cache acls in the inode (i_acl and i_default_acl), so there > has to be some way to "bypass the cache" if you want to put it that way. > > All normal filesystems that cache ACLs locally implement the get_acl and > set_acl inode operations. They put posix_acl_access_xattr_handler > and posix_acl_default_xattr_handler into s_xattr and use the > generic_{get,set,remove}xattr inode operations to hook things up > appropriately. The xattr handlers convert to/from xattrs and struct posix_acl > and call the get_acl and set_acl inode operations. That's where the namespace > conversion is supposed to happen as well; this avoids special-casing > it in the VFS. The acl support does look like what I would expect this code to look like. Unfortunately some fileystems still (as of 4.6) write xattrs directly to disk. > The only filesystems that don't do things that way are CIFS and Lustre. Doing > the appropriate namespace mapping in CIFS is easy (see this patch). So it's only > Lustre that's left with the original in-place xattr conversion. You know I really wish this was the case. And perhaps I need to look to see what has just been merged in the merge window. There are clearly some xattr changes post 4.6 that I am not seeing. As of 4.6.0 writing xattrs from a user namespace is broken by this change. > There have been cleanups in this area before, so I assume things were > fixed. In any case, I didn't blindly hack this together, I've actually checked > all the filesystems. You know that scares me. Because I just started looking and the first oddball case I checked was broken. > >> There is a complication that is comming shortly (next merge window >> unless major unfixable bugs show up between now and them). There is a >> new field s_user_ns that is being introduced for filesystems to record >> their owner and their default translation rules to kuids. That will >> make the hard coded &init_user_ns values in your patch wrong. > > Okay, that conflict should be easy enough to resolve. Where's that code? > >>>> Please Cc the user namespace maintainers before. Thank you! >>> >>> Eric, Andy, anyone else? >> >> Serge Hallyn has a pending patch that adds a similar translation to >> the security.capability xattr. Which is one more case of where caching >> and translation at the VFS layer are makes sense. > > No. Just like with this patch, that mapping really needs to go into the > appropriate xattr handler. Again, where's that code, please? I will start looking at what is in Linus's tree. But unless it happens to address my concerns, I am not even going to consider the notion that things should be in an ``xattr handler''. There are very good reasons why that conversion does not, nor should not happen inside of filesystem specific code. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
ebiederm@xmission.com (Eric W. Biederman) writes: >> >> No. Just like with this patch, that mapping really needs to go into the >> appropriate xattr handler. Again, where's that code, please? > > I will start looking at what is in Linus's tree. But unless it happens > to address my concerns, I am not even going to consider the notion that > things should be in an ``xattr handler''. > > There are very good reasons why that conversion does not, nor should not > happen inside of filesystem specific code. Ok. I have looked. The cleanups in Linus' tree seem worthwhile. The notion that mapping needs to go into a non-generic xattr handler is broken. If everything can always use a single set of acl handlers for posix acls then I will be happy to consider something like your patch. Fundamentally for vfs and security module supported attributes what a user writes in setxattr needs to be disconnected from what actually gets written to disk. Things evolve and userspace breaks if we don't handle the compatibility between old and new in the kernel. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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@redhat.com> --- 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? 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