On 2024-05-09 16:34:06, Darrick J. Wong wrote:
> On Thu, May 09, 2024 at 05:14:59PM +0200, Andrey Albershteyn wrote:
> > As XFS didn't have ioctls for special files setting an inode
> > extended attributes was rejected for them in xfs_fileattr_set().
> > Same applies for reading.
> >
> > With XFS's project quota directories this is necessary. When project
> > is setup, xfs_quota opens and calls FS_IOC_SETFSXATTR on every inode
> > in the directory. However, special files are skipped due to open()
> > returning a special inode for them. So, they don't even get to this
> > check.
> >
> > The further patch introduces XFS_IOC_SETFSXATTRAT which will call
> > xfs_fileattr_set/get() on a special file. This patch add handling of
> > setting xflags and project ID for special files.
> >
> > Signed-off-by: Andrey Albershteyn <aalbersh@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_ioctl.c | 96 ++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 92 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index f0117188f302..515c9b4b862d 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -459,9 +459,6 @@ xfs_fileattr_get(
> > {
> > struct xfs_inode *ip = XFS_I(d_inode(dentry));
> >
> > - if (d_is_special(dentry))
> > - return -ENOTTY;
> > -
> > xfs_ilock(ip, XFS_ILOCK_SHARED);
> > xfs_fill_fsxattr(ip, XFS_DATA_FORK, fa);
> > xfs_iunlock(ip, XFS_ILOCK_SHARED);
> > @@ -721,6 +718,97 @@ xfs_ioctl_setattr_check_projid(
> > return 0;
> > }
> >
> > +static int
> > +xfs_fileattr_spec_set(
> > + struct mnt_idmap *idmap,
> > + struct dentry *dentry,
> > + struct fileattr *fa)
> > +{
> > + struct xfs_inode *ip = XFS_I(d_inode(dentry));
> > + struct xfs_mount *mp = ip->i_mount;
> > + struct xfs_trans *tp;
> > + struct xfs_dquot *pdqp = NULL;
> > + struct xfs_dquot *olddquot = NULL;
> > + int error;
> > +
> > + if (!fa->fsx_valid)
> > + return -EOPNOTSUPP;
> > +
> > + if (fa->fsx_extsize ||
> > + fa->fsx_nextents ||
> > + fa->fsx_cowextsize)
> > + return -EOPNOTSUPP;
> > +
> > + error = xfs_ioctl_setattr_check_projid(ip, fa);
> > + if (error)
> > + return error;
> > +
> > + /*
> > + * If disk quotas is on, we make sure that the dquots do exist on disk,
> > + * before we start any other transactions. Trying to do this later
> > + * is messy. We don't care to take a readlock to look at the ids
> > + * in inode here, because we can't hold it across the trans_reserve.
> > + * If the IDs do change before we take the ilock, we're covered
> > + * because the i_*dquot fields will get updated anyway.
> > + */
> > + if (fa->fsx_valid && XFS_IS_QUOTA_ON(mp)) {
>
> Didn't we already check fsx_valid?
oh, right
>
> Also, what's different about the behavior of setxattr on special files
> (vs. directories and regular files) such that we need a separate function?
> Is it to disable the ability to set the extent size hints or the xflags?
yes, that's it, and the function looks a bit easier to follow for me
here
>
> --D
>
> > + error = xfs_qm_vop_dqalloc(ip, VFS_I(ip)->i_uid,
> > + VFS_I(ip)->i_gid, fa->fsx_projid,
> > + XFS_QMOPT_PQUOTA, NULL, NULL, &pdqp);
> > + if (error)
> > + return error;
> > + }
> > +
> > + tp = xfs_ioctl_setattr_get_trans(ip, pdqp);
> > + if (IS_ERR(tp)) {
> > + error = PTR_ERR(tp);
> > + goto error_free_dquots;
> > + }
> > +
> > + error = xfs_ioctl_setattr_xflags(tp, ip, fa);
> > + if (error)
> > + goto error_trans_cancel;
> > +
> > + /*
> > + * Change file ownership. Must be the owner or privileged. CAP_FSETID
> > + * overrides the following restrictions:
> > + *
> > + * The set-user-ID and set-group-ID bits of a file will be cleared upon
> > + * successful return from chown()
> > + */
> > +
> > + if ((VFS_I(ip)->i_mode & (S_ISUID | S_ISGID)) &&
> > + !capable_wrt_inode_uidgid(idmap, VFS_I(ip), CAP_FSETID))
> > + VFS_I(ip)->i_mode &= ~(S_ISUID | S_ISGID);
> > +
> > + /* Change the ownerships and register project quota modifications */
> > + if (ip->i_projid != fa->fsx_projid) {
> > + if (XFS_IS_PQUOTA_ON(mp)) {
> > + olddquot =
> > + xfs_qm_vop_chown(tp, ip, &ip->i_pdquot, pdqp);
> > + }
> > + ip->i_projid = fa->fsx_projid;
> > + }
> > +
> > + error = xfs_trans_commit(tp);
> > +
> > + /*
> > + * Release any dquot(s) the inode had kept before chown.
> > + */
> > + xfs_qm_dqrele(olddquot);
> > + xfs_qm_dqrele(pdqp);
> > +
> > + return error;
> > +
> > +error_trans_cancel:
> > + xfs_trans_cancel(tp);
> > +error_free_dquots:
> > + xfs_qm_dqrele(pdqp);
> > + return error;
> > +
> > + return 0;
> > +}
> > +
> > int
> > xfs_fileattr_set(
> > struct mnt_idmap *idmap,
> > @@ -737,7 +825,7 @@ xfs_fileattr_set(
> > trace_xfs_ioctl_setattr(ip);
> >
> > if (d_is_special(dentry))
> > - return -ENOTTY;
> > + return xfs_fileattr_spec_set(idmap, dentry, fa);
> >
> > if (!fa->fsx_valid) {
> > if (fa->flags & ~(FS_IMMUTABLE_FL | FS_APPEND_FL |
> > --
> > 2.42.0
> >
> >
>
--
- Andrey
