Re: [PATCH v2 2/4] fs: add FS_IOC_FSSETXATTRAT and FS_IOC_FSGETXATTRAT

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, May 20, 2024 at 06:46:21PM +0200, Andrey Albershteyn wrote:
> XFS has project quotas which could be attached to a directory. All
> new inodes in these directories inherit project ID set on parent
> directory.
> 
> The project is created from userspace by opening and calling
> FS_IOC_FSSETXATTR on each inode. This is not possible for special
> files such as FIFO, SOCK, BLK etc. as opening them returns a special
> inode from VFS. Therefore, some inodes are left with empty project
> ID. Those inodes then are not shown in the quota accounting but
> still exist in the directory.
> 
> This patch adds two new ioctls which allows userspace, such as
> xfs_quota, to set project ID on special files by using parent
> directory to open FS inode. This will let xfs_quota set ID on all
> inodes and also reset it when project is removed. Also, as
> vfs_fileattr_set() is now will called on special files too, let's
> forbid any other attributes except projid and nextents (symlink can
> have one).
> 
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> ---
>  fs/ioctl.c              | 93 +++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/fs.h | 11 +++++
>  2 files changed, 104 insertions(+)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 1d5abfdf0f22..3e3aacb6ea6e 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -22,6 +22,7 @@
>  #include <linux/mount.h>
>  #include <linux/fscrypt.h>
>  #include <linux/fileattr.h>
> +#include <linux/namei.h>
>  
>  #include "internal.h"
>  
> @@ -647,6 +648,19 @@ static int fileattr_set_prepare(struct inode *inode,
>  	if (fa->fsx_cowextsize == 0)
>  		fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
>  
> +	/*
> +	 * The only use case for special files is to set project ID, forbid any
> +	 * other attributes
> +	 */
> +	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
> +		if (fa->fsx_xflags & ~FS_XFLAG_PROJINHERIT)

When would PROJINHERIT be set on a non-reg/non-dir file?

> +			return -EINVAL;
> +		if (!S_ISLNK(inode->i_mode) && fa->fsx_nextents)

FS_IOC_FSSETXATTR doesn't enforce anything for fsx_nextents for any
other type of file, does it?

> +			return -EINVAL;
> +		if (fa->fsx_extsize || fa->fsx_cowextsize)
> +			return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -763,6 +777,79 @@ static int ioctl_fssetxattr(struct file *file, void __user *argp)
>  	return err;
>  }
>  
> +static int ioctl_fsgetxattrat(struct file *file, void __user *argp)
> +{
> +	struct path filepath;
> +	struct fsxattrat fsxat;
> +	struct fileattr fa;
> +	int error;
> +
> +	if (!S_ISDIR(file_inode(file)->i_mode))
> +		return -EBADF;
> +
> +	if (copy_from_user(&fsxat, argp, sizeof(struct fsxattrat)))
> +		return -EFAULT;
> +
> +	error = user_path_at(fsxat.dfd, fsxat.path, 0, &filepath);
> +	if (error)
> +		return error;
> +
> +	error = vfs_fileattr_get(filepath.dentry, &fa);
> +	if (error) {
> +		path_put(&filepath);
> +		return error;
> +	}
> +
> +	fsxat.fsx.fsx_xflags = fa.fsx_xflags;
> +	fsxat.fsx.fsx_extsize = fa.fsx_extsize;
> +	fsxat.fsx.fsx_nextents = fa.fsx_nextents;
> +	fsxat.fsx.fsx_projid = fa.fsx_projid;
> +	fsxat.fsx.fsx_cowextsize = fa.fsx_cowextsize;
> +
> +	if (copy_to_user(argp, &fsxat, sizeof(struct fsxattrat)))
> +		error = -EFAULT;
> +
> +	path_put(&filepath);
> +	return error;
> +}
> +
> +static int ioctl_fssetxattrat(struct file *file, void __user *argp)
> +{
> +	struct mnt_idmap *idmap = file_mnt_idmap(file);
> +	struct fsxattrat fsxat;
> +	struct path filepath;
> +	struct fileattr fa;
> +	int error;
> +
> +	if (!S_ISDIR(file_inode(file)->i_mode))
> +		return -EBADF;
> +
> +	if (copy_from_user(&fsxat, argp, sizeof(struct fsxattrat)))
> +		return -EFAULT;
> +
> +	error = user_path_at(fsxat.dfd, fsxat.path, 0, &filepath);
> +	if (error)
> +		return error;
> +
> +	error = mnt_want_write(filepath.mnt);
> +	if (error) {
> +		path_put(&filepath);
> +		return error;

This could be a goto to the path_put below.

> +	}
> +
> +	fileattr_fill_xflags(&fa, fsxat.fsx.fsx_xflags);
> +	fa.fsx_extsize = fsxat.fsx.fsx_extsize;
> +	fa.fsx_nextents = fsxat.fsx.fsx_nextents;
> +	fa.fsx_projid = fsxat.fsx.fsx_projid;
> +	fa.fsx_cowextsize = fsxat.fsx.fsx_cowextsize;
> +	fa.fsx_valid = true;
> +
> +	error = vfs_fileattr_set(idmap, filepath.dentry, &fa);

Why not pass &fsxat.fsx directly to vfs_fileattr_set?

> +	mnt_drop_write(filepath.mnt);
> +	path_put(&filepath);
> +	return error;
> +}
> +
>  static int ioctl_getfsuuid(struct file *file, void __user *argp)
>  {
>  	struct super_block *sb = file_inode(file)->i_sb;
> @@ -872,6 +959,12 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd,
>  	case FS_IOC_FSSETXATTR:
>  		return ioctl_fssetxattr(filp, argp);
>  
> +	case FS_IOC_FSGETXATTRAT:
> +		return ioctl_fsgetxattrat(filp, argp);
> +
> +	case FS_IOC_FSSETXATTRAT:
> +		return ioctl_fssetxattrat(filp, argp);
> +
>  	case FS_IOC_GETFSUUID:
>  		return ioctl_getfsuuid(filp, argp);
>  
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 45e4e64fd664..f8cd8d7bf35d 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -139,6 +139,15 @@ struct fsxattr {
>  	unsigned char	fsx_pad[8];
>  };
>  
> +/*
> + * Structure passed to FS_IOC_FSGETXATTRAT/FS_IOC_FSSETXATTRAT
> + */
> +struct fsxattrat {
> +	struct fsxattr	fsx;		/* XATTR to get/set */
> +	__u32		dfd;		/* parent dir */
> +	const char	__user *path;

As I mentioned last time[1], embedding a pointer in an ioctl structure
creates porting problems because pointer sizes vary between process
personalities, so the size of struct fsxattrat will vary and lead to
copy_to/from_user overflows.


[1] https://lore.kernel.org/linux-xfs/20240509232517.GR360919@frogsfrogsfrogs/

--D

> +};
> +
>  /*
>   * Flags for the fsx_xflags field
>   */
> @@ -231,6 +240,8 @@ struct fsxattr {
>  #define FS_IOC32_SETVERSION		_IOW('v', 2, int)
>  #define FS_IOC_FSGETXATTR		_IOR('X', 31, struct fsxattr)
>  #define FS_IOC_FSSETXATTR		_IOW('X', 32, struct fsxattr)
> +#define FS_IOC_FSGETXATTRAT		_IOR('X', 33, struct fsxattrat)
> +#define FS_IOC_FSSETXATTRAT		_IOW('X', 34, struct fsxattrat)
>  #define FS_IOC_GETFSLABEL		_IOR(0x94, 49, char[FSLABEL_MAX])
>  #define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
>  /* Returns the external filesystem UUID, the same one blkid returns */
> -- 
> 2.42.0
> 
>