On 2024-05-20 10:51:59, Darrick J. Wong wrote:
> 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@xxxxxxxxxx>
> > ---
> > 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?
Oh I thought it's set on all inodes in projects anyway, but looks
like it's dropped for files in xfs_flags2diflags(). The xfs_quota
just set it for each inode so I just haven't checked it. I will drop
this check and change xfs_quota to not set PROJINHERIT for special
files.
> > + 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?
yes, it doesn't enforce anything, as I described in the comment I
tried to reject any other uses. But symlink has fsx_nextents == 1 so
get + set call will fail.
>
> > + 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?
vfs_fileattr_set() takes fileattr and there won't be fsx_valid, no? I
suppose they aren't aligned
>
> > + 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.
Oh right, sorry, totally forgot about that
>
>
> [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
> >
> >
>
--
- Andrey
