| Message ID | 20200221173722.538788-1-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: move the posix_acl_fix_xattr_{to_from}_user out of xattr code | expand |
Miklos, On Fri, Feb 21, 2020 at 7:01 PM Christoph Hellwig <hch@lst.de> wrote: > There is no excuse to ever perform actions related to a specific handler > directly from the generic xattr code as we have handler that understand > the specific data in given attrs. As a nice sideeffect this removes > tons of pointless boilerplate code. > > Signed-off-by: Christoph Hellwig <hch@lst.de> can you please review this change from an overlayfs point of view? Thanks, Andreas > --- > fs/posix_acl.c | 62 ++------------------------------- > fs/xattr.c | 8 +---- > include/linux/posix_acl_xattr.h | 12 ------- > 3 files changed, 3 insertions(+), 79 deletions(-) > > diff --git a/fs/posix_acl.c b/fs/posix_acl.c > index 249672bf54fe..09f1b7d186f0 100644 > --- a/fs/posix_acl.c > +++ b/fs/posix_acl.c > @@ -663,64 +663,6 @@ int posix_acl_update_mode(struct inode *inode, umode_t *mode_p, > } > EXPORT_SYMBOL(posix_acl_update_mode); > > -/* > - * 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) > -{ > - struct posix_acl_xattr_header *header = value; > - struct posix_acl_xattr_entry *entry = (void *)(header + 1), *end; > - int count; > - kuid_t uid; > - kgid_t gid; > - > - if (!value) > - return; > - if (size < sizeof(struct 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. > */ > @@ -851,7 +793,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; > @@ -889,7 +831,7 @@ posix_acl_xattr_set(const struct xattr_handler *handler, > int ret; > > 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 90dd78f0eb27..c31e9a9ea172 100644 > --- a/fs/xattr.c > +++ b/fs/xattr.c > @@ -437,10 +437,7 @@ 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); > - else if (strcmp(kname, XATTR_NAME_CAPS) == 0) { > + if (strcmp(kname, XATTR_NAME_CAPS) == 0) { > error = cap_convert_nscap(d, &kvalue, size); > if (error < 0) > goto out; > @@ -537,9 +534,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, error); > 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 2387709991b5..8f5e70a1bd05 100644 > --- a/include/linux/posix_acl_xattr.h > +++ b/include/linux/posix_acl_xattr.h > @@ -32,18 +32,6 @@ posix_acl_xattr_count(size_t size) > return size / sizeof(struct 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.24.1 >
On Tue, Mar 03, 2020 at 02:42:50PM +0100, Andreas Gruenbacher wrote: > Miklos, > > On Fri, Feb 21, 2020 at 7:01 PM Christoph Hellwig <hch@lst.de> wrote: > > There is no excuse to ever perform actions related to a specific handler > > directly from the generic xattr code as we have handler that understand > > the specific data in given attrs. As a nice sideeffect this removes > > tons of pointless boilerplate code. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > can you please review this change from an overlayfs point of view? ping?
On Tue, Mar 17, 2020 at 3:57 PM Christoph Hellwig <hch@lst.de> wrote: > > On Tue, Mar 03, 2020 at 02:42:50PM +0100, Andreas Gruenbacher wrote: > > Miklos, > > > > On Fri, Feb 21, 2020 at 7:01 PM Christoph Hellwig <hch@lst.de> wrote: > > > There is no excuse to ever perform actions related to a specific handler > > > directly from the generic xattr code as we have handler that understand > > > the specific data in given attrs. As a nice sideeffect this removes > > > tons of pointless boilerplate code. > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > > can you please review this change from an overlayfs point of view? > > ping? To me it looks like these need fixup: fs/overlayfs/dir.c: - err = posix_acl_to_xattr(&init_user_ns, acl, buffer, size); + err = posix_acl_to_xattr(current_user_ns(), acl, buffer, size); fs/overlayfs/super.c: - acl = posix_acl_from_xattr(&init_user_ns, value, size); + acl = posix_acl_from_xattr(current_user_ns(), value, size); Thanks, Miklos
diff --git a/fs/posix_acl.c b/fs/posix_acl.c index 249672bf54fe..09f1b7d186f0 100644 --- a/fs/posix_acl.c +++ b/fs/posix_acl.c @@ -663,64 +663,6 @@ int posix_acl_update_mode(struct inode *inode, umode_t *mode_p, } EXPORT_SYMBOL(posix_acl_update_mode); -/* - * 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) -{ - struct posix_acl_xattr_header *header = value; - struct posix_acl_xattr_entry *entry = (void *)(header + 1), *end; - int count; - kuid_t uid; - kgid_t gid; - - if (!value) - return; - if (size < sizeof(struct 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. */ @@ -851,7 +793,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; @@ -889,7 +831,7 @@ posix_acl_xattr_set(const struct xattr_handler *handler, int ret; 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 90dd78f0eb27..c31e9a9ea172 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -437,10 +437,7 @@ 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); - else if (strcmp(kname, XATTR_NAME_CAPS) == 0) { + if (strcmp(kname, XATTR_NAME_CAPS) == 0) { error = cap_convert_nscap(d, &kvalue, size); if (error < 0) goto out; @@ -537,9 +534,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, error); 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 2387709991b5..8f5e70a1bd05 100644 --- a/include/linux/posix_acl_xattr.h +++ b/include/linux/posix_acl_xattr.h @@ -32,18 +32,6 @@ posix_acl_xattr_count(size_t size) return size / sizeof(struct 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,
There is no excuse to ever perform actions related to a specific handler directly from the generic xattr code as we have handler that understand the specific data in given attrs. As a nice sideeffect this removes tons of pointless boilerplate code. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/posix_acl.c | 62 ++------------------------------- fs/xattr.c | 8 +---- include/linux/posix_acl_xattr.h | 12 ------- 3 files changed, 3 insertions(+), 79 deletions(-)