[PATCH 0/4] Introduce XFS_IOC_SETFSXATTRAT/XFS_IOC_GETFSXATTRAT ioctls

[PATCH 0/4] Introduce XFS_IOC_SETFSXATTRAT/XFS_IOC_GETFSXATTRAT ioctls

[Andrey Albershteyn]

Hi all,

This patchset adds new ioctl for setting/getting inode extended
attributes on special files. This roots from xfs_quota not being
able to set project IDs on special files [1]. New XFS ioctl can be
used directly on filesystem inodes to set project ID.

Corresponding xfsprogs patch will follow.

[1]: https://lore.kernel.org/linux-xfs/20240314170700.352845-3-aalbersh@redhat.com/

Andrey Albershteyn (4):
  fs: export copy_fsxattr_from_user()
  xfs: allow renames of project-less inodes
  xfs: allow setting xattrs on special files
  xfs: add XFS_IOC_SETFSXATTRAT and XFS_IOC_GETFSXATTRAT

 fs/ioctl.c               |  11 ++-
 fs/xfs/libxfs/xfs_fs.h   |  11 +++
 fs/xfs/xfs_inode.c       |  15 +++-
 fs/xfs/xfs_ioctl.c       | 182 ++++++++++++++++++++++++++++++++++++++-
 include/linux/fileattr.h |   1 +
 5 files changed, 212 insertions(+), 8 deletions(-)

-- 
2.42.0

[PATCH 1/4] fs: export copy_fsxattr_from_user()

[Andrey Albershteyn]

Export copy_fsxattr_from_user(). This function will be used in
further patch by XFS in XFS_IOC_SETFSXATTRAT ioctl.

Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
---
 fs/ioctl.c               | 11 +++++++++--
 include/linux/fileattr.h |  1 +
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 1d5abfdf0f22..2017c928194e 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -559,8 +559,14 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
 }
 EXPORT_SYMBOL(copy_fsxattr_to_user);
 
-static int copy_fsxattr_from_user(struct fileattr *fa,
-				  struct fsxattr __user *ufa)
+/**
+ * copy_fsxattr_from_user - copy fsxattr from userspace.
+ * @fa:		fileattr pointer
+ * @ufa:	fsxattr user pointer
+ *
+ * Return: 0 on success, or -EFAULT on failure.
+ */
+int copy_fsxattr_from_user(struct fileattr *fa, struct fsxattr __user *ufa)
 {
 	struct fsxattr xfa;
 
@@ -575,6 +581,7 @@ static int copy_fsxattr_from_user(struct fileattr *fa,
 
 	return 0;
 }
+EXPORT_SYMBOL(copy_fsxattr_from_user);
 
 /*
  * Generic function to check FS_IOC_FSSETXATTR/FS_IOC_SETFLAGS values and reject
diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
index 47c05a9851d0..3c4f8c75abc0 100644
--- a/include/linux/fileattr.h
+++ b/include/linux/fileattr.h
@@ -34,6 +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)
 
 void fileattr_fill_xflags(struct fileattr *fa, u32 xflags);
 void fileattr_fill_flags(struct fileattr *fa, u32 flags);
-- 
2.42.0

[PATCH 2/4] xfs: allow renames of project-less inodes

[Andrey Albershteyn]

Identical problem as worked around in commit e23d7e82b707 ("xfs:
allow cross-linking special files without project quota") exists
with renames. Renaming special file without project ID is not
possible inside PROJINHERIT directory.

Special files inodes can not have project ID set from userspace and
are skipped during initial project setup. Those inodes are left
project-less in the project directory. New inodes created after
project initialization do have an ID. Creating hard links or
renaming those project-less inodes then fails on different ID check.

Add workaround to allow renames of special files without project ID.

Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
---
 fs/xfs/xfs_inode.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 58fb7a5062e1..508113515eec 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3275,8 +3275,19 @@ xfs_rename(
 	 */
 	if (unlikely((target_dp->i_diflags & XFS_DIFLAG_PROJINHERIT) &&
 		     target_dp->i_projid != src_ip->i_projid)) {
-		error = -EXDEV;
-		goto out_trans_cancel;
+		/*
+		 * Project quota setup skips special files which can
+		 * leave inodes in a PROJINHERIT directory without a
+		 * project ID set. We need to allow renames to be made
+		 * to these "project-less" inodes because userspace
+		 * expects them to succeed after project ID setup,
+		 * but everything else should be rejected.
+		 */
+		if (!special_file(VFS_I(src_ip)->i_mode) ||
+		    src_ip->i_projid != 0) {
+			error = -EXDEV;
+			goto out_trans_cancel;
+		}
 	}
 
 	/* RENAME_EXCHANGE is unique from here on. */
-- 
2.42.0

[PATCH 3/4] xfs: allow setting xattrs on special files

[Andrey Albershteyn]

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)) {
+		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

[PATCH 4/4] xfs: add XFS_IOC_SETFSXATTRAT and XFS_IOC_GETFSXATTRAT

[Andrey Albershteyn]

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@redhat.com>
---
 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 */
+	__u32		dfd;			/* parent dir */
+	const char	__user *path;
+};
+
 /*
  * 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)
 /*	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);
+
+	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

Re: [PATCH 4/4] xfs: add XFS_IOC_SETFSXATTRAT and XFS_IOC_GETFSXATTRAT

[Darrick J. Wong]

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@redhat.com>
> ---
>  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.

> +	__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).

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?

--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
> 
> 

Re: [PATCH 2/4] xfs: allow renames of project-less inodes

[Darrick J. Wong]

On Thu, May 09, 2024 at 05:14:58PM +0200, Andrey Albershteyn wrote:
> Identical problem as worked around in commit e23d7e82b707 ("xfs:
> allow cross-linking special files without project quota") exists
> with renames. Renaming special file without project ID is not
> possible inside PROJINHERIT directory.
> 
> Special files inodes can not have project ID set from userspace and
> are skipped during initial project setup. Those inodes are left
> project-less in the project directory. New inodes created after
> project initialization do have an ID. Creating hard links or
> renaming those project-less inodes then fails on different ID check.
> 
> Add workaround to allow renames of special files without project ID.
> 
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 58fb7a5062e1..508113515eec 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3275,8 +3275,19 @@ xfs_rename(
>  	 */
>  	if (unlikely((target_dp->i_diflags & XFS_DIFLAG_PROJINHERIT) &&
>  		     target_dp->i_projid != src_ip->i_projid)) {
> -		error = -EXDEV;
> -		goto out_trans_cancel;
> +		/*
> +		 * Project quota setup skips special files which can
> +		 * leave inodes in a PROJINHERIT directory without a
> +		 * project ID set. We need to allow renames to be made
> +		 * to these "project-less" inodes because userspace
> +		 * expects them to succeed after project ID setup,
> +		 * but everything else should be rejected.
> +		 */
> +		if (!special_file(VFS_I(src_ip)->i_mode) ||
> +		    src_ip->i_projid != 0) {
> +			error = -EXDEV;
> +			goto out_trans_cancel;
> +		}
>  	}

Should this be a shared helper called by xfs_rename and xfs_link?

--D

>  
>  	/* RENAME_EXCHANGE is unique from here on. */
> -- 
> 2.42.0
> 
> 

Re: [PATCH 3/4] xfs: allow setting xattrs on special files

[Darrick J. Wong]

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
> 
> 

Re: [PATCH 4/4] xfs: add XFS_IOC_SETFSXATTRAT and XFS_IOC_GETFSXATTRAT

[Dave Chinner]

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@redhat.com>

This really should be a VFS layer file ioctl (like
FS_IOC_FSSETXATTR). Path resolution needs to be done before we call
into the destination filesystem as the path may cross mount points
and take us outside the filesytem that the parent dir points to.

IOWs, there should be no need to change anything in XFS to support
FS_IOC_[GS]ETFSXATTRAT() as once the path has been resolved to a
dentry at the VFS we can just call the existing
vfs_fileattr_get()/vfs_fileattr_set() functions to get/set the
xattr.

The changes to allow/deny setting attributes on special files
then goes into fileattr_set_prepare(), and with that I don't think
there's much that needs changing in XFS at all...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/4] xfs: add XFS_IOC_SETFSXATTRAT and XFS_IOC_GETFSXATTRAT

[Christoph Hellwig]

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.

Having these ioctls in XFS while the non-AT ones are in the VFS feels
really odd.  What is the reason to make them XFS-specific?

Re: [PATCH 4/4] xfs: add XFS_IOC_SETFSXATTRAT and XFS_IOC_GETFSXATTRAT

[Andrey Albershteyn]

On 2024-05-10 09:55:35, Dave Chinner 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@redhat.com>
> 
> This really should be a VFS layer file ioctl (like
> FS_IOC_FSSETXATTR). Path resolution needs to be done before we call
> into the destination filesystem as the path may cross mount points
> and take us outside the filesytem that the parent dir points to.
> 

I see, thanks! I haven't thought about cross mount points.

> IOWs, there should be no need to change anything in XFS to support
> FS_IOC_[GS]ETFSXATTRAT() as once the path has been resolved to a
> dentry at the VFS we can just call the existing
> vfs_fileattr_get()/vfs_fileattr_set() functions to get/set the
> xattr.
> 
> The changes to allow/deny setting attributes on special files
> then goes into fileattr_set_prepare(), and with that I don't think
> there's much that needs changing in XFS at all...
> 

Yeah, then this would probably will do it.

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

-- 
- Andrey

Re: [PATCH 2/4] xfs: allow renames of project-less inodes

[Andrey Albershteyn]

On 2024-05-09 16:28:05, Darrick J. Wong wrote:
> On Thu, May 09, 2024 at 05:14:58PM +0200, Andrey Albershteyn wrote:
> > Identical problem as worked around in commit e23d7e82b707 ("xfs:
> > allow cross-linking special files without project quota") exists
> > with renames. Renaming special file without project ID is not
> > possible inside PROJINHERIT directory.
> > 
> > Special files inodes can not have project ID set from userspace and
> > are skipped during initial project setup. Those inodes are left
> > project-less in the project directory. New inodes created after
> > project initialization do have an ID. Creating hard links or
> > renaming those project-less inodes then fails on different ID check.
> > 
> > Add workaround to allow renames of special files without project ID.
> > 
> > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> > ---
> >  fs/xfs/xfs_inode.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 58fb7a5062e1..508113515eec 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -3275,8 +3275,19 @@ xfs_rename(
> >  	 */
> >  	if (unlikely((target_dp->i_diflags & XFS_DIFLAG_PROJINHERIT) &&
> >  		     target_dp->i_projid != src_ip->i_projid)) {
> > -		error = -EXDEV;
> > -		goto out_trans_cancel;
> > +		/*
> > +		 * Project quota setup skips special files which can
> > +		 * leave inodes in a PROJINHERIT directory without a
> > +		 * project ID set. We need to allow renames to be made
> > +		 * to these "project-less" inodes because userspace
> > +		 * expects them to succeed after project ID setup,
> > +		 * but everything else should be rejected.
> > +		 */
> > +		if (!special_file(VFS_I(src_ip)->i_mode) ||
> > +		    src_ip->i_projid != 0) {
> > +			error = -EXDEV;
> > +			goto out_trans_cancel;
> > +		}
> >  	}
> 
> Should this be a shared helper called by xfs_rename and xfs_link?

yeah, it can be

> 
> --D
> 
> >  
> >  	/* RENAME_EXCHANGE is unique from here on. */
> > -- 
> > 2.42.0
> > 
> > 
> 

-- 
- Andrey

Re: [PATCH 3/4] xfs: allow setting xattrs on special files

[Andrey Albershteyn]

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
> > 
> > 
> 

-- 
- Andrey

Re: [PATCH 4/4] xfs: add XFS_IOC_SETFSXATTRAT and XFS_IOC_GETFSXATTRAT

[Andrey Albershteyn]

On 2024-05-09 21:58:34, Christoph Hellwig 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.
> 
> Having these ioctls in XFS while the non-AT ones are in the VFS feels
> really odd.  What is the reason to make them XFS-specific?
> 

I just don't see other uses for these in other fs, and in xfs it's
just for project quota. So, I put them in XFS. But based on other
feedback I will move them to VFS.

-- 
- Andrey

Re: [PATCH 4/4] xfs: add XFS_IOC_SETFSXATTRAT and XFS_IOC_GETFSXATTRAT

[Andrey Albershteyn]

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@redhat.com>
> > ---
> >  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

Re: [PATCH 4/4] xfs: add XFS_IOC_SETFSXATTRAT and XFS_IOC_GETFSXATTRAT

[Darrick J. Wong]

On Fri, May 10, 2024 at 11:50:28AM +0200, Andrey Albershteyn wrote:
> On 2024-05-09 21:58:34, Christoph Hellwig 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.
> > 
> > Having these ioctls in XFS while the non-AT ones are in the VFS feels
> > really odd.  What is the reason to make them XFS-specific?
> > 
> 
> I just don't see other uses for these in other fs, and in xfs it's
> just for project quota. So, I put them in XFS. But based on other
> feedback I will move them to VFS.

Yeah, ext4 has project quota now too. ;)

--D

> -- 
> - Andrey
> 
>