Message-ID: <20210313143148.d6rhgmhxwq6abb6y@wittgenstein>
Date:   Sat, 13 Mar 2021 15:31:48 +0100
From:   Christian Brauner <christian.brauner@...ntu.com>
To:     Vivek Goyal <vgoyal@...hat.com>, Christoph Hellwig <hch@....de>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel@...r.kernel.org, Andy Lutomirski <luto@...nel.org>,
        Theodore Tso <tytso@....edu>, Alban Crequy <alban@...volk.io>,
        Tycho Andersen <tycho@...ho.ws>,
        Seth Forshee <seth.forshee@...onical.com>,
        Stéphane Graber <stgraber@...ntu.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Aleksa Sarai <cyphar@...har.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Serge Hallyn <serge@...lyn.com>, linux-ext4@...r.kernel.org,
        linux-xfs@...r.kernel.org
Subject: Re: [PATCH v6 02/40] fs: add id translation helpers

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@ubuntu.com
> > Cc: David Howells <dhowells@...hat.com>
> > Cc: Al Viro <viro@...iv.linux.org.uk>
> > Cc: linux-fsdevel@...r.kernel.org
> > Reviewed-by: Christoph Hellwig <hch@....de>
> > Signed-off-by: Christian Brauner <christian.brauner@...ntu.com>
> > ---
> > /* v2 */
> > - Christoph Hellwig <hch@....de>:
> >   - Get rid of the ifdefs and the config option that hid idmapped mounts.
> > 
> > /* v3 */
> > unchanged
> > 
> > /* v4 */
> > - Serge Hallyn <serge@...lyn.com>:
> >   - 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):

>From 1bab0249295d0cad359f39a38e6171bcd2d68a60 Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner@...ntu.com>
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@...hat.com>
Cc: Christoph Hellwig <hch@....de>
Cc: Al Viro <viro@...iv.linux.org.uk>
Signed-off-by: Christian Brauner <christian.brauner@...ntu.com>
---
 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@...ntu.com>
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@...hat.com>
Cc: Christoph Hellwig <hch@....de>
Cc: Al Viro <viro@...iv.linux.org.uk>
Signed-off-by: Christian Brauner <christian.brauner@...ntu.com>
---
 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

Powered by blists - more mailing lists