On 2024-05-09 16:25:17, Darrick J. Wong wrote:
> On Thu, May 09, 2024 at 05:15:00PM +0200, Andrey Albershteyn wrote:
> > XFS has project quotas which could be attached to a directory. All
> > new inodes in these directories inherit project ID.
> >
> > 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 return special
> > inode from VFS. Therefore, some inodes are left with empty project
> > ID.
> >
> > This patch adds new XFS ioctl which allows userspace, such as
> > xfs_quota, to set project ID on special files. This will let
> > xfs_quota set ID on all inodes and also reset it when project is
> > removed.
> >
> > Signed-off-by: Andrey Albershteyn <aalbersh@xxxxxxxxxx>
> > ---
> > fs/xfs/libxfs/xfs_fs.h | 11 +++++
> > fs/xfs/xfs_ioctl.c | 86 ++++++++++++++++++++++++++++++++++++++++
> > include/linux/fileattr.h | 2 +-
> > 3 files changed, 98 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > index 97996cb79aaa..f68e98005d4b 100644
> > --- a/fs/xfs/libxfs/xfs_fs.h
> > +++ b/fs/xfs/libxfs/xfs_fs.h
> > @@ -670,6 +670,15 @@ typedef struct xfs_swapext
> > struct xfs_bstat sx_stat; /* stat of target b4 copy */
> > } xfs_swapext_t;
> >
> > +/*
> > + * Structure passed to XFS_IOC_GETFSXATTRAT/XFS_IOC_GETFSXATTRAT
> > + */
> > +struct xfs_xattrat_req {
> > + struct fsxattr __user *fsx; /* XATTR to get/set */
>
> Shouldn't this fsxattr object be embedded directly into xfs_xattrat_req?
> That's one less pointer to mess with.
Yes, I think it can, will change that
>
> > + __u32 dfd; /* parent dir */
> > + const char __user *path;
>
> Fugly wart: passing in a pointer as part of a ioctl structure means that
> you also have to implement an ioctl32.c wrapper because pointer sizes
> are not the same across the personalities that the kernel can run (e.g.
> i386 on an x64 kernel).
aha, I see, thanks! Will look into it
>
> Unfortunately the only way I know of to work around the ioctl32 crud is
> to declare this as a __u64 field, employ a bunch of uintptr_t casting to
> shut up gcc, and pretend that pointers never exceed 64 bits.
>
> > +};
> > +
> > /*
> > * Flags for going down operation
> > */
> > @@ -997,6 +1006,8 @@ struct xfs_getparents_by_handle {
> > #define XFS_IOC_BULKSTAT _IOR ('X', 127, struct xfs_bulkstat_req)
> > #define XFS_IOC_INUMBERS _IOR ('X', 128, struct xfs_inumbers_req)
> > #define XFS_IOC_EXCHANGE_RANGE _IOWR('X', 129, struct xfs_exchange_range)
> > +#define XFS_IOC_GETFSXATTRAT _IOR ('X', 130, struct xfs_xattrat_req)
> > +#define XFS_IOC_SETFSXATTRAT _IOW ('X', 131, struct xfs_xattrat_req)
>
> These really ought to be defined in the VFS alongside
> FS_IOC_FSGETXATTR, not in XFS.
>
> > /* XFS_IOC_GETFSUUID ---------- deprecated 140 */
> >
> >
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 515c9b4b862d..d54dba9128a0 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1408,6 +1408,74 @@ xfs_ioctl_fs_counts(
> > return 0;
> > }
> >
> > +static int
> > +xfs_xattrat_get(
> > + struct file *dir,
> > + const char __user *pathname,
> > + struct xfs_xattrat_req *xreq)
> > +{
> > + struct path filepath;
> > + struct xfs_inode *ip;
> > + struct fileattr fa;
> > + int error = -EBADF;
> > +
> > + memset(&fa, 0, sizeof(struct fileattr));
> > +
> > + if (!S_ISDIR(file_inode(dir)->i_mode))
> > + return error;
> > +
> > + error = user_path_at(xreq->dfd, pathname, 0, &filepath);
> > + if (error)
> > + return error;
> > +
> > + ip = XFS_I(filepath.dentry->d_inode);
>
> Can we trust that this path points to an XFS inode? Or even the same
> filesystem as the ioctl fd? I think if you put the user_path_at part in
> the VFS, you could use the resulting filepath.dentry to call the regular
> ->fileattr_[gs]et functions, couldn't you?
Yeah, I missed that this could be a cross mount point, I will move
it to VFS.
>
> --D
>
> > +
> > + xfs_ilock(ip, XFS_ILOCK_SHARED);
> > + xfs_fill_fsxattr(ip, XFS_DATA_FORK, &fa);
> > + xfs_iunlock(ip, XFS_ILOCK_SHARED);
> > +
> > + error = copy_fsxattr_to_user(&fa, xreq->fsx);
> > +
> > + path_put(&filepath);
> > + return error;
> > +}
> > +
> > +static int
> > +xfs_xattrat_set(
> > + struct file *dir,
> > + const char __user *pathname,
> > + struct xfs_xattrat_req *xreq)
> > +{
> > + struct fileattr fa;
> > + struct path filepath;
> > + struct mnt_idmap *idmap = file_mnt_idmap(dir);
> > + int error = -EBADF;
> > +
> > + if (!S_ISDIR(file_inode(dir)->i_mode))
> > + return error;
> > +
> > + error = copy_fsxattr_from_user(&fa, xreq->fsx);
> > + if (error)
> > + return error;
> > +
> > + error = user_path_at(xreq->dfd, pathname, 0, &filepath);
> > + if (error)
> > + return error;
> > +
> > + error = mnt_want_write(filepath.mnt);
> > + if (error) {
> > + path_put(&filepath);
> > + return error;
> > + }
> > +
> > + fa.fsx_valid = true;
> > + error = vfs_fileattr_set(idmap, filepath.dentry, &fa);
> > +
> > + mnt_drop_write(filepath.mnt);
> > + path_put(&filepath);
> > + return error;
> > +}
> > +
> > /*
> > * These long-unused ioctls were removed from the official ioctl API in 5.17,
> > * but retain these definitions so that we can log warnings about them.
> > @@ -1652,6 +1720,24 @@ xfs_file_ioctl(
> > sb_end_write(mp->m_super);
> > return error;
> > }
> > + case XFS_IOC_GETFSXATTRAT: {
> > + struct xfs_xattrat_req xreq;
> > +
> > + if (copy_from_user(&xreq, arg, sizeof(struct xfs_xattrat_req)))
> > + return -EFAULT;
> > +
> > + error = xfs_xattrat_get(filp, xreq.path, &xreq);
> > + return error;
> > + }
> > + case XFS_IOC_SETFSXATTRAT: {
> > + struct xfs_xattrat_req xreq;
> > +
> > + if (copy_from_user(&xreq, arg, sizeof(struct xfs_xattrat_req)))
> > + return -EFAULT;
> > +
> > + error = xfs_xattrat_set(filp, xreq.path, &xreq);
> > + return error;
> > + }
> >
> > case XFS_IOC_EXCHANGE_RANGE:
> > return xfs_ioc_exchange_range(filp, arg);
> > diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
> > index 3c4f8c75abc0..8598e94b530b 100644
> > --- a/include/linux/fileattr.h
> > +++ b/include/linux/fileattr.h
> > @@ -34,7 +34,7 @@ struct fileattr {
> > };
> >
> > int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa);
> > -int copy_fsxattr_from_user(struct fileattr *fa, struct fsxattr __user *ufa)
> > +int copy_fsxattr_from_user(struct fileattr *fa, struct fsxattr __user *ufa);
> >
> > void fileattr_fill_xflags(struct fileattr *fa, u32 xflags);
> > void fileattr_fill_flags(struct fileattr *fa, u32 flags);
> > --
> > 2.42.0
> >
> >
>
--
- Andrey
