* [RFC PATCH v2] hostfs: handle idmapped mounts @ 2023-03-01 1:50 Glenn Washburn 2023-03-02 8:39 ` Christian Brauner 0 siblings, 1 reply; 6+ messages in thread From: Glenn Washburn @ 2023-03-01 1:50 UTC (permalink / raw) To: Richard Weinberger, Anton Ivanov, Johannes Berg, linux-um Cc: Glenn Washburn, linux-kernel, Christian Brauner, Seth Forshee, linux-fsdevel Let hostfs handle idmapped mounts. This allows to have the same hostfs mount appear in multiple locations with different id mappings. root@(none):/media# id uid=0(root) gid=0(root) groups=0(root) root@(none):/media# mkdir mnt idmapped root@(none):/media# mount -thostfs -o/home/user hostfs mnt root@(none):/media# touch mnt/aaa root@(none):/media# mount-idmapped --map-mount u:`id -u user`:0:1 --map-mount g:`id -g user`:0:1 /media/mnt /media/idmapped root@(none):/media# ls -l mnt/aaa idmapped/aaa -rw-r--r-- 1 root root 0 Jan 28 01:23 idmapped/aaa -rw-r--r-- 1 user user 0 Jan 28 01:23 mnt/aaa root@(none):/media# touch idmapped/bbb root@(none):/media# ls -l mnt/bbb idmapped/bbb -rw-r--r-- 1 root root 0 Jan 28 01:26 idmapped/bbb -rw-r--r-- 1 user user 0 Jan 28 01:26 mnt/bbb Signed-off-by: Glenn Washburn <development@efficientek.com> --- Changes from v1: * Rebase on to tip. The above commands work and have the results expected. The __vfsuid_val(make_vfsuid(...)) seems ugly to get the uid_t, but it seemed like the best one I've come across. Is there a better way? Glenn --- fs/hostfs/hostfs_kern.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c index c18bb50c31b6..9459da99a0db 100644 --- a/fs/hostfs/hostfs_kern.c +++ b/fs/hostfs/hostfs_kern.c @@ -786,7 +786,7 @@ static int hostfs_permission(struct mnt_idmap *idmap, err = access_file(name, r, w, x); __putname(name); if (!err) - err = generic_permission(&nop_mnt_idmap, ino, desired); + err = generic_permission(idmap, ino, desired); return err; } @@ -794,13 +794,14 @@ static int hostfs_setattr(struct mnt_idmap *idmap, struct dentry *dentry, struct iattr *attr) { struct inode *inode = d_inode(dentry); + struct user_namespace *fs_userns = i_user_ns(inode); struct hostfs_iattr attrs; char *name; int err; int fd = HOSTFS_I(inode)->fd; - err = setattr_prepare(&nop_mnt_idmap, dentry, attr); + err = setattr_prepare(idmap, dentry, attr); if (err) return err; @@ -814,11 +815,11 @@ static int hostfs_setattr(struct mnt_idmap *idmap, } if (attr->ia_valid & ATTR_UID) { attrs.ia_valid |= HOSTFS_ATTR_UID; - attrs.ia_uid = from_kuid(&init_user_ns, attr->ia_uid); + attrs.ia_uid = __vfsuid_val(make_vfsuid(idmap, fs_userns, attr->ia_uid)); } if (attr->ia_valid & ATTR_GID) { attrs.ia_valid |= HOSTFS_ATTR_GID; - attrs.ia_gid = from_kgid(&init_user_ns, attr->ia_gid); + attrs.ia_gid = __vfsgid_val(make_vfsgid(idmap, fs_userns, attr->ia_gid)); } if (attr->ia_valid & ATTR_SIZE) { attrs.ia_valid |= HOSTFS_ATTR_SIZE; @@ -857,7 +858,7 @@ static int hostfs_setattr(struct mnt_idmap *idmap, attr->ia_size != i_size_read(inode)) truncate_setsize(inode, attr->ia_size); - setattr_copy(&nop_mnt_idmap, inode, attr); + setattr_copy(idmap, inode, attr); mark_inode_dirty(inode); return 0; } @@ -991,7 +992,7 @@ static struct file_system_type hostfs_type = { .name = "hostfs", .mount = hostfs_read_sb, .kill_sb = hostfs_kill_sb, - .fs_flags = 0, + .fs_flags = FS_ALLOW_IDMAP, }; MODULE_ALIAS_FS("hostfs"); base-commit: 7fa08de735e41001a70c8ca869b2b159d74c2339 -- 2.30.2 _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v2] hostfs: handle idmapped mounts 2023-03-01 1:50 [RFC PATCH v2] hostfs: handle idmapped mounts Glenn Washburn @ 2023-03-02 8:39 ` Christian Brauner 2023-03-04 6:28 ` Glenn Washburn 0 siblings, 1 reply; 6+ messages in thread From: Christian Brauner @ 2023-03-02 8:39 UTC (permalink / raw) To: Glenn Washburn Cc: Richard Weinberger, Anton Ivanov, Johannes Berg, linux-um, linux-kernel, Seth Forshee, linux-fsdevel On Tue, Feb 28, 2023 at 07:50:02PM -0600, Glenn Washburn wrote: > Let hostfs handle idmapped mounts. This allows to have the same hostfs > mount appear in multiple locations with different id mappings. > > root@(none):/media# id > uid=0(root) gid=0(root) groups=0(root) > root@(none):/media# mkdir mnt idmapped > root@(none):/media# mount -thostfs -o/home/user hostfs mnt > > root@(none):/media# touch mnt/aaa > root@(none):/media# mount-idmapped --map-mount u:`id -u user`:0:1 --map-mount g:`id -g user`:0:1 /media/mnt /media/idmapped > root@(none):/media# ls -l mnt/aaa idmapped/aaa > -rw-r--r-- 1 root root 0 Jan 28 01:23 idmapped/aaa > -rw-r--r-- 1 user user 0 Jan 28 01:23 mnt/aaa > > root@(none):/media# touch idmapped/bbb > root@(none):/media# ls -l mnt/bbb idmapped/bbb > -rw-r--r-- 1 root root 0 Jan 28 01:26 idmapped/bbb > -rw-r--r-- 1 user user 0 Jan 28 01:26 mnt/bbb > > Signed-off-by: Glenn Washburn <development@efficientek.com> > --- > Changes from v1: > * Rebase on to tip. The above commands work and have the results expected. > The __vfsuid_val(make_vfsuid(...)) seems ugly to get the uid_t, but it > seemed like the best one I've come across. Is there a better way? Sure, I can help you with that. ;) > > Glenn > --- > fs/hostfs/hostfs_kern.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c > index c18bb50c31b6..9459da99a0db 100644 > --- a/fs/hostfs/hostfs_kern.c > +++ b/fs/hostfs/hostfs_kern.c > @@ -786,7 +786,7 @@ static int hostfs_permission(struct mnt_idmap *idmap, > err = access_file(name, r, w, x); > __putname(name); > if (!err) > - err = generic_permission(&nop_mnt_idmap, ino, desired); > + err = generic_permission(idmap, ino, desired); > return err; > } > > @@ -794,13 +794,14 @@ static int hostfs_setattr(struct mnt_idmap *idmap, > struct dentry *dentry, struct iattr *attr) > { > struct inode *inode = d_inode(dentry); > + struct user_namespace *fs_userns = i_user_ns(inode); Fyi, since hostfs can't be mounted in a user namespace fs_userns == &init_user_ns so it doesn't really matter what you use. > struct hostfs_iattr attrs; > char *name; > int err; > > int fd = HOSTFS_I(inode)->fd; > > - err = setattr_prepare(&nop_mnt_idmap, dentry, attr); > + err = setattr_prepare(idmap, dentry, attr); > if (err) > return err; > > @@ -814,11 +815,11 @@ static int hostfs_setattr(struct mnt_idmap *idmap, > } > if (attr->ia_valid & ATTR_UID) { > attrs.ia_valid |= HOSTFS_ATTR_UID; > - attrs.ia_uid = from_kuid(&init_user_ns, attr->ia_uid); > + attrs.ia_uid = __vfsuid_val(make_vfsuid(idmap, fs_userns, attr->ia_uid)); > } > if (attr->ia_valid & ATTR_GID) { > attrs.ia_valid |= HOSTFS_ATTR_GID; > - attrs.ia_gid = from_kgid(&init_user_ns, attr->ia_gid); > + attrs.ia_gid = __vfsgid_val(make_vfsgid(idmap, fs_userns, attr->ia_gid)); Heh, if you look include/linux/fs.h: /* * The two anonymous unions wrap structures with the same member. * * Filesystems raising FS_ALLOW_IDMAP need to use ia_vfs{g,u}id which * are a dedicated type requiring the filesystem to use the dedicated * helpers. Other filesystem can continue to use ia_{g,u}id until they * have been ported. * * They always contain the same value. In other words FS_ALLOW_IDMAP * pass down the same value on idmapped mounts as they would on regular * mounts. */ union { kuid_t ia_uid; vfsuid_t ia_vfsuid; }; union { kgid_t ia_gid; vfsgid_t ia_vfsgid; }; this just is: attrs.ia_uid = from_vfsuid(idmap, fs_userns, attr->ia_vfsuid)); attrs.ia_gid = from_vfsgid(idmap, fs_userns, attr->ia_vfsgid)); (I plan to fully replace ia_{g,u}id at some point.) Christian _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v2] hostfs: handle idmapped mounts 2023-03-02 8:39 ` Christian Brauner @ 2023-03-04 6:28 ` Glenn Washburn 2023-03-04 12:01 ` Christian Brauner 0 siblings, 1 reply; 6+ messages in thread From: Glenn Washburn @ 2023-03-04 6:28 UTC (permalink / raw) To: Christian Brauner Cc: Richard Weinberger, Anton Ivanov, Johannes Berg, linux-um, linux-kernel, Seth Forshee, linux-fsdevel On Thu, 2 Mar 2023 09:39:28 +0100 Christian Brauner <brauner@kernel.org> wrote: > On Tue, Feb 28, 2023 at 07:50:02PM -0600, Glenn Washburn wrote: > > Let hostfs handle idmapped mounts. This allows to have the same > > hostfs mount appear in multiple locations with different id > > mappings. > > > > root@(none):/media# id > > uid=0(root) gid=0(root) groups=0(root) > > root@(none):/media# mkdir mnt idmapped > > root@(none):/media# mount -thostfs -o/home/user hostfs mnt > > > > root@(none):/media# touch mnt/aaa > > root@(none):/media# mount-idmapped --map-mount u:`id -u user`:0:1 > > --map-mount g:`id -g user`:0:1 /media/mnt /media/idmapped > > root@(none):/media# ls -l mnt/aaa idmapped/aaa -rw-r--r-- 1 root > > root 0 Jan 28 01:23 idmapped/aaa -rw-r--r-- 1 user user 0 Jan 28 > > 01:23 mnt/aaa > > > > root@(none):/media# touch idmapped/bbb > > root@(none):/media# ls -l mnt/bbb idmapped/bbb > > -rw-r--r-- 1 root root 0 Jan 28 01:26 idmapped/bbb > > -rw-r--r-- 1 user user 0 Jan 28 01:26 mnt/bbb > > > > Signed-off-by: Glenn Washburn <development@efficientek.com> > > --- > > Changes from v1: > > * Rebase on to tip. The above commands work and have the results > > expected. The __vfsuid_val(make_vfsuid(...)) seems ugly to get the > > uid_t, but it seemed like the best one I've come across. Is there a > > better way? > > Sure, I can help you with that. ;) Thank you! > > > > Glenn > > --- > > fs/hostfs/hostfs_kern.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c > > index c18bb50c31b6..9459da99a0db 100644 > > --- a/fs/hostfs/hostfs_kern.c > > +++ b/fs/hostfs/hostfs_kern.c > > @@ -786,7 +786,7 @@ static int hostfs_permission(struct mnt_idmap > > *idmap, err = access_file(name, r, w, x); > > __putname(name); > > if (!err) > > - err = generic_permission(&nop_mnt_idmap, ino, > > desired); > > + err = generic_permission(idmap, ino, desired); > > return err; > > } > > > > @@ -794,13 +794,14 @@ static int hostfs_setattr(struct mnt_idmap > > *idmap, struct dentry *dentry, struct iattr *attr) > > { > > struct inode *inode = d_inode(dentry); > > + struct user_namespace *fs_userns = i_user_ns(inode); > > Fyi, since hostfs can't be mounted in a user namespace > fs_userns == &init_user_ns > so it doesn't really matter what you use. What would you suggest as preferable? > > struct hostfs_iattr attrs; > > char *name; > > int err; > > > > int fd = HOSTFS_I(inode)->fd; > > > > - err = setattr_prepare(&nop_mnt_idmap, dentry, attr); > > + err = setattr_prepare(idmap, dentry, attr); > > if (err) > > return err; > > > > @@ -814,11 +815,11 @@ static int hostfs_setattr(struct mnt_idmap > > *idmap, } > > if (attr->ia_valid & ATTR_UID) { > > attrs.ia_valid |= HOSTFS_ATTR_UID; > > - attrs.ia_uid = from_kuid(&init_user_ns, > > attr->ia_uid); > > + attrs.ia_uid = __vfsuid_val(make_vfsuid(idmap, > > fs_userns, attr->ia_uid)); } > > if (attr->ia_valid & ATTR_GID) { > > attrs.ia_valid |= HOSTFS_ATTR_GID; > > - attrs.ia_gid = from_kgid(&init_user_ns, > > attr->ia_gid); > > + attrs.ia_gid = __vfsgid_val(make_vfsgid(idmap, > > fs_userns, attr->ia_gid)); > > Heh, if you look include/linux/fs.h: > > /* > * The two anonymous unions wrap structures with the same > member. * > * Filesystems raising FS_ALLOW_IDMAP need to use > ia_vfs{g,u}id which > * are a dedicated type requiring the filesystem to use the > dedicated > * helpers. Other filesystem can continue to use ia_{g,u}id > until they > * have been ported. > * > * They always contain the same value. In other words > FS_ALLOW_IDMAP > * pass down the same value on idmapped mounts as they would > on regular > * mounts. > */ > union { > kuid_t ia_uid; > vfsuid_t ia_vfsuid; > }; > union { > kgid_t ia_gid; > vfsgid_t ia_vfsgid; > }; > > this just is: > > attrs.ia_uid = from_vfsuid(idmap, fs_userns, attr->ia_vfsuid)); > attrs.ia_gid = from_vfsgid(idmap, fs_userns, attr->ia_vfsgid)); Its easy to miss from this patch because of lack of context, but attrs is a struct hostfs_iattr, not struct iattr. And attrs.ia_uid is of type uid_t, not kuid_t. So the above fails to compile. This is why I needed to wrap make_vfsuid() in __vfsuid_val() (to get the uid_t). I had decided against using from_vfsuid() because then I thought I'd need to use from_kuid() to get the uid_t. And from_kuid() takes the namespace (again), which seemed uglier. Based on this, what do you suggest? Glenn _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v2] hostfs: handle idmapped mounts 2023-03-04 6:28 ` Glenn Washburn @ 2023-03-04 12:01 ` Christian Brauner 2023-03-16 6:20 ` Glenn Washburn 0 siblings, 1 reply; 6+ messages in thread From: Christian Brauner @ 2023-03-04 12:01 UTC (permalink / raw) To: Glenn Washburn Cc: Richard Weinberger, Anton Ivanov, Johannes Berg, linux-um, linux-kernel, Seth Forshee, linux-fsdevel On Sat, Mar 04, 2023 at 12:28:46AM -0600, Glenn Washburn wrote: > On Thu, 2 Mar 2023 09:39:28 +0100 > Christian Brauner <brauner@kernel.org> wrote: > > > On Tue, Feb 28, 2023 at 07:50:02PM -0600, Glenn Washburn wrote: > > > Let hostfs handle idmapped mounts. This allows to have the same > > > hostfs mount appear in multiple locations with different id > > > mappings. > > > > > > root@(none):/media# id > > > uid=0(root) gid=0(root) groups=0(root) > > > root@(none):/media# mkdir mnt idmapped > > > root@(none):/media# mount -thostfs -o/home/user hostfs mnt > > > > > > root@(none):/media# touch mnt/aaa > > > root@(none):/media# mount-idmapped --map-mount u:`id -u user`:0:1 > > > --map-mount g:`id -g user`:0:1 /media/mnt /media/idmapped > > > root@(none):/media# ls -l mnt/aaa idmapped/aaa -rw-r--r-- 1 root > > > root 0 Jan 28 01:23 idmapped/aaa -rw-r--r-- 1 user user 0 Jan 28 > > > 01:23 mnt/aaa > > > > > > root@(none):/media# touch idmapped/bbb > > > root@(none):/media# ls -l mnt/bbb idmapped/bbb > > > -rw-r--r-- 1 root root 0 Jan 28 01:26 idmapped/bbb > > > -rw-r--r-- 1 user user 0 Jan 28 01:26 mnt/bbb > > > > > > Signed-off-by: Glenn Washburn <development@efficientek.com> > > > --- > > > Changes from v1: > > > * Rebase on to tip. The above commands work and have the results > > > expected. The __vfsuid_val(make_vfsuid(...)) seems ugly to get the > > > uid_t, but it seemed like the best one I've come across. Is there a > > > better way? > > > > Sure, I can help you with that. ;) > > Thank you! > > > > > > > Glenn > > > --- > > > fs/hostfs/hostfs_kern.c | 13 +++++++------ > > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > > > diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c > > > index c18bb50c31b6..9459da99a0db 100644 > > > --- a/fs/hostfs/hostfs_kern.c > > > +++ b/fs/hostfs/hostfs_kern.c > > > @@ -786,7 +786,7 @@ static int hostfs_permission(struct mnt_idmap > > > *idmap, err = access_file(name, r, w, x); > > > __putname(name); > > > if (!err) > > > - err = generic_permission(&nop_mnt_idmap, ino, > > > desired); > > > + err = generic_permission(idmap, ino, desired); > > > return err; > > > } > > > > > > @@ -794,13 +794,14 @@ static int hostfs_setattr(struct mnt_idmap > > > *idmap, struct dentry *dentry, struct iattr *attr) > > > { > > > struct inode *inode = d_inode(dentry); > > > + struct user_namespace *fs_userns = i_user_ns(inode); > > > > Fyi, since hostfs can't be mounted in a user namespace > > fs_userns == &init_user_ns > > so it doesn't really matter what you use. > > What would you suggest as preferable? I would leave init_user_ns hardcoded as it clearly indicates that hostfs can only be mounted in the initial user namespace. Plus, the patch is smaller. > > > > struct hostfs_iattr attrs; > > > char *name; > > > int err; > > > > > > int fd = HOSTFS_I(inode)->fd; > > > > > > - err = setattr_prepare(&nop_mnt_idmap, dentry, attr); > > > + err = setattr_prepare(idmap, dentry, attr); > > > if (err) > > > return err; > > > > > > @@ -814,11 +815,11 @@ static int hostfs_setattr(struct mnt_idmap > > > *idmap, } > > > if (attr->ia_valid & ATTR_UID) { > > > attrs.ia_valid |= HOSTFS_ATTR_UID; > > > - attrs.ia_uid = from_kuid(&init_user_ns, > > > attr->ia_uid); > > > + attrs.ia_uid = __vfsuid_val(make_vfsuid(idmap, > > > fs_userns, attr->ia_uid)); } > > > if (attr->ia_valid & ATTR_GID) { > > > attrs.ia_valid |= HOSTFS_ATTR_GID; > > > - attrs.ia_gid = from_kgid(&init_user_ns, > > > attr->ia_gid); > > > + attrs.ia_gid = __vfsgid_val(make_vfsgid(idmap, > > > fs_userns, attr->ia_gid)); > > > > Heh, if you look include/linux/fs.h: > > > > /* > > * The two anonymous unions wrap structures with the same > > member. * > > * Filesystems raising FS_ALLOW_IDMAP need to use > > ia_vfs{g,u}id which > > * are a dedicated type requiring the filesystem to use the > > dedicated > > * helpers. Other filesystem can continue to use ia_{g,u}id > > until they > > * have been ported. > > * > > * They always contain the same value. In other words > > FS_ALLOW_IDMAP > > * pass down the same value on idmapped mounts as they would > > on regular > > * mounts. > > */ > > union { > > kuid_t ia_uid; > > vfsuid_t ia_vfsuid; > > }; > > union { > > kgid_t ia_gid; > > vfsgid_t ia_vfsgid; > > }; > > > > this just is: > > > > attrs.ia_uid = from_vfsuid(idmap, fs_userns, attr->ia_vfsuid)); > > attrs.ia_gid = from_vfsgid(idmap, fs_userns, attr->ia_vfsgid)); > > Its easy to miss from this patch because of lack of context, but attrs > is a struct hostfs_iattr, not struct iattr. And attrs.ia_uid is of type > uid_t, not kuid_t. So the above fails to compile. This is why I needed Oh, I see. And then that raw value is used by calling fchmod()/chmod()/chown()/fchown() and so on. That's rather special. Ok, then I know what to do. > to wrap make_vfsuid() in __vfsuid_val() (to get the uid_t). Right. My point had been - independent of the struct hostfs_iattr issue you thankfully pointed out - that make_vfsuid() is wrong here. make_vfsuid() is used to map a filesystem wide k{g,u}id_t according to the mount's idmapping that operation originated from. But that's done by the vfs way before we're calling into the filesystem. For example, it's done in chown_common(). So the value placed in struct iattr (the VFS struct) is already a vfs{g,u}id stored in iattr->ia_vfs{g,u}id. So you need to use from_vfs{g,u}id() here. > > I had decided against using from_vfsuid() because then I thought I'd > need to use from_kuid() to get the uid_t. And from_kuid() takes the > namespace (again), which seemed uglier. > > Based on this, what do you suggest? Ok, so just some details on the background before I paste what I think we should do. As soon as you support idmapped mounts you at least technically are always dealing with two mappings: (1) First, there's the filesystem wide idmapping which is taken from the namespace the filessytem was mounted in. This idmapping is applied when you read the raw uid/gid value from disk and turn into a kuid_t type. That value is persistent and stored in inode->i_{g,u}id. All things that are cached and that can be accessed from multiple mounts concurrently can only ever cache k{g,u}id_t aka filesystem values. (2) Whenever we're dealing with an operation that's coming from an idmapped mount we need to take the idmapping of the mount into account. That idmapping is completely separate type struct mnt_idmap that's opaque for filesystems and most of the vfs. That idmapping is used to generate the vfs{g,u}id_t. IOW, translates from the filesystem representation to a mount/vfs representation. So, in order to store the correct value on disk we need to invert those two idmappings to arrive at the raw value that we want to store: (U1) from_vfsuid() // map to the filesystem wide value aka something that we can store in inode->i_{g,u}id and that's cacheable. This is done in setattr_copy(). (U2) from_kuid() // map the filesystem wide value to the raw value we want to store on disk For nearly all filesystems these steps almost never need to be performed explicitly. Instead, dedicated vfs helpers will do this: (U1) i_{g,u}id_update() // map to filesystem wide value (U2) i_{g,u}id_read() // map to raw on-disk value For filesystems that don't support being mounted in namespaces the (U2) step is always a nop. So technically there's no difference between: (U2) from_kuid() and __kuid_val(kuid) but it's cleaner to use the helpers even in that case. So given how hostfs works these two steps need to be performed explicitly. So I suggest (untested): diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c index c18bb50c31b6..72b7e1bcc32e 100644 --- a/fs/hostfs/hostfs_kern.c +++ b/fs/hostfs/hostfs_kern.c @@ -813,12 +813,22 @@ static int hostfs_setattr(struct mnt_idmap *idmap, attrs.ia_mode = attr->ia_mode; } if (attr->ia_valid & ATTR_UID) { + kuid_t kuid; + attrs.ia_valid |= HOSTFS_ATTR_UID; - attrs.ia_uid = from_kuid(&init_user_ns, attr->ia_uid); + /* Map the vfs id into the filesystem. */ + kuid = from_vfsuid(idmap, &init_user_ns, attr->ia_vfsuid); + /* Map the filesystem id to its raw on disk value. */ + attrs.ia_uid = from_kuid(&init_user_ns, kuid); } if (attr->ia_valid & ATTR_GID) { + kgid_t kgid; + attrs.ia_valid |= HOSTFS_ATTR_GID; - attrs.ia_gid = from_kgid(&init_user_ns, attr->ia_gid); + /* Map the vfs id into the filesystem. */ + kgid = from_vfsgid(idmap, &init_user_ns, attr->ia_vfsgid); + /* Map the filesystem id to its raw on disk value. */ + attrs.ia_gid = from_kgid(&init_user_ns, kgid); } if (attr->ia_valid & ATTR_SIZE) { attrs.ia_valid |= HOSTFS_ATTR_SIZE; _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v2] hostfs: handle idmapped mounts 2023-03-04 12:01 ` Christian Brauner @ 2023-03-16 6:20 ` Glenn Washburn 2023-03-16 10:37 ` Christian Brauner 0 siblings, 1 reply; 6+ messages in thread From: Glenn Washburn @ 2023-03-16 6:20 UTC (permalink / raw) To: Christian Brauner Cc: Richard Weinberger, Anton Ivanov, Johannes Berg, linux-um, linux-kernel, Seth Forshee, linux-fsdevel On 3/4/23 12:01, Christian Brauner wrote: > On Sat, Mar 04, 2023 at 12:28:46AM -0600, Glenn Washburn wrote: >> On Thu, 2 Mar 2023 09:39:28 +0100 >> Christian Brauner <brauner@kernel.org> wrote: >> >>> On Tue, Feb 28, 2023 at 07:50:02PM -0600, Glenn Washburn wrote: >>>> Let hostfs handle idmapped mounts. This allows to have the same >>>> hostfs mount appear in multiple locations with different id >>>> mappings. >>>> >>>> root@(none):/media# id >>>> uid=0(root) gid=0(root) groups=0(root) >>>> root@(none):/media# mkdir mnt idmapped >>>> root@(none):/media# mount -thostfs -o/home/user hostfs mnt >>>> >>>> root@(none):/media# touch mnt/aaa >>>> root@(none):/media# mount-idmapped --map-mount u:`id -u user`:0:1 >>>> --map-mount g:`id -g user`:0:1 /media/mnt /media/idmapped >>>> root@(none):/media# ls -l mnt/aaa idmapped/aaa -rw-r--r-- 1 root >>>> root 0 Jan 28 01:23 idmapped/aaa -rw-r--r-- 1 user user 0 Jan 28 >>>> 01:23 mnt/aaa >>>> >>>> root@(none):/media# touch idmapped/bbb >>>> root@(none):/media# ls -l mnt/bbb idmapped/bbb >>>> -rw-r--r-- 1 root root 0 Jan 28 01:26 idmapped/bbb >>>> -rw-r--r-- 1 user user 0 Jan 28 01:26 mnt/bbb >>>> >>>> Signed-off-by: Glenn Washburn <development@efficientek.com> >>>> --- >>>> Changes from v1: >>>> * Rebase on to tip. The above commands work and have the results >>>> expected. The __vfsuid_val(make_vfsuid(...)) seems ugly to get the >>>> uid_t, but it seemed like the best one I've come across. Is there a >>>> better way? >>> >>> Sure, I can help you with that. ;) >> >> Thank you! >> >>>> >>>> Glenn >>>> --- >>>> fs/hostfs/hostfs_kern.c | 13 +++++++------ >>>> 1 file changed, 7 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c >>>> index c18bb50c31b6..9459da99a0db 100644 >>>> --- a/fs/hostfs/hostfs_kern.c >>>> +++ b/fs/hostfs/hostfs_kern.c >>>> @@ -786,7 +786,7 @@ static int hostfs_permission(struct mnt_idmap >>>> *idmap, err = access_file(name, r, w, x); >>>> __putname(name); >>>> if (!err) >>>> - err = generic_permission(&nop_mnt_idmap, ino, >>>> desired); >>>> + err = generic_permission(idmap, ino, desired); >>>> return err; >>>> } >>>> >>>> @@ -794,13 +794,14 @@ static int hostfs_setattr(struct mnt_idmap >>>> *idmap, struct dentry *dentry, struct iattr *attr) >>>> { >>>> struct inode *inode = d_inode(dentry); >>>> + struct user_namespace *fs_userns = i_user_ns(inode); >>> >>> Fyi, since hostfs can't be mounted in a user namespace >>> fs_userns == &init_user_ns >>> so it doesn't really matter what you use. >> >> What would you suggest as preferable? > > I would leave init_user_ns hardcoded as it clearly indicates that hostfs > can only be mounted in the initial user namespace. Plus, the patch is > smaller. > >> >>>> struct hostfs_iattr attrs; >>>> char *name; >>>> int err; >>>> >>>> int fd = HOSTFS_I(inode)->fd; >>>> >>>> - err = setattr_prepare(&nop_mnt_idmap, dentry, attr); >>>> + err = setattr_prepare(idmap, dentry, attr); >>>> if (err) >>>> return err; >>>> >>>> @@ -814,11 +815,11 @@ static int hostfs_setattr(struct mnt_idmap >>>> *idmap, } >>>> if (attr->ia_valid & ATTR_UID) { >>>> attrs.ia_valid |= HOSTFS_ATTR_UID; >>>> - attrs.ia_uid = from_kuid(&init_user_ns, >>>> attr->ia_uid); >>>> + attrs.ia_uid = __vfsuid_val(make_vfsuid(idmap, >>>> fs_userns, attr->ia_uid)); } >>>> if (attr->ia_valid & ATTR_GID) { >>>> attrs.ia_valid |= HOSTFS_ATTR_GID; >>>> - attrs.ia_gid = from_kgid(&init_user_ns, >>>> attr->ia_gid); >>>> + attrs.ia_gid = __vfsgid_val(make_vfsgid(idmap, >>>> fs_userns, attr->ia_gid)); >>> >>> Heh, if you look include/linux/fs.h: >>> >>> /* >>> * The two anonymous unions wrap structures with the same >>> member. * >>> * Filesystems raising FS_ALLOW_IDMAP need to use >>> ia_vfs{g,u}id which >>> * are a dedicated type requiring the filesystem to use the >>> dedicated >>> * helpers. Other filesystem can continue to use ia_{g,u}id >>> until they >>> * have been ported. >>> * >>> * They always contain the same value. In other words >>> FS_ALLOW_IDMAP >>> * pass down the same value on idmapped mounts as they would >>> on regular >>> * mounts. >>> */ >>> union { >>> kuid_t ia_uid; >>> vfsuid_t ia_vfsuid; >>> }; >>> union { >>> kgid_t ia_gid; >>> vfsgid_t ia_vfsgid; >>> }; >>> >>> this just is: >>> >>> attrs.ia_uid = from_vfsuid(idmap, fs_userns, attr->ia_vfsuid)); >>> attrs.ia_gid = from_vfsgid(idmap, fs_userns, attr->ia_vfsgid)); >> >> Its easy to miss from this patch because of lack of context, but attrs >> is a struct hostfs_iattr, not struct iattr. And attrs.ia_uid is of type >> uid_t, not kuid_t. So the above fails to compile. This is why I needed > > Oh, I see. And then that raw value is used by calling > fchmod()/chmod()/chown()/fchown() and so on. That's rather special. > Ok, then I know what to do. > >> to wrap make_vfsuid() in __vfsuid_val() (to get the uid_t). > > Right. My point had been - independent of the struct hostfs_iattr issue > you thankfully pointed out - that make_vfsuid() is wrong here. > > make_vfsuid() is used to map a filesystem wide k{g,u}id_t according to > the mount's idmapping that operation originated from. But that's done > by the vfs way before we're calling into the filesystem. For example, > it's done in chown_common(). > > So the value placed in struct iattr (the VFS struct) is already a > vfs{g,u}id stored in iattr->ia_vfs{g,u}id. So you need to use > from_vfs{g,u}id() here. > >> >> I had decided against using from_vfsuid() because then I thought I'd >> need to use from_kuid() to get the uid_t. And from_kuid() takes the >> namespace (again), which seemed uglier. >> >> Based on this, what do you suggest? > > Ok, so just some details on the background before I paste what I think > we should do. > As soon as you support idmapped mounts you at least technically are Thanks for the detailed explanation. I apologize for not getting back to this sooner. > always dealing with two mappings: > > (1) First, there's the filesystem wide idmapping which is taken from the > namespace the filessytem was mounted in. This idmapping is applied > when you read the raw uid/gid value from disk and turn into a kuid_t > type. That value is persistent and stored in inode->i_{g,u}id. All > things that are cached and that can be accessed from multiple mounts > concurrently can only ever cache k{g,u}id_t aka filesystem values. > (2) Whenever we're dealing with an operation that's coming from an > idmapped mount we need to take the idmapping of the mount into > account. That idmapping is completely separate type struct > mnt_idmap that's opaque for filesystems and most of the vfs. > > That idmapping is used to generate the vfs{g,u}id_t. IOW, translates > from the filesystem representation to a mount/vfs representation. > > So, in order to store the correct value on disk we need to invert those > two idmappings to arrive at the raw value that we want to store: > (U1) from_vfsuid() // map to the filesystem wide value aka something > that we can store in inode->i_{g,u}id and that's cacheable. This is > done in setattr_copy(). > (U2) from_kuid() // map the filesystem wide value to the raw value we > want to store on disk It seems to me that there are actually 3 mappings, with the third being (U2) above (ie vfsuid_t -> kuid_t). And that from_vfsuid() does mappings (1) and (2) above. Is this incorrect? Whats confusing to me is that from_vfsuid() takes both an idmap and a user namespace, so presumably it will handle both mapping types (1) and (2). And then there's from_kuid() which takes an idmap, so I thought it might also do a type (2) mapping. But looking at the code it doesn't seem to ever use its idmap parameter. Can you explain the rational behind having from_kuid() take an idmap? Is it legacy that will be cleaned up as this code settles down / stabilizes? Or perhaps its > > For nearly all filesystems these steps almost never need to be performed > explicitly. Instead, dedicated vfs helpers will do this: > > (U1) i_{g,u}id_update() // map to filesystem wide value > (U2) i_{g,u}id_read() // map to raw on-disk value > > For filesystems that don't support being mounted in namespaces the (U2) > step is always a nop. So technically there's no difference between: > > (U2) from_kuid() and __kuid_val(kuid) > > but it's cleaner to use the helpers even in that case. > > So given how hostfs works these two steps need to be performed > explicitly. So I suggest (untested): > > diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c > index c18bb50c31b6..72b7e1bcc32e 100644 > --- a/fs/hostfs/hostfs_kern.c > +++ b/fs/hostfs/hostfs_kern.c > @@ -813,12 +813,22 @@ static int hostfs_setattr(struct mnt_idmap *idmap, > attrs.ia_mode = attr->ia_mode; > } > if (attr->ia_valid & ATTR_UID) { > + kuid_t kuid; > + > attrs.ia_valid |= HOSTFS_ATTR_UID; > - attrs.ia_uid = from_kuid(&init_user_ns, attr->ia_uid); > + /* Map the vfs id into the filesystem. */ > + kuid = from_vfsuid(idmap, &init_user_ns, attr->ia_vfsuid); > + /* Map the filesystem id to its raw on disk value. */ > + attrs.ia_uid = from_kuid(&init_user_ns, kuid); Its interesting that this is what I originally discarded, as an unfamiliar reader, it looks like you're doing two namespace mappings. But that's not happening because from_kuid() disregards its namespace parameter. I've tested this and it does seems to work. Thanks! Glenn > } > if (attr->ia_valid & ATTR_GID) { > + kgid_t kgid; > + > attrs.ia_valid |= HOSTFS_ATTR_GID; > - attrs.ia_gid = from_kgid(&init_user_ns, attr->ia_gid); > + /* Map the vfs id into the filesystem. */ > + kgid = from_vfsgid(idmap, &init_user_ns, attr->ia_vfsgid); > + /* Map the filesystem id to its raw on disk value. */ > + attrs.ia_gid = from_kgid(&init_user_ns, kgid); > } > if (attr->ia_valid & ATTR_SIZE) { > attrs.ia_valid |= HOSTFS_ATTR_SIZE; _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v2] hostfs: handle idmapped mounts 2023-03-16 6:20 ` Glenn Washburn @ 2023-03-16 10:37 ` Christian Brauner 0 siblings, 0 replies; 6+ messages in thread From: Christian Brauner @ 2023-03-16 10:37 UTC (permalink / raw) To: Glenn Washburn Cc: Richard Weinberger, Anton Ivanov, Johannes Berg, linux-um, linux-kernel, Seth Forshee, linux-fsdevel On Thu, Mar 16, 2023 at 06:20:19AM +0000, Glenn Washburn wrote: > On 3/4/23 12:01, Christian Brauner wrote: > > On Sat, Mar 04, 2023 at 12:28:46AM -0600, Glenn Washburn wrote: > > > On Thu, 2 Mar 2023 09:39:28 +0100 > > > Christian Brauner <brauner@kernel.org> wrote: > > > > > > > On Tue, Feb 28, 2023 at 07:50:02PM -0600, Glenn Washburn wrote: > > > > > Let hostfs handle idmapped mounts. This allows to have the same > > > > > hostfs mount appear in multiple locations with different id > > > > > mappings. > > > > > > > > > > root@(none):/media# id > > > > > uid=0(root) gid=0(root) groups=0(root) > > > > > root@(none):/media# mkdir mnt idmapped > > > > > root@(none):/media# mount -thostfs -o/home/user hostfs mnt > > > > > > > > > > root@(none):/media# touch mnt/aaa > > > > > root@(none):/media# mount-idmapped --map-mount u:`id -u user`:0:1 > > > > > --map-mount g:`id -g user`:0:1 /media/mnt /media/idmapped > > > > > root@(none):/media# ls -l mnt/aaa idmapped/aaa -rw-r--r-- 1 root > > > > > root 0 Jan 28 01:23 idmapped/aaa -rw-r--r-- 1 user user 0 Jan 28 > > > > > 01:23 mnt/aaa > > > > > > > > > > root@(none):/media# touch idmapped/bbb > > > > > root@(none):/media# ls -l mnt/bbb idmapped/bbb > > > > > -rw-r--r-- 1 root root 0 Jan 28 01:26 idmapped/bbb > > > > > -rw-r--r-- 1 user user 0 Jan 28 01:26 mnt/bbb > > > > > > > > > > Signed-off-by: Glenn Washburn <development@efficientek.com> > > > > > --- > > > > > Changes from v1: > > > > > * Rebase on to tip. The above commands work and have the results > > > > > expected. The __vfsuid_val(make_vfsuid(...)) seems ugly to get the > > > > > uid_t, but it seemed like the best one I've come across. Is there a > > > > > better way? > > > > > > > > Sure, I can help you with that. ;) > > > > > > Thank you! > > > > > > > > > > > > > Glenn > > > > > --- > > > > > fs/hostfs/hostfs_kern.c | 13 +++++++------ > > > > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c > > > > > index c18bb50c31b6..9459da99a0db 100644 > > > > > --- a/fs/hostfs/hostfs_kern.c > > > > > +++ b/fs/hostfs/hostfs_kern.c > > > > > @@ -786,7 +786,7 @@ static int hostfs_permission(struct mnt_idmap > > > > > *idmap, err = access_file(name, r, w, x); > > > > > __putname(name); > > > > > if (!err) > > > > > - err = generic_permission(&nop_mnt_idmap, ino, > > > > > desired); > > > > > + err = generic_permission(idmap, ino, desired); > > > > > return err; > > > > > } > > > > > @@ -794,13 +794,14 @@ static int hostfs_setattr(struct mnt_idmap > > > > > *idmap, struct dentry *dentry, struct iattr *attr) > > > > > { > > > > > struct inode *inode = d_inode(dentry); > > > > > + struct user_namespace *fs_userns = i_user_ns(inode); > > > > > > > > Fyi, since hostfs can't be mounted in a user namespace > > > > fs_userns == &init_user_ns > > > > so it doesn't really matter what you use. > > > > > > What would you suggest as preferable? > > > > I would leave init_user_ns hardcoded as it clearly indicates that hostfs > > can only be mounted in the initial user namespace. Plus, the patch is > > smaller. > > > > > > > > > > struct hostfs_iattr attrs; > > > > > char *name; > > > > > int err; > > > > > int fd = HOSTFS_I(inode)->fd; > > > > > - err = setattr_prepare(&nop_mnt_idmap, dentry, attr); > > > > > + err = setattr_prepare(idmap, dentry, attr); > > > > > if (err) > > > > > return err; > > > > > @@ -814,11 +815,11 @@ static int hostfs_setattr(struct mnt_idmap > > > > > *idmap, } > > > > > if (attr->ia_valid & ATTR_UID) { > > > > > attrs.ia_valid |= HOSTFS_ATTR_UID; > > > > > - attrs.ia_uid = from_kuid(&init_user_ns, > > > > > attr->ia_uid); > > > > > + attrs.ia_uid = __vfsuid_val(make_vfsuid(idmap, > > > > > fs_userns, attr->ia_uid)); } > > > > > if (attr->ia_valid & ATTR_GID) { > > > > > attrs.ia_valid |= HOSTFS_ATTR_GID; > > > > > - attrs.ia_gid = from_kgid(&init_user_ns, > > > > > attr->ia_gid); > > > > > + attrs.ia_gid = __vfsgid_val(make_vfsgid(idmap, > > > > > fs_userns, attr->ia_gid)); > > > > > > > > Heh, if you look include/linux/fs.h: > > > > > > > > /* > > > > * The two anonymous unions wrap structures with the same > > > > member. * > > > > * Filesystems raising FS_ALLOW_IDMAP need to use > > > > ia_vfs{g,u}id which > > > > * are a dedicated type requiring the filesystem to use the > > > > dedicated > > > > * helpers. Other filesystem can continue to use ia_{g,u}id > > > > until they > > > > * have been ported. > > > > * > > > > * They always contain the same value. In other words > > > > FS_ALLOW_IDMAP > > > > * pass down the same value on idmapped mounts as they would > > > > on regular > > > > * mounts. > > > > */ > > > > union { > > > > kuid_t ia_uid; > > > > vfsuid_t ia_vfsuid; > > > > }; > > > > union { > > > > kgid_t ia_gid; > > > > vfsgid_t ia_vfsgid; > > > > }; > > > > > > > > this just is: > > > > > > > > attrs.ia_uid = from_vfsuid(idmap, fs_userns, attr->ia_vfsuid)); > > > > attrs.ia_gid = from_vfsgid(idmap, fs_userns, attr->ia_vfsgid)); > > > > > > Its easy to miss from this patch because of lack of context, but attrs > > > is a struct hostfs_iattr, not struct iattr. And attrs.ia_uid is of type > > > uid_t, not kuid_t. So the above fails to compile. This is why I needed > > > > Oh, I see. And then that raw value is used by calling > > fchmod()/chmod()/chown()/fchown() and so on. That's rather special. > > Ok, then I know what to do. > > > > > to wrap make_vfsuid() in __vfsuid_val() (to get the uid_t). > > > > Right. My point had been - independent of the struct hostfs_iattr issue > > you thankfully pointed out - that make_vfsuid() is wrong here. > > > > make_vfsuid() is used to map a filesystem wide k{g,u}id_t according to > > the mount's idmapping that operation originated from. But that's done > > by the vfs way before we're calling into the filesystem. For example, > > it's done in chown_common(). > > > > So the value placed in struct iattr (the VFS struct) is already a > > vfs{g,u}id stored in iattr->ia_vfs{g,u}id. So you need to use > > from_vfs{g,u}id() here. > > > > > > > > I had decided against using from_vfsuid() because then I thought I'd > > > need to use from_kuid() to get the uid_t. And from_kuid() takes the > > > namespace (again), which seemed uglier. > > > > > > Based on this, what do you suggest? > > > > Ok, so just some details on the background before I paste what I think > > we should do. > > As soon as you support idmapped mounts you at least technically are > Thanks for the detailed explanation. I apologize for not getting back to > this sooner. No need to apologize! > > > always dealing with two mappings: > > > > (1) First, there's the filesystem wide idmapping which is taken from the > > namespace the filessytem was mounted in. This idmapping is applied > > when you read the raw uid/gid value from disk and turn into a kuid_t > > type. That value is persistent and stored in inode->i_{g,u}id. All > > things that are cached and that can be accessed from multiple mounts > > concurrently can only ever cache k{g,u}id_t aka filesystem values. > > (2) Whenever we're dealing with an operation that's coming from an > > idmapped mount we need to take the idmapping of the mount into > > account. That idmapping is completely separate type struct > > mnt_idmap that's opaque for filesystems and most of the vfs. > > > > That idmapping is used to generate the vfs{g,u}id_t. IOW, translates > > from the filesystem representation to a mount/vfs representation. > > > > So, in order to store the correct value on disk we need to invert those > > two idmappings to arrive at the raw value that we want to store: > > (U1) from_vfsuid() // map to the filesystem wide value aka something > > that we can store in inode->i_{g,u}id and that's cacheable. This is > > done in setattr_copy(). > > (U2) from_kuid() // map the filesystem wide value to the raw value we > > want to store on disk > > It seems to me that there are actually 3 mappings, with the third being (U2) > above (ie vfsuid_t -> kuid_t). And that from_vfsuid() does mappings (1) and > (2) above. Is this incorrect? My language was confusing. You're dealing two _idmappings_ but 3 mappings _may_ be performed iff the filesystem you're dealing with can itself be mounted with an idmapping/in a namespace. Otherwise it's just two mappings as the filesystem idmapping is a nop, i.e., maps 0:0, 1:1, ..., n:n. For details you can read Documentation/filesystems/idmappings.rst but note that it's out of date and requires the patch I picked up a few days ago: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/commit/?h=for-next&id=5d3ca5968480758a29a0b2777da9049a7c5134e3 > > Whats confusing to me is that from_vfsuid() takes both an idmap and a user > namespace, so presumably it will handle both mapping types (1) and (2). And > then there's from_kuid() which takes an idmap, so I thought it might also do > a type (2) mapping. But looking at the code it doesn't seem to ever use its > idmap parameter. Can you explain the rational behind having from_kuid() take > an idmap? Is it legacy that will be cleaned up as this code settles down / > stabilizes? Or perhaps its Sure, I can explain. The from_kuid() function maps from a kuid_t into a uid_t and the make_kuid() function maps from a uid_t into a kuid_t. They are used to interact with a caller's idmapping (current_user_ns()) or a filesystem idmapping (sb->s_user_ns). (As far as I'm concerned, that's already a severe ambiguity as a filesystem idmapping is something very different from a caller idmapping. Technically, this should've been expressed in the form of a dedicated type struct fs_userns or sm to keep them distinct. But that's a different topic.) So, idmapped mounts bring in the concept of per mount ownership. So you need to distinguish between filesystem wide ownership (kuid_t/kgid_t) and mount local/VFS ownership (vfsuid_t/vfsgid_t). Expressing this in two different types is a safety measure so you can't generate a vfsuid_t via make_kuid() and you can't generate a uid_t from a vfsuid_t via from_kuid(). Because they fundamentally shouldn't be concerned with VFS/mount ownership to avoid confusion. So the next step is to ensure that the idmapping used to generate such vfsuid_t/vfsgid_t is type-distinct from caller- and filesystem namespaces. This way you can never pass a mount idmapping to from_kuid(), make_kuid(), or accidently use it for permission checking in ns_capable() etc. So the dedicated struct mnt_idmap type makes such bugs impossible as it can only be passed to dedicated helpers and cannot be dereferenced outside of a tiny piece of core VFS code. In addition to that, we should aim to let filesystems create idmapped mounts without having to allocate namespaces. Ofc, for the container use-case it's very convenient to just be able to reuse the container's namespace, attach it to a mount and thus delegate the mount to the container. This is especially nice because it _can_ be used make container ownership completely transient. IOW, you could spawn the same container with a random idmapping everytime because the idmap isn't persisted on-disk. Back to your case. As I've explained. hostfs cannot be mounted inside of namespaces. Thus, the idmapping attached to the filesystem retievable via i_user_ns() is a nop, i.e., it maps 0:0, 1:1, ..., n:n. So there's nothing to do for from_kuid() or make_kuid() before mapping into a mount. More details you should be able to find in Documentation/filesystems/idmappings.rst as mentioned before. > > > > > For nearly all filesystems these steps almost never need to be performed > > explicitly. Instead, dedicated vfs helpers will do this: > > > > (U1) i_{g,u}id_update() // map to filesystem wide value > > (U2) i_{g,u}id_read() // map to raw on-disk value > > > > For filesystems that don't support being mounted in namespaces the (U2) > > step is always a nop. So technically there's no difference between: > > > > (U2) from_kuid() and __kuid_val(kuid) > > > > but it's cleaner to use the helpers even in that case. > > > > So given how hostfs works these two steps need to be performed > > explicitly. So I suggest (untested): > > > > diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c > > index c18bb50c31b6..72b7e1bcc32e 100644 > > --- a/fs/hostfs/hostfs_kern.c > > +++ b/fs/hostfs/hostfs_kern.c > > @@ -813,12 +813,22 @@ static int hostfs_setattr(struct mnt_idmap *idmap, > > attrs.ia_mode = attr->ia_mode; > > } > > if (attr->ia_valid & ATTR_UID) { > > + kuid_t kuid; > > + > > attrs.ia_valid |= HOSTFS_ATTR_UID; > > - attrs.ia_uid = from_kuid(&init_user_ns, attr->ia_uid); > > + /* Map the vfs id into the filesystem. */ > > + kuid = from_vfsuid(idmap, &init_user_ns, attr->ia_vfsuid); > > + /* Map the filesystem id to its raw on disk value. */ > > + attrs.ia_uid = from_kuid(&init_user_ns, kuid); > > Its interesting that this is what I originally discarded, as an unfamiliar > reader, it looks like you're doing two namespace mappings. But that's not > happening because from_kuid() disregards its namespace parameter. It's a nop because hostfs isn't mountable inside of namespace/with an idmapping. > > I've tested this and it does seems to work. Thanks! +1 Not sure if you have xfstests integration but if you do: sudo ./check -g idmapped you'll get a whole testsuite dedicated to it. _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-16 10:37 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-01 1:50 [RFC PATCH v2] hostfs: handle idmapped mounts Glenn Washburn 2023-03-02 8:39 ` Christian Brauner 2023-03-04 6:28 ` Glenn Washburn 2023-03-04 12:01 ` Christian Brauner 2023-03-16 6:20 ` Glenn Washburn 2023-03-16 10:37 ` Christian Brauner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).