[PATCH v2 0/2] porting the GETFSUUID ioctl to xfs

[PATCH v2 0/2] porting the GETFSUUID ioctl to xfs

[Catherine Hoang]

Hi all,

This patch aims to hoist the ext4 get/set ioctls to the VFS so we have a
common interface for tools such as coreutils. The second patch adds support
for FS_IOC_GETFSUUID in xfs (with FS_IOC_SETFSUUID planned for future patches).

Comments and feedback appreciated!

Catherine

Catherine Hoang (2):
  fs: hoist get/set UUID ioctls
  xfs: add FS_IOC_GETFSUUID ioctl

 fs/ext4/ext4.h          | 13 ++-----------
 fs/xfs/xfs_ioctl.c      | 36 ++++++++++++++++++++++++++++++++++++
 include/uapi/linux/fs.h | 11 +++++++++++
 3 files changed, 49 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] fs: hoist get/set UUID ioctls

[Catherine Hoang]

Hoist the EXT4_IOC_[GS]ETFSUUID ioctls so that they can be used by all
filesystems. This allows us to have a common interface for tools such as
coreutils.

Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
---
 fs/ext4/ext4.h          | 13 ++-----------
 include/uapi/linux/fs.h | 11 +++++++++++
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 8d5453852f98..b200302a3732 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -722,8 +722,8 @@ enum {
 #define EXT4_IOC_GETSTATE		_IOW('f', 41, __u32)
 #define EXT4_IOC_GET_ES_CACHE		_IOWR('f', 42, struct fiemap)
 #define EXT4_IOC_CHECKPOINT		_IOW('f', 43, __u32)
-#define EXT4_IOC_GETFSUUID		_IOR('f', 44, struct fsuuid)
-#define EXT4_IOC_SETFSUUID		_IOW('f', 44, struct fsuuid)
+#define EXT4_IOC_GETFSUUID		FS_IOC_GETFSUUID
+#define EXT4_IOC_SETFSUUID		FS_IOC_SETFSUUID
 
 #define EXT4_IOC_SHUTDOWN _IOR ('X', 125, __u32)
 
@@ -753,15 +753,6 @@ enum {
 						EXT4_IOC_CHECKPOINT_FLAG_ZEROOUT | \
 						EXT4_IOC_CHECKPOINT_FLAG_DRY_RUN)
 
-/*
- * Structure for EXT4_IOC_GETFSUUID/EXT4_IOC_SETFSUUID
- */
-struct fsuuid {
-	__u32       fsu_len;
-	__u32       fsu_flags;
-	__u8        fsu_uuid[];
-};
-
 #if defined(__KERNEL__) && defined(CONFIG_COMPAT)
 /*
  * ioctl commands in 32 bit emulation
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index b7b56871029c..63b925444592 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -121,6 +121,15 @@ struct fsxattr {
 	unsigned char	fsx_pad[8];
 };
 
+/*
+ * Structure for FS_IOC_GETFSUUID/FS_IOC_SETFSUUID
+ */
+struct fsuuid {
+	__u32       fsu_len;
+	__u32       fsu_flags;
+	__u8        fsu_uuid[];
+};
+
 /*
  * Flags for the fsx_xflags field
  */
@@ -215,6 +224,8 @@ struct fsxattr {
 #define FS_IOC_FSSETXATTR		_IOW('X', 32, struct fsxattr)
 #define FS_IOC_GETFSLABEL		_IOR(0x94, 49, char[FSLABEL_MAX])
 #define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
+#define FS_IOC_GETFSUUID		_IOR('f', 44, struct fsuuid)
+#define FS_IOC_SETFSUUID		_IOW('f', 44, struct fsuuid)
 
 /*
  * Inode flags (FS_IOC_GETFLAGS / FS_IOC_SETFLAGS)
-- 
2.25.1

Re: [PATCH v2 1/2] fs: hoist get/set UUID ioctls

[Dave Chinner]

On Fri, Nov 18, 2022 at 01:14:07PM -0800, Catherine Hoang wrote:
> Hoist the EXT4_IOC_[GS]ETFSUUID ioctls so that they can be used by all
> filesystems. This allows us to have a common interface for tools such as
> coreutils.
> 
> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/ext4/ext4.h          | 13 ++-----------
>  include/uapi/linux/fs.h | 11 +++++++++++
>  2 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 8d5453852f98..b200302a3732 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -722,8 +722,8 @@ enum {
>  #define EXT4_IOC_GETSTATE		_IOW('f', 41, __u32)
>  #define EXT4_IOC_GET_ES_CACHE		_IOWR('f', 42, struct fiemap)
>  #define EXT4_IOC_CHECKPOINT		_IOW('f', 43, __u32)
> -#define EXT4_IOC_GETFSUUID		_IOR('f', 44, struct fsuuid)
> -#define EXT4_IOC_SETFSUUID		_IOW('f', 44, struct fsuuid)
> +#define EXT4_IOC_GETFSUUID		FS_IOC_GETFSUUID
> +#define EXT4_IOC_SETFSUUID		FS_IOC_SETFSUUID
>  
>  #define EXT4_IOC_SHUTDOWN _IOR ('X', 125, __u32)
>  
> @@ -753,15 +753,6 @@ enum {
>  						EXT4_IOC_CHECKPOINT_FLAG_ZEROOUT | \
>  						EXT4_IOC_CHECKPOINT_FLAG_DRY_RUN)
>  
> -/*
> - * Structure for EXT4_IOC_GETFSUUID/EXT4_IOC_SETFSUUID
> - */
> -struct fsuuid {
> -	__u32       fsu_len;
> -	__u32       fsu_flags;
> -	__u8        fsu_uuid[];
> -};
> -
>  #if defined(__KERNEL__) && defined(CONFIG_COMPAT)
>  /*
>   * ioctl commands in 32 bit emulation
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index b7b56871029c..63b925444592 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -121,6 +121,15 @@ struct fsxattr {
>  	unsigned char	fsx_pad[8];
>  };
>  
> +/*
> + * Structure for FS_IOC_GETFSUUID/FS_IOC_SETFSUUID
> + */
> +struct fsuuid {
> +	__u32       fsu_len;
> +	__u32       fsu_flags;
> +	__u8        fsu_uuid[];
> +};

As I pointed out in my last comments, flex arrays in user APIs are
really unfriendly, because it means the structures have to be
dynamically allocated and can't be put on the stack. This makes the
obvious use of the API (i.e. a local stack struct fsuuid
declaration) dangerous to users.

Also, UUIDs are 16 bytes long - always have been, always will be.
So why does this API need to support -variable length UUIDs-? If
this is intended for use with other types of filesystem IDs (e.g.
GUIDs), then it needs to be named differently and it needs to have
a man page written for it to explain what it contains....

Shouldn't these landmines get fixed before we promote the API to
being a VFS-wide operation?

Also, if this is VFS wide, then why do we need filesystem specific
implementations to retreive the UUID? After all, the VFS superblock
has the public filesystem UUID in it (i.e. sb->s_uuid), and so we
should just have a single ioctl implementation that reads out the
sb->s_uuid. Yes, Setting the UUID is a different matter altogether
because filesystems need to change on-disk stuff, but we don't need
to reimplement retreiving sb->s_uuid in every filesystem...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH v2 2/2] xfs: add FS_IOC_GETFSUUID ioctl

[Catherine Hoang]

Add a new ioctl to retrieve the UUID of a mounted xfs filesystem. This is a
precursor to adding the SETFSUUID ioctl.

Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
---
 fs/xfs/xfs_ioctl.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 1f783e979629..cf77715afe9e 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1865,6 +1865,39 @@ xfs_fs_eofblocks_from_user(
 	return 0;
 }
 
+static int
+xfs_ioctl_getuuid(
+	struct xfs_mount	*mp,
+	struct fsuuid __user	*ufsuuid)
+{
+	struct fsuuid		fsuuid;
+	__u8			uuid[UUID_SIZE];
+
+	if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
+		return -EFAULT;
+
+	if (fsuuid.fsu_len == 0) {
+		fsuuid.fsu_len = UUID_SIZE;
+		if (copy_to_user(&ufsuuid->fsu_len, &fsuuid.fsu_len,
+					sizeof(fsuuid.fsu_len)))
+			return -EFAULT;
+		return 0;
+	}
+
+	if (fsuuid.fsu_len < UUID_SIZE || fsuuid.fsu_flags != 0)
+		return -EINVAL;
+
+	spin_lock(&mp->m_sb_lock);
+	memcpy(uuid, &mp->m_sb.sb_uuid, UUID_SIZE);
+	spin_unlock(&mp->m_sb_lock);
+
+	fsuuid.fsu_len = UUID_SIZE;
+	if (copy_to_user(ufsuuid, &fsuuid, sizeof(fsuuid)) ||
+	    copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE))
+		return -EFAULT;
+	return 0;
+}
+
 /*
  * 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.
@@ -2153,6 +2186,9 @@ xfs_file_ioctl(
 		return error;
 	}
 
+	case FS_IOC_GETFSUUID:
+		return xfs_ioctl_getuuid(mp, arg);
+
 	default:
 		return -ENOTTY;
 	}
-- 
2.25.1

Re: [PATCH v2 2/2] xfs: add FS_IOC_GETFSUUID ioctl

[Darrick J. Wong]

On Fri, Nov 18, 2022 at 01:14:08PM -0800, Catherine Hoang wrote:
> Add a new ioctl to retrieve the UUID of a mounted xfs filesystem. This is a
> precursor to adding the SETFSUUID ioctl.
> 
> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

LGTM,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_ioctl.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 1f783e979629..cf77715afe9e 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1865,6 +1865,39 @@ xfs_fs_eofblocks_from_user(
>  	return 0;
>  }
>  
> +static int
> +xfs_ioctl_getuuid(
> +	struct xfs_mount	*mp,
> +	struct fsuuid __user	*ufsuuid)
> +{
> +	struct fsuuid		fsuuid;
> +	__u8			uuid[UUID_SIZE];
> +
> +	if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
> +		return -EFAULT;
> +
> +	if (fsuuid.fsu_len == 0) {
> +		fsuuid.fsu_len = UUID_SIZE;
> +		if (copy_to_user(&ufsuuid->fsu_len, &fsuuid.fsu_len,
> +					sizeof(fsuuid.fsu_len)))
> +			return -EFAULT;
> +		return 0;
> +	}
> +
> +	if (fsuuid.fsu_len < UUID_SIZE || fsuuid.fsu_flags != 0)
> +		return -EINVAL;
> +
> +	spin_lock(&mp->m_sb_lock);
> +	memcpy(uuid, &mp->m_sb.sb_uuid, UUID_SIZE);
> +	spin_unlock(&mp->m_sb_lock);
> +
> +	fsuuid.fsu_len = UUID_SIZE;
> +	if (copy_to_user(ufsuuid, &fsuuid, sizeof(fsuuid)) ||
> +	    copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE))
> +		return -EFAULT;
> +	return 0;
> +}
> +
>  /*
>   * 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.
> @@ -2153,6 +2186,9 @@ xfs_file_ioctl(
>  		return error;
>  	}
>  
> +	case FS_IOC_GETFSUUID:
> +		return xfs_ioctl_getuuid(mp, arg);
> +
>  	default:
>  		return -ENOTTY;
>  	}
> -- 
> 2.25.1
> 

Re: [PATCH v2 2/2] xfs: add FS_IOC_GETFSUUID ioctl

[Dave Chinner]

On Fri, Nov 18, 2022 at 01:14:08PM -0800, Catherine Hoang wrote:
> Add a new ioctl to retrieve the UUID of a mounted xfs filesystem. This is a
> precursor to adding the SETFSUUID ioctl.
> 
> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/xfs_ioctl.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 1f783e979629..cf77715afe9e 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1865,6 +1865,39 @@ xfs_fs_eofblocks_from_user(
>  	return 0;
>  }
>  
> +static int
> +xfs_ioctl_getuuid(
> +	struct xfs_mount	*mp,
> +	struct fsuuid __user	*ufsuuid)
> +{
> +	struct fsuuid		fsuuid;
> +	__u8			uuid[UUID_SIZE];

uuid_t, please, not an open coded uuid_t.

> +
> +	if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
> +		return -EFAULT;

I still think this failing to copy the flex array member and then
having to declare a local uuid buffer is an ugly wart, not just on
the API side of things.

> +	if (fsuuid.fsu_len == 0) {
> +		fsuuid.fsu_len = UUID_SIZE;

XFS uses sizeof(uuid_t) for the size of it's uuids, not UUID_SIZE.

> +		if (copy_to_user(&ufsuuid->fsu_len, &fsuuid.fsu_len,
> +					sizeof(fsuuid.fsu_len)))
> +			return -EFAULT;
> +		return 0;
> +	}
> +
> +	if (fsuuid.fsu_len < UUID_SIZE || fsuuid.fsu_flags != 0)
> +		return -EINVAL;
> +
> +	spin_lock(&mp->m_sb_lock);
> +	memcpy(uuid, &mp->m_sb.sb_uuid, UUID_SIZE);
> +	spin_unlock(&mp->m_sb_lock);

Hmmmm. Shouldn't we be promoting xfs_fs_get_uuid() to xfs_super.c
(without the pNFS warning!) and calling that here, rather than open
coding another "get the XFS superblock UUID" function here?

i.e.
	if (fsuuid.fsu_flags != 0)
		return -EINVAL;

	error = xfs_fs_get_uuid(&mp->m_sb, uuid, &fsuuid.fsu_len, NULL);
	if (error)
		return -EINVAL;

Also, uuid_copy()?

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 2/2] xfs: add FS_IOC_GETFSUUID ioctl

[Darrick J. Wong]

On Tue, Nov 22, 2022 at 08:02:23AM +1100, Dave Chinner wrote:
> On Fri, Nov 18, 2022 at 01:14:08PM -0800, Catherine Hoang wrote:
> > Add a new ioctl to retrieve the UUID of a mounted xfs filesystem. This is a
> > precursor to adding the SETFSUUID ioctl.
> > 
> > Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> > Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> > ---
> >  fs/xfs/xfs_ioctl.c | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 1f783e979629..cf77715afe9e 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1865,6 +1865,39 @@ xfs_fs_eofblocks_from_user(
> >  	return 0;
> >  }
> >  
> > +static int
> > +xfs_ioctl_getuuid(
> > +	struct xfs_mount	*mp,
> > +	struct fsuuid __user	*ufsuuid)
> > +{
> > +	struct fsuuid		fsuuid;
> > +	__u8			uuid[UUID_SIZE];
> 
> uuid_t, please, not an open coded uuid_t.
> 
> > +
> > +	if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
> > +		return -EFAULT;
> 
> I still think this failing to copy the flex array member and then
> having to declare a local uuid buffer is an ugly wart, not just on
> the API side of things.
> 
> > +	if (fsuuid.fsu_len == 0) {
> > +		fsuuid.fsu_len = UUID_SIZE;
> 
> XFS uses sizeof(uuid_t) for the size of it's uuids, not UUID_SIZE.
> 
> > +		if (copy_to_user(&ufsuuid->fsu_len, &fsuuid.fsu_len,
> > +					sizeof(fsuuid.fsu_len)))
> > +			return -EFAULT;
> > +		return 0;
> > +	}
> > +
> > +	if (fsuuid.fsu_len < UUID_SIZE || fsuuid.fsu_flags != 0)
> > +		return -EINVAL;
> > +
> > +	spin_lock(&mp->m_sb_lock);
> > +	memcpy(uuid, &mp->m_sb.sb_uuid, UUID_SIZE);
> > +	spin_unlock(&mp->m_sb_lock);
> 
> Hmmmm. Shouldn't we be promoting xfs_fs_get_uuid() to xfs_super.c
> (without the pNFS warning!) and calling that here, rather than open
> coding another "get the XFS superblock UUID" function here?

I disagree that it's worth the effort to create a helper function to
wrap four lines of code, particularly since the pnfs code has this extra
weird wart of returning the byte offset(?) of the uuid location.

> i.e.
> 	if (fsuuid.fsu_flags != 0)
> 		return -EINVAL;
> 
> 	error = xfs_fs_get_uuid(&mp->m_sb, uuid, &fsuuid.fsu_len, NULL);
> 	if (error)
> 		return -EINVAL;
> 
> Also, uuid_copy()?

Why does xfs_fs_get_uuid use memcpy then?  Did the compiler reject the
u8* -> uuid_t * type conversion?

Alternately there's export_uuid().

--D

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

Re: [PATCH v2 2/2] xfs: add FS_IOC_GETFSUUID ioctl

[Dave Chinner]

On Mon, Nov 21, 2022 at 02:18:47PM -0800, Darrick J. Wong wrote:
> On Tue, Nov 22, 2022 at 08:02:23AM +1100, Dave Chinner wrote:
> > On Fri, Nov 18, 2022 at 01:14:08PM -0800, Catherine Hoang wrote:
> > i.e.
> > 	if (fsuuid.fsu_flags != 0)
> > 		return -EINVAL;
> > 
> > 	error = xfs_fs_get_uuid(&mp->m_sb, uuid, &fsuuid.fsu_len, NULL);
> > 	if (error)
> > 		return -EINVAL;
> > 
> > Also, uuid_copy()?
> 
> Why does xfs_fs_get_uuid use memcpy then?  Did the compiler reject the
> u8* -> uuid_t * type conversion?

No idea, I've completely forgotten about the reasons for the code
being written that way.

These days people seem to care about making the compiler do all the
type checking and type conversions for us. The use of UUIDs and
various types within XFS has been quite ad hoc, so I'm just
suggesting that we improve it somewhat.

Using types and APIs that mean we don't have to code the length of
UUIDs everywhere seems like a Good Idea for newly written code...

> Alternately there's export_uuid().

But we don't need that if we use uuid_t for all the local
representations of the UUID....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 0/2] porting the GETFSUUID ioctl to xfs

[Dave Chinner]

On Fri, Nov 18, 2022 at 01:14:06PM -0800, Catherine Hoang wrote:
> Hi all,
> 
> This patch aims to hoist the ext4 get/set ioctls to the VFS so we have a
> common interface for tools such as coreutils. The second patch adds support
> for FS_IOC_GETFSUUID in xfs (with FS_IOC_SETFSUUID planned for future patches).

FWIW, the next version needs to be cc'd to
linux-fsdevel@vger.kernel.org because we're talking about lifting an
ioctl to the VFS layer...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com