On Sat, Mar 13, 2021 at 03:31:48PM +0100, Christian Brauner wrote:
> On Fri, Mar 12, 2021 at 07:05:29PM -0500, Vivek Goyal wrote:
> > On Thu, Jan 21, 2021 at 02:19:21PM +0100, Christian Brauner wrote:
> > > Add simple helpers to make it easy to map kuids into and from idmapped
> > > mounts. We provide simple wrappers that filesystems can use to e.g.
> > > initialize inodes similar to i_{uid,gid}_read() and i_{uid,gid}_write().
> > > Accessing an inode through an idmapped mount maps the i_uid and i_gid of
> > > the inode to the mount's user namespace. If the fsids are used to
> > > initialize inodes they are unmapped according to the mount's user
> > > namespace. Passing the initial user namespace to these helpers makes
> > > them a nop and so any non-idmapped paths will not be impacted.
> > >
> > > Link: https://lore.kernel.org/r/20210112220124.837960-9-christian.brauner@xxxxxxxxxx
> > > Cc: David Howells <dhowells@xxxxxxxxxx>
> > > Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> > > Cc: linux-fsdevel@xxxxxxxxxxxxxxx
> > > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> > > Signed-off-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> > > ---
> > > /* v2 */
> > > - Christoph Hellwig <hch@xxxxxx>:
> > > - Get rid of the ifdefs and the config option that hid idmapped mounts.
> > >
> > > /* v3 */
> > > unchanged
> > >
> > > /* v4 */
> > > - Serge Hallyn <serge@xxxxxxxxxx>:
> > > - Use "mnt_userns" to refer to a vfsmount's userns everywhere to make
> > > terminology consistent.
> > >
> > > /* v5 */
> > > unchanged
> > > base-commit: 7c53f6b671f4aba70ff15e1b05148b10d58c2837
> > >
> > > /* v6 */
> > > unchanged
> > > base-commit: 19c329f6808995b142b3966301f217c831e7cf31
> > > ---
> > > include/linux/fs.h | 47 ++++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 47 insertions(+)
> > >
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index fd0b80e6361d..3165998e2294 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -40,6 +40,7 @@
> > > #include <linux/build_bug.h>
> > > #include <linux/stddef.h>
> > > #include <linux/mount.h>
> > > +#include <linux/cred.h>
> > >
> > > #include <asm/byteorder.h>
> > > #include <uapi/linux/fs.h>
> > > @@ -1573,6 +1574,52 @@ static inline void i_gid_write(struct inode *inode, gid_t gid)
> > > inode->i_gid = make_kgid(inode->i_sb->s_user_ns, gid);
> > > }
> > >
> > > +static inline kuid_t kuid_into_mnt(struct user_namespace *mnt_userns,
> > > + kuid_t kuid)
> > > +{
> > > + return make_kuid(mnt_userns, __kuid_val(kuid));
> > > +}
> > > +
> >
> > Hi Christian,
> >
> > I am having little trouble w.r.t function names and trying to figure
> > out whether they are mapping id down or up.
> >
> > For example, kuid_into_mnt() ultimately calls map_id_down(). That is,
> > id visible inside user namespace is mapped to host
> > (if observer is in init_user_ns, IIUC).
> >
> > But fsuid_into_mnt() ultimately calls map_id_up(). That's take a kuid
> > and map it into the user_namespace.
> >
> > So both the helpers end with into_mnt() but one maps id down and
> > other maps id up. I found this confusing and was wondering how
> > should I visualize it. So thought of asking you.
> >
> > Is this intentional or can naming be improved so that *_into_mnt()
> > means one thing (Either map_id_up() or map_id_down()). And vice-a-versa
> > for *_from_mnt().
>
> [Trimming my crazy Cc list to not spam everyone.].
>
> Hey Vivek,
>
> Thank you for your feedback, really appreciated!
>
> The naming was intended to always signify that the helpers always return
> a k{u,g}id but I can certainly see how the naming isn't as clear as it
> should be for those helpers in other ways. I would suggest we remove
> such direct exposures of these helpers completely and make it simpler
> for callers by introducing very straightforward helpers. See the tiny
> patches below (only compile tested for now):
Hi Chirstian,
Thanks for the following patches. Now we are only left with
kuid_from_mnt() and kuid_into_mnt() and I can wrap my head around
it.
Thanks
Vivek
>
> From 1bab0249295d0cad359f39a38e6171bcd2d68a60 Mon Sep 17 00:00:00 2001
> From: Christian Brauner <christian.brauner@xxxxxxxxxx>
> Date: Sat, 13 Mar 2021 15:08:04 +0100
> Subject: [PATCH 1/2] fs: introduce fsuidgid_has_mapping() helper
>
> Don't open-code the checks and instead move them into a clean little
> helper we can call. This also reduces the risk that if we ever changing
> something here we forget to change all locations.
>
> Inspired-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> ---
> fs/namei.c | 11 +++--------
> include/linux/fs.h | 13 +++++++++++++
> 2 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 216f16e74351..bc03cbc37ba7 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2823,16 +2823,14 @@ static int may_delete(struct user_namespace *mnt_userns, struct inode *dir,
> static inline int may_create(struct user_namespace *mnt_userns,
> struct inode *dir, struct dentry *child)
> {
> - struct user_namespace *s_user_ns;
> audit_inode_child(dir, child, AUDIT_TYPE_CHILD_CREATE);
> if (child->d_inode)
> return -EEXIST;
> if (IS_DEADDIR(dir))
> return -ENOENT;
> - s_user_ns = dir->i_sb->s_user_ns;
> - if (!kuid_has_mapping(s_user_ns, fsuid_into_mnt(mnt_userns)) ||
> - !kgid_has_mapping(s_user_ns, fsgid_into_mnt(mnt_userns)))
> + if (!fsuidgid_has_mapping(dir->i_sb, mnt_userns))
> return -EOVERFLOW;
> +
> return inode_permission(mnt_userns, dir, MAY_WRITE | MAY_EXEC);
> }
>
> @@ -3034,14 +3032,11 @@ static int may_o_create(struct user_namespace *mnt_userns,
> const struct path *dir, struct dentry *dentry,
> umode_t mode)
> {
> - struct user_namespace *s_user_ns;
> int error = security_path_mknod(dir, dentry, mode, 0);
> if (error)
> return error;
>
> - s_user_ns = dir->dentry->d_sb->s_user_ns;
> - if (!kuid_has_mapping(s_user_ns, fsuid_into_mnt(mnt_userns)) ||
> - !kgid_has_mapping(s_user_ns, fsgid_into_mnt(mnt_userns)))
> + if (!fsuidgid_has_mapping(dir->dentry->d_sb, mnt_userns))
> return -EOVERFLOW;
>
> error = inode_permission(mnt_userns, dir->dentry->d_inode,
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ec8f3ddf4a6a..a970a43afb0a 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1620,6 +1620,19 @@ static inline kgid_t fsgid_into_mnt(struct user_namespace *mnt_userns)
> return kgid_from_mnt(mnt_userns, current_fsgid());
> }
>
> +static inline bool fsuidgid_has_mapping(struct super_block *sb,
> + struct user_namespace *mnt_userns)
> +{
> + struct user_namespace *s_user_ns = sb->s_user_ns;
> + if (!kuid_has_mapping(s_user_ns,
> + kuid_from_mnt(mnt_userns, current_fsuid())))
> + return false;
> + if (!kgid_has_mapping(s_user_ns,
> + kgid_from_mnt(mnt_userns, current_fsgid())))
> + return false;
> + return true;
> +}
> +
> extern struct timespec64 current_time(struct inode *inode);
>
> /*
> --
> 2.27.0
>
>
> From 2f316f7de3ac96ecc8cc889724c0132e96b47b51 Mon Sep 17 00:00:00 2001
> From: Christian Brauner <christian.brauner@xxxxxxxxxx>
> Date: Sat, 13 Mar 2021 15:11:55 +0100
> Subject: [PATCH 2/2] fs: introduce two little fs{u,g}id inode initialization
> helpers
>
> As Vivek pointed out we could tweak the names of the fs{u,g}id helpers.
> That's already good but the better approach is to not expose them in
> this way to filesystems at all and simply give the filesystems two
> helpers inode_fsuid_set() and inode_fsgid_set() that will do the right
> thing.
>
> Inspired-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> ---
> fs/ext4/ialloc.c | 2 +-
> fs/inode.c | 4 ++--
> fs/xfs/xfs_inode.c | 2 +-
> include/linux/fs.h | 10 ++++++----
> 4 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index 633ae7becd61..755a68bb7e22 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -970,7 +970,7 @@ struct inode *__ext4_new_inode(struct user_namespace *mnt_userns,
> i_gid_write(inode, owner[1]);
> } else if (test_opt(sb, GRPID)) {
> inode->i_mode = mode;
> - inode->i_uid = fsuid_into_mnt(mnt_userns);
> + inode_fsuid_set(inode, mnt_userns);
> inode->i_gid = dir->i_gid;
> } else
> inode_init_owner(mnt_userns, inode, dir, mode);
> diff --git a/fs/inode.c b/fs/inode.c
> index a047ab306f9a..21c5a620ca89 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2148,7 +2148,7 @@ EXPORT_SYMBOL(init_special_inode);
> void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
> const struct inode *dir, umode_t mode)
> {
> - inode->i_uid = fsuid_into_mnt(mnt_userns);
> + inode_fsuid_set(inode, mnt_userns);
> if (dir && dir->i_mode & S_ISGID) {
> inode->i_gid = dir->i_gid;
>
> @@ -2160,7 +2160,7 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
> !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
> mode &= ~S_ISGID;
> } else
> - inode->i_gid = fsgid_into_mnt(mnt_userns);
> + inode_fsgid_set(inode, mnt_userns);
> inode->i_mode = mode;
> }
> EXPORT_SYMBOL(inode_init_owner);
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 46a861d55e48..aa924db90cd9 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -812,7 +812,7 @@ xfs_init_new_inode(
>
> if (dir && !(dir->i_mode & S_ISGID) &&
> (mp->m_flags & XFS_MOUNT_GRPID)) {
> - inode->i_uid = fsuid_into_mnt(mnt_userns);
> + inode_fsuid_set(inode, mnt_userns);
> inode->i_gid = dir->i_gid;
> inode->i_mode = mode;
> } else {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index a970a43afb0a..b337daa6b191 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1610,14 +1610,16 @@ static inline kgid_t kgid_from_mnt(struct user_namespace *mnt_userns,
> return KGIDT_INIT(from_kgid(mnt_userns, kgid));
> }
>
> -static inline kuid_t fsuid_into_mnt(struct user_namespace *mnt_userns)
> +static inline void inode_fsuid_set(struct inode *inode,
> + struct user_namespace *mnt_userns)
> {
> - return kuid_from_mnt(mnt_userns, current_fsuid());
> + inode->i_uid = kuid_from_mnt(mnt_userns, current_fsuid());
> }
>
> -static inline kgid_t fsgid_into_mnt(struct user_namespace *mnt_userns)
> +static inline void inode_fsgid_set(struct inode *inode,
> + struct user_namespace *mnt_userns)
> {
> - return kgid_from_mnt(mnt_userns, current_fsgid());
> + inode->i_gid = kgid_from_mnt(mnt_userns, current_fsgid());
> }
>
> static inline bool fsuidgid_has_mapping(struct super_block *sb,
> --
> 2.27.0
>
