Received-SPF: pass (google.com: domain of linux-kernel+bounces-188760-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 24 May 2024 15:58:15 +0200
From: Christian Brauner <brauner@kernel.org>
To: Michael Jeanson <mjeanson@efficios.com>, 
	Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
Cc: linux-kernel@vger.kernel.org, Namjae Jeon <linkinjeon@kernel.org>, 
	Sungjong Seo <sj1557.seo@samsung.com>, Seth Forshee <sforshee@kernel.org>, 
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] exfat: handle idmapped mounts
Message-ID: <20240524-verordnen-exhumieren-2ea2e8bc2d08@brauner>
References: <20240523011007.40649-1-mjeanson@efficios.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240523011007.40649-1-mjeanson@efficios.com>

[Cc: Alex]

On Wed, May 22, 2024 at 09:10:07PM -0400, Michael Jeanson wrote:
> Pass the idmapped mount information to the different helper
> functions. Adapt the uid/gid checks in exfat_setattr to use the
> vfsuid/vfsgid helpers.
> 
> Based on the fat implementation in commit 4b7899368108
> ("fat: handle idmapped mounts") by Christian Brauner.
> 
> Cc: Namjae Jeon <linkinjeon@kernel.org>
> Cc: Sungjong Seo <sj1557.seo@samsung.com>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Seth Forshee <sforshee@kernel.org>
> Cc: linux-fsdevel@vger.kernel.org
> Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
> ---

Looks good to me but maybe Alex sees some issues I dont,
Reviewed-by: Christian Brauner <brauner@kernel.org>

>  fs/exfat/file.c  | 22 +++++++++++++---------
>  fs/exfat/super.c |  2 +-
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/exfat/file.c b/fs/exfat/file.c
> index 9adfc38ca7da..64c31867bc76 100644
> --- a/fs/exfat/file.c
> +++ b/fs/exfat/file.c
> @@ -89,12 +89,14 @@ static int exfat_cont_expand(struct inode *inode, loff_t size)
>  	return -EIO;
>  }
>  
> -static bool exfat_allow_set_time(struct exfat_sb_info *sbi, struct inode *inode)
> +static bool exfat_allow_set_time(struct mnt_idmap *idmap,
> +				 struct exfat_sb_info *sbi, struct inode *inode)
>  {
>  	mode_t allow_utime = sbi->options.allow_utime;
>  
> -	if (!uid_eq(current_fsuid(), inode->i_uid)) {
> -		if (in_group_p(inode->i_gid))
> +	if (!vfsuid_eq_kuid(i_uid_into_vfsuid(idmap, inode),
> +			    current_fsuid())) {
> +		if (vfsgid_in_group_p(i_gid_into_vfsgid(idmap, inode)))
>  			allow_utime >>= 3;
>  		if (allow_utime & MAY_WRITE)
>  			return true;
> @@ -283,7 +285,7 @@ int exfat_getattr(struct mnt_idmap *idmap, const struct path *path,
>  	struct inode *inode = d_backing_inode(path->dentry);
>  	struct exfat_inode_info *ei = EXFAT_I(inode);
>  
> -	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
> +	generic_fillattr(idmap, request_mask, inode, stat);
>  	exfat_truncate_atime(&stat->atime);
>  	stat->result_mask |= STATX_BTIME;
>  	stat->btime.tv_sec = ei->i_crtime.tv_sec;
> @@ -311,20 +313,22 @@ int exfat_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>  	/* Check for setting the inode time. */
>  	ia_valid = attr->ia_valid;
>  	if ((ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET | ATTR_TIMES_SET)) &&
> -	    exfat_allow_set_time(sbi, inode)) {
> +	    exfat_allow_set_time(idmap, sbi, inode)) {
>  		attr->ia_valid &= ~(ATTR_MTIME_SET | ATTR_ATIME_SET |
>  				ATTR_TIMES_SET);
>  	}
>  
> -	error = setattr_prepare(&nop_mnt_idmap, dentry, attr);
> +	error = setattr_prepare(idmap, dentry, attr);
>  	attr->ia_valid = ia_valid;
>  	if (error)
>  		goto out;
>  
>  	if (((attr->ia_valid & ATTR_UID) &&
> -	     !uid_eq(attr->ia_uid, sbi->options.fs_uid)) ||
> +	      (!uid_eq(from_vfsuid(idmap, i_user_ns(inode), attr->ia_vfsuid),
> +	       sbi->options.fs_uid))) ||
>  	    ((attr->ia_valid & ATTR_GID) &&
> -	     !gid_eq(attr->ia_gid, sbi->options.fs_gid)) ||
> +	      (!gid_eq(from_vfsgid(idmap, i_user_ns(inode), attr->ia_vfsgid),
> +	       sbi->options.fs_gid))) ||
>  	    ((attr->ia_valid & ATTR_MODE) &&
>  	     (attr->ia_mode & ~(S_IFREG | S_IFLNK | S_IFDIR | 0777)))) {
>  		error = -EPERM;
> @@ -343,7 +347,7 @@ int exfat_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>  	if (attr->ia_valid & ATTR_SIZE)
>  		inode_set_mtime_to_ts(inode, inode_set_ctime_current(inode));
>  
> -	setattr_copy(&nop_mnt_idmap, inode, attr);
> +	setattr_copy(idmap, inode, attr);
>  	exfat_truncate_inode_atime(inode);
>  
>  	if (attr->ia_valid & ATTR_SIZE) {
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
> index 3d5ea2cfad66..1f2b3b0c4923 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -788,7 +788,7 @@ static struct file_system_type exfat_fs_type = {
>  	.init_fs_context	= exfat_init_fs_context,
>  	.parameters		= exfat_parameters,
>  	.kill_sb		= exfat_kill_sb,
> -	.fs_flags		= FS_REQUIRES_DEV,
> +	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
>  };
>  
>  static void exfat_inode_init_once(void *foo)
> -- 
> 2.45.1
>