[fix fsdevel address]
On Mon, May 20, 2024 at 10:03 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Mon, May 20, 2024 at 7:46 PM Andrey Albershteyn <aalbersh@xxxxxxxxxx> 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)
> > + return -EINVAL;
> > + if (!S_ISLNK(inode->i_mode) && fa->fsx_nextents)
> > + 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;
>
> So the *only* thing that is done with the fd of the ioctl is to verify
> that it is a directory fd - there is no verification that this fd is on the
> same sb as the path to act on.
>
> Was this the intention? It does not make a lot of sense to me
> and AFAIK there is no precedent to an API like this.
>
> There are ioctls that operate on the filesystem using any
> fd on that fs, such as FS_IOC_GETFS{UUID,SYSFSPATH}
> and maybe the closest example to what you are trying to add
> XFS_IOC_BULKSTAT.
>
> Trying to think of a saner API for this - perhaps pass an O_PATH
> fd without any filename in struct fsxattrat, saving you also the
> headache of passing a variable length string in an ioctl.
>
> Then atfile = fdget_raw(fsxat.atfd) and verify that atfile->f_path
> and file->f_path are on the same sb before proceeding to operate
> on atfile->f_path.dentry.
>
> The (maybe more sane) alternative is to add syscalls instead of
> ioctls, but I won't force you to go there...
>
> What do everyone think?
>
> Thanks,
> Amir.
