| Message ID | 20240509151459.3622910-5-aalbersh@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Introduce XFS_IOC_SETFSXATTRAT/XFS_IOC_GETFSXATTRAT ioctls | expand |
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@redhat.com> > --- > 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? 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? --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 > >
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@redhat.com> > > --- > > 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 > > > > >
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)) { + 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 |
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@redhat.com> --- fs/xfs/xfs_ioctl.c | 96 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 92 insertions(+), 4 deletions(-)