diff mbox series

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

Message ID 20221118211408.72796-3-catherine.hoang@oracle.com (mailing list archive)
State New, archived
Headers show
Series porting the GETFSUUID ioctl to xfs | expand

Commit Message

Catherine Hoang Nov. 18, 2022, 9:14 p.m. UTC 
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(+)

Comments

Darrick J. Wong Nov. 21, 2022, 6:05 p.m. UTC | #1 
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
>
Dave Chinner Nov. 21, 2022, 9:02 p.m. UTC | #2 
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.
Darrick J. Wong Nov. 21, 2022, 10:18 p.m. UTC | #3 
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
Dave Chinner Nov. 21, 2022, 11:20 p.m. UTC | #4 
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.
diff mbox series

Patch

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