| Message ID | 20200218143411.2389182-16-christian.brauner@ubuntu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | user_namespace: introduce fsid mappings | expand |
On Tue, Feb 18, 2020 at 03:34:01PM +0100, Christian Brauner wrote: > diff --git a/fs/posix_acl.c b/fs/posix_acl.c > index 249672bf54fe..ed6112c9b804 100644 > --- a/fs/posix_acl.c > +++ b/fs/posix_acl.c > @@ -22,6 +22,7 @@ > #include <linux/xattr.h> > #include <linux/export.h> > #include <linux/user_namespace.h> > +#include <linux/fsuidgid.h> > > static struct posix_acl **acl_by_type(struct inode *inode, int type) > { > @@ -692,12 +693,12 @@ static void posix_acl_fix_xattr_userns( > 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)); > + uid = make_kfsuid(from, le32_to_cpu(entry->e_id)); > + entry->e_id = cpu_to_le32(from_kfsuid(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)); > + gid = make_kfsgid(from, le32_to_cpu(entry->e_id)); > + entry->e_id = cpu_to_le32(from_kfsgid(to, gid)); > break; Before we touch this code any more it needs to move to the right place. Poking into ACLs from generic xattr code is a complete layering violation, and all this needs to be moved so that it is called by the actual handlers called from the file systems.
On Tue, Feb 18, 2020 at 02:26:31PM -0800, Christoph Hellwig wrote: > On Tue, Feb 18, 2020 at 03:34:01PM +0100, Christian Brauner wrote: > > diff --git a/fs/posix_acl.c b/fs/posix_acl.c > > index 249672bf54fe..ed6112c9b804 100644 > > --- a/fs/posix_acl.c > > +++ b/fs/posix_acl.c > > @@ -22,6 +22,7 @@ > > #include <linux/xattr.h> > > #include <linux/export.h> > > #include <linux/user_namespace.h> > > +#include <linux/fsuidgid.h> > > > > static struct posix_acl **acl_by_type(struct inode *inode, int type) > > { > > @@ -692,12 +693,12 @@ static void posix_acl_fix_xattr_userns( > > 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)); > > + uid = make_kfsuid(from, le32_to_cpu(entry->e_id)); > > + entry->e_id = cpu_to_le32(from_kfsuid(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)); > > + gid = make_kfsgid(from, le32_to_cpu(entry->e_id)); > > + entry->e_id = cpu_to_le32(from_kfsgid(to, gid)); > > break; > > Before we touch this code any more it needs to move to the right place. > Poking into ACLs from generic xattr code is a complete layering git history shows that it was deliberately placed after the fs specific xattr handlers have been called so individual filesystems don't need to be aware of id mappings to make maintenance easier. Same goes for vfs capabilities. Moving this down into individual filesystem seems like a maintenance nightmare where now each individual filesystem will have to remember to fixup their ids. For namespaced vfs caps which are handled at the same level in setxattr() it will also mean breaking backwards compatible translation from non-namespaced vfs caps aware userspace to namespaced vfs-caps aware kernels.
diff --git a/fs/posix_acl.c b/fs/posix_acl.c index 249672bf54fe..ed6112c9b804 100644 --- a/fs/posix_acl.c +++ b/fs/posix_acl.c @@ -22,6 +22,7 @@ #include <linux/xattr.h> #include <linux/export.h> #include <linux/user_namespace.h> +#include <linux/fsuidgid.h> static struct posix_acl **acl_by_type(struct inode *inode, int type) { @@ -692,12 +693,12 @@ static void posix_acl_fix_xattr_userns( 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)); + uid = make_kfsuid(from, le32_to_cpu(entry->e_id)); + entry->e_id = cpu_to_le32(from_kfsuid(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)); + gid = make_kfsgid(from, le32_to_cpu(entry->e_id)); + entry->e_id = cpu_to_le32(from_kfsgid(to, gid)); break; default: break; @@ -765,14 +766,14 @@ posix_acl_from_xattr(struct user_namespace *user_ns, case ACL_USER: acl_e->e_uid = - make_kuid(user_ns, + make_kfsuid(user_ns, le32_to_cpu(entry->e_id)); if (!uid_valid(acl_e->e_uid)) goto fail; break; case ACL_GROUP: acl_e->e_gid = - make_kgid(user_ns, + make_kfsgid(user_ns, le32_to_cpu(entry->e_id)); if (!gid_valid(acl_e->e_gid)) goto fail; @@ -817,11 +818,11 @@ posix_acl_to_xattr(struct user_namespace *user_ns, const struct posix_acl *acl, switch(acl_e->e_tag) { case ACL_USER: ext_entry->e_id = - cpu_to_le32(from_kuid(user_ns, acl_e->e_uid)); + cpu_to_le32(from_kfsuid(user_ns, acl_e->e_uid)); break; case ACL_GROUP: ext_entry->e_id = - cpu_to_le32(from_kgid(user_ns, acl_e->e_gid)); + cpu_to_le32(from_kfsgid(user_ns, acl_e->e_gid)); break; default: ext_entry->e_id = cpu_to_le32(ACL_UNDEFINED_ID);
Switch posix_acls() to lookup fsids in the fsid mappings. If no fsid mappings are setup the behavior is unchanged, i.e. fsids are looked up in the id mappings. Afaict, all filesystems that share a superblock in all user namespaces currently do not support acls so this change should be safe to do unconditionally. Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> --- /* v2 */ unchanged /* v3 */ unchanged --- fs/posix_acl.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)