Re: [PATCH v2 01/31] vfs: pass S_IFDIR mode to vfs_prepare_mode()

[Jan Kara]

On Tue 30-06-26 12:53:30, Jori Koolstra wrote:
> There is a comment in vfs_prepare_mode() that says:
> 
>   Note that it's currently valid for @type to be 0 if a directory is
>   created. Filesystems raise that flag individually and we need to check
>   whether each filesystem can deal with receiving S_IFDIR from the vfs
>   before we enforce a non-zero type.
> 
> It is safe to do this clean-up except that three filesystems (fuse,
> cifs, and coda) forward the mkdir @mode unchanged to something outside
> the kernel. Mask S_IFDIR back out in coda_mkdir(), fuse_mkdir() and
> cifs_mkdir() so that what is sent outside the kernel is unchanged.
> Their maintainers can drop the mask once they have confirmed it is safe.
> 
> Assisted-by: Claude Opus 4.8 (1M context)
> Signed-off-by: Jori Koolstra <jkoolstra@xxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/coda/dir.c         | 7 ++++++-
>  fs/fuse/dir.c         | 8 ++++++++
>  fs/namei.c            | 7 +------
>  fs/smb/client/inode.c | 7 +++++++
>  4 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/coda/dir.c b/fs/coda/dir.c
> index 835eb7fdfdad..9ad4d217c8b6 100644
> --- a/fs/coda/dir.c
> +++ b/fs/coda/dir.c
> @@ -179,7 +179,12 @@ static struct dentry *coda_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  	if (is_root_inode(dir) && coda_iscontrol(name, len))
>  		return ERR_PTR(-EPERM);
>  
> -	attrs.va_mode = mode;
> +	/*
> +	 * vfs_mkdir() now passes S_IFDIR in @mode, but @mode is forwarded
> +	 * verbatim to userspace, which has only ever been given the permission
> +	 * bits. Strip the type bit until venus is known to cope with it.
> +	 */
> +	attrs.va_mode = mode & ~S_IFDIR;
>  	error = venus_mkdir(dir->i_sb, coda_i2f(dir),
>  			       name, len, &newfid, &attrs);
>  	if (error)
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 0e2a1039fa43..7decbe4ea48a 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1117,6 +1117,14 @@ static struct dentry *fuse_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  	if (!fm->fc->dont_mask)
>  		mode &= ~current_umask();
>  
> +	/*
> +	 * vfs_mkdir() now passes S_IFDIR in @mode, but @mode is forwarded
> +	 * verbatim to the userspace server which has only ever been given the
> +	 * permission bits. Strip the type bit until the protocol is known to
> +	 * cope with it.
> +	 */
> +	mode &= ~S_IFDIR;
> +
>  	memset(&inarg, 0, sizeof(inarg));
>  	inarg.mode = mode;
>  	inarg.umask = current_umask();
> diff --git a/fs/namei.c b/fs/namei.c
> index 5cc9f0f466b8..6554803d6903 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4140,11 +4140,6 @@ EXPORT_SYMBOL(end_renaming);
>   * after setgid stripping allows the same ordering for both non-POSIX ACL and
>   * POSIX ACL supporting filesystems.
>   *
> - * Note that it's currently valid for @type to be 0 if a directory is created.
> - * Filesystems raise that flag individually and we need to check whether each
> - * filesystem can deal with receiving S_IFDIR from the vfs before we enforce a
> - * non-zero type.
> - *
>   * Returns: mode to be passed to the filesystem
>   */
>  static inline umode_t vfs_prepare_mode(struct mnt_idmap *idmap,
> @@ -5256,7 +5251,7 @@ struct dentry *vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  	if (!dir->i_op->mkdir)
>  		goto err;
>  
> -	mode = vfs_prepare_mode(idmap, dir, mode, S_IRWXUGO | S_ISVTX, 0);
> +	mode = vfs_prepare_mode(idmap, dir, mode, S_IRWXUGO | S_ISVTX, S_IFDIR);
>  	error = security_inode_mkdir(dir, dentry, mode);
>  	if (error)
>  		goto err;
> diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> index 1dbcfd163ff0..369dfd56c1e6 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -2282,6 +2282,13 @@ struct dentry *cifs_mkdir(struct mnt_idmap *idmap, struct inode *inode,
>  	const char *full_path;
>  	void *page;
>  
> +	/*
> +	 * vfs_mkdir() now passes S_IFDIR in @mode, but @mode is forwarded
> +	 * verbatim to the server and in the past only contained permission
> +	 * bits. Strip the type bit until SMB is verified to deal with it.
> +	 */
> +	mode &= ~S_IFDIR;
> +
>  	cifs_dbg(FYI, "In cifs_mkdir, mode = %04ho inode = 0x%p\n",
>  		 mode, inode);
>  
> -- 
> 2.54.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR