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.
