diff mbox series

[v1,4/9] fs/fuse: support idmapped getattr inode op

Message ID	20240108120824.122178-5-aleksandr.mikhalitsyn@canonical.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel+bounces-19475-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1; From: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> To: mszeredi@redhat.com Cc: brauner@kernel.org, stgraber@stgraber.org, linux-fsdevel@vger.kernel.org, Seth Forshee <sforshee@kernel.org>, Miklos Szeredi <miklos@szeredi.hu>, Amir Goldstein <amir73il@gmail.com>, Bernd Schubert <bschubert@ddn.com>, Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>, linux-kernel@vger.kernel.org Subject: [PATCH v1 4/9] fs/fuse: support idmapped getattr inode op Date: Mon, 8 Jan 2024 13:08:19 +0100 Message-Id: <20240108120824.122178-5-aleksandr.mikhalitsyn@canonical.com> In-Reply-To: <20240108120824.122178-1-aleksandr.mikhalitsyn@canonical.com> References: <20240108120824.122178-1-aleksandr.mikhalitsyn@canonical.com> Precedence: bulk MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX
Series	fuse: basic support for idmapped mounts \| [v1,0/9] fuse: basic support for idmapped mounts [v1,1/9] fs/namespace: introduce fs_type->allow_idmap hook [v1,2/9] fs/fuse: add FUSE_OWNER_UID_GID_EXT extension [v1,3/9] fs/fuse: support idmap for mkdir/mknod/symlink/create [v1,4/9] fs/fuse: support idmapped getattr inode op [v1,5/9] fs/fuse: support idmapped ->permission inode op [v1,6/9] fs/fuse: support idmapped ->setattr op [v1,7/9] fs/fuse: drop idmap argument from __fuse_get_acl [v1,8/9] fs/fuse: support idmapped ->set_acl [v1,9/9] fs/fuse: allow idmapped mounts

Commit Message

Aleksandr Mikhalitsyn Jan. 8, 2024, 12:08 p.m. UTC

  We have to:
- pass an idmapping to the generic_fillattr()
to properly handle UIG/GID mapping for the userspace.
- pass -/- to fuse_fillattr() (analog of generic_fillattr() in fuse).

Difference between these two is that generic_fillattr() takes all
the stat() data from the inode directly, while fuse_fillattr() codepath
takes a fresh data just from the userspace reply on the FUSE_GETATTR request.

Cc: Christian Brauner <brauner@kernel.org>
Cc: Seth Forshee <sforshee@kernel.org>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Bernd Schubert <bschubert@ddn.com>
Cc: <linux-fsdevel@vger.kernel.org>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
 fs/fuse/dir.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

Comments

Christian Brauner Jan. 20, 2024, 3:21 p.m. UTC | #1

>  int fuse_update_attributes(struct inode *inode, struct file *file, u32 mask)
>  {
> -	return fuse_update_get_attr(inode, file, NULL, mask, 0);
> +	return fuse_update_get_attr(&nop_mnt_idmap, inode, file, NULL, mask, 0);
>  }
>  
>  int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
> @@ -1506,7 +1510,7 @@ static int fuse_perm_getattr(struct inode *inode, int mask)
>  		return -ECHILD;
>  
>  	forget_all_cached_acls(inode);
> -	return fuse_do_getattr(inode, NULL, NULL);
> +	return fuse_do_getattr(&nop_mnt_idmap, inode, NULL, NULL);
>  }
>  
>  /*
> @@ -2062,7 +2066,7 @@ static int fuse_setattr(struct mnt_idmap *idmap, struct dentry *entry,
>  			 * ia_mode calculation may have used stale i_mode.
>  			 * Refresh and recalculate.
>  			 */
> -			ret = fuse_do_getattr(inode, NULL, file);
> +			ret = fuse_do_getattr(&nop_mnt_idmap, inode, NULL, file);
>  			if (ret)
>  				return ret;

These are internal getattr requests that don't originate from a specific mount?
Can you please add a comment about this in the commit message so it's
clear why it's ok to not pass the idmapping?

Aleksandr Mikhalitsyn Jan. 29, 2024, 3:42 p.m. UTC | #2

On Sat, 20 Jan 2024 16:21:08 +0100
Christian Brauner <brauner@kernel.org> wrote:

> >  int fuse_update_attributes(struct inode *inode, struct file *file, u32 mask)
> >  {
> > -	return fuse_update_get_attr(inode, file, NULL, mask, 0);
> > +	return fuse_update_get_attr(&nop_mnt_idmap, inode, file, NULL, mask, 0);
> >  }
> >  
> >  int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
> > @@ -1506,7 +1510,7 @@ static int fuse_perm_getattr(struct inode *inode, int mask)
> >  		return -ECHILD;
> >  
> >  	forget_all_cached_acls(inode);
> > -	return fuse_do_getattr(inode, NULL, NULL);
> > +	return fuse_do_getattr(&nop_mnt_idmap, inode, NULL, NULL);
> >  }
> >  
> >  /*
> > @@ -2062,7 +2066,7 @@ static int fuse_setattr(struct mnt_idmap *idmap, struct dentry *entry,
> >  			 * ia_mode calculation may have used stale i_mode.
> >  			 * Refresh and recalculate.
> >  			 */
> > -			ret = fuse_do_getattr(inode, NULL, file);
> > +			ret = fuse_do_getattr(&nop_mnt_idmap, inode, NULL, file);
> >  			if (ret)
> >  				return ret;
> 

Hi, Christian!

> These are internal getattr requests that don't originate from a specific mount?

These requests do originate from a specific mount, but we don't need an idmapping in there
because 3rd argument of fuse_do_getattr(struct mnt_idmap *idmap, struct inode *inode, struct kstat *stat, struct file *file) is NULL,
which means that this request will not fill an stat structure => we don't need to take an idmapping into account.

> Can you please add a comment about this in the commit message so it's
> clear why it's ok to not pass the idmapping?

Sure, that's my bad that I haven't added this explanation! It's not obvious at all.

Will be fixed in -v2.

Thanks,
Alex

diff mbox series

Patch

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index a0968f086b62..5efcf06622f0 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1149,18 +1149,22 @@  static int fuse_link(struct dentry *entry, struct inode *newdir,
 	return err;
 }
 
-static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr,
-			  struct kstat *stat)
+static void fuse_fillattr(struct mnt_idmap *idmap, struct inode *inode,
+			  struct fuse_attr *attr, struct kstat *stat)
 {
 	unsigned int blkbits;
 	struct fuse_conn *fc = get_fuse_conn(inode);
+	vfsuid_t vfsuid = make_vfsuid(idmap, fc->user_ns,
+				      make_kuid(fc->user_ns, attr->uid));
+	vfsgid_t vfsgid = make_vfsgid(idmap, fc->user_ns,
+				      make_kgid(fc->user_ns, attr->gid));
 
 	stat->dev = inode->i_sb->s_dev;
 	stat->ino = attr->ino;
 	stat->mode = (inode->i_mode & S_IFMT) | (attr->mode & 07777);
 	stat->nlink = attr->nlink;
-	stat->uid = make_kuid(fc->user_ns, attr->uid);
-	stat->gid = make_kgid(fc->user_ns, attr->gid);
+	stat->uid = vfsuid_into_kuid(vfsuid);
+	stat->gid = vfsgid_into_kgid(vfsgid);
 	stat->rdev = inode->i_rdev;
 	stat->atime.tv_sec = attr->atime;
 	stat->atime.tv_nsec = attr->atimensec;
@@ -1199,8 +1203,8 @@  static void fuse_statx_to_attr(struct fuse_statx *sx, struct fuse_attr *attr)
 	attr->blksize = sx->blksize;
 }
 
-static int fuse_do_statx(struct inode *inode, struct file *file,
-			 struct kstat *stat)
+static int fuse_do_statx(struct mnt_idmap *idmap, struct inode *inode,
+			 struct file *file, struct kstat *stat)
 {
 	int err;
 	struct fuse_attr attr;
@@ -1253,15 +1257,15 @@  static int fuse_do_statx(struct inode *inode, struct file *file,
 		stat->result_mask = sx->mask & (STATX_BASIC_STATS | STATX_BTIME);
 		stat->btime.tv_sec = sx->btime.tv_sec;
 		stat->btime.tv_nsec = min_t(u32, sx->btime.tv_nsec, NSEC_PER_SEC - 1);
-		fuse_fillattr(inode, &attr, stat);
+		fuse_fillattr(idmap, inode, &attr, stat);
 		stat->result_mask |= STATX_TYPE;
 	}
 
 	return 0;
 }
 
-static int fuse_do_getattr(struct inode *inode, struct kstat *stat,
-			   struct file *file)
+static int fuse_do_getattr(struct mnt_idmap *idmap, struct inode *inode,
+			   struct kstat *stat, struct file *file)
 {
 	int err;
 	struct fuse_getattr_in inarg;
@@ -1300,15 +1304,15 @@  static int fuse_do_getattr(struct inode *inode, struct kstat *stat,
 					       ATTR_TIMEOUT(&outarg),
 					       attr_version);
 			if (stat)
-				fuse_fillattr(inode, &outarg.attr, stat);
+				fuse_fillattr(idmap, inode, &outarg.attr, stat);
 		}
 	}
 	return err;
 }
 
-static int fuse_update_get_attr(struct inode *inode, struct file *file,
-				struct kstat *stat, u32 request_mask,
-				unsigned int flags)
+static int fuse_update_get_attr(struct mnt_idmap *idmap, struct inode *inode,
+				struct file *file, struct kstat *stat,
+				u32 request_mask, unsigned int flags)
 {
 	struct fuse_inode *fi = get_fuse_inode(inode);
 	struct fuse_conn *fc = get_fuse_conn(inode);
@@ -1339,16 +1343,16 @@  static int fuse_update_get_attr(struct inode *inode, struct file *file,
 		forget_all_cached_acls(inode);
 		/* Try statx if BTIME is requested */
 		if (!fc->no_statx && (request_mask & ~STATX_BASIC_STATS)) {
-			err = fuse_do_statx(inode, file, stat);
+			err = fuse_do_statx(idmap, inode, file, stat);
 			if (err == -ENOSYS) {
 				fc->no_statx = 1;
 				goto retry;
 			}
 		} else {
-			err = fuse_do_getattr(inode, stat, file);
+			err = fuse_do_getattr(idmap, inode, stat, file);
 		}
 	} else if (stat) {
-		generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
+		generic_fillattr(idmap, request_mask, inode, stat);
 		stat->mode = fi->orig_i_mode;
 		stat->ino = fi->orig_ino;
 		if (test_bit(FUSE_I_BTIME, &fi->state)) {
@@ -1362,7 +1366,7 @@  static int fuse_update_get_attr(struct inode *inode, struct file *file,
 
 int fuse_update_attributes(struct inode *inode, struct file *file, u32 mask)
 {
-	return fuse_update_get_attr(inode, file, NULL, mask, 0);
+	return fuse_update_get_attr(&nop_mnt_idmap, inode, file, NULL, mask, 0);
 }
 
 int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
@@ -1506,7 +1510,7 @@  static int fuse_perm_getattr(struct inode *inode, int mask)
 		return -ECHILD;
 
 	forget_all_cached_acls(inode);
-	return fuse_do_getattr(inode, NULL, NULL);
+	return fuse_do_getattr(&nop_mnt_idmap, inode, NULL, NULL);
 }
 
 /*
@@ -2062,7 +2066,7 @@  static int fuse_setattr(struct mnt_idmap *idmap, struct dentry *entry,
 			 * ia_mode calculation may have used stale i_mode.
 			 * Refresh and recalculate.
 			 */
-			ret = fuse_do_getattr(inode, NULL, file);
+			ret = fuse_do_getattr(&nop_mnt_idmap, inode, NULL, file);
 			if (ret)
 				return ret;
 
@@ -2119,7 +2123,7 @@  static int fuse_getattr(struct mnt_idmap *idmap,
 		return -EACCES;
 	}
 
-	return fuse_update_get_attr(inode, NULL, stat, request_mask, flags);
+	return fuse_update_get_attr(idmap, inode, NULL, stat, request_mask, flags);
 }
 
 static const struct inode_operations fuse_dir_inode_operations = {