[PATCH v1] xfs_spaceman: add fsuuid command

[PATCH v1] xfs_spaceman: add fsuuid command

[Catherine Hoang]

Add support for the fsuuid command to retrieve the UUID of a mounted
filesystem.

Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
---
 spaceman/Makefile |  4 +--
 spaceman/fsuuid.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
 spaceman/init.c   |  1 +
 spaceman/space.h  |  1 +
 4 files changed, 67 insertions(+), 2 deletions(-)
 create mode 100644 spaceman/fsuuid.c

diff --git a/spaceman/Makefile b/spaceman/Makefile
index 1f048d54..901e4e6d 100644
--- a/spaceman/Makefile
+++ b/spaceman/Makefile
@@ -7,10 +7,10 @@ include $(TOPDIR)/include/builddefs
 
 LTCOMMAND = xfs_spaceman
 HFILES = init.h space.h
-CFILES = info.c init.c file.c health.c prealloc.c trim.c
+CFILES = info.c init.c file.c health.c prealloc.c trim.c fsuuid.c
 LSRCFILES = xfs_info.sh
 
-LLDLIBS = $(LIBXCMD) $(LIBFROG)
+LLDLIBS = $(LIBXCMD) $(LIBFROG) $(LIBUUID)
 LTDEPENDENCIES = $(LIBXCMD) $(LIBFROG)
 LLDFLAGS = -static
 
diff --git a/spaceman/fsuuid.c b/spaceman/fsuuid.c
new file mode 100644
index 00000000..be12c1ad
--- /dev/null
+++ b/spaceman/fsuuid.c
@@ -0,0 +1,63 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2022 Oracle.
+ * All Rights Reserved.
+ */
+
+#include "libxfs.h"
+#include "libfrog/fsgeom.h"
+#include "libfrog/paths.h"
+#include "command.h"
+#include "init.h"
+#include "space.h"
+#include <sys/ioctl.h>
+
+#ifndef FS_IOC_GETFSUUID
+#define FS_IOC_GETFSUUID	_IOR('f', 44, struct fsuuid)
+#define UUID_SIZE 16
+struct fsuuid {
+    __u32   fsu_len;
+    __u32   fsu_flags;
+    __u8    fsu_uuid[];
+};
+#endif
+
+static cmdinfo_t fsuuid_cmd;
+
+static int
+fsuuid_f(
+	int		argc,
+	char		**argv)
+{
+	struct fsuuid	fsuuid;
+	int		error;
+	char		bp[40];
+
+	fsuuid.fsu_len = UUID_SIZE;
+	fsuuid.fsu_flags = 0;
+
+	error = ioctl(file->xfd.fd, FS_IOC_GETFSUUID, &fsuuid);
+
+	if (error) {
+		perror("fsuuid");
+		exitcode = 1;
+	} else {
+		platform_uuid_unparse((uuid_t *)fsuuid.fsu_uuid, bp);
+		printf("UUID = %s\n", bp);
+	}
+
+	return 0;
+}
+
+void
+fsuuid_init(void)
+{
+	fsuuid_cmd.name = "fsuuid";
+	fsuuid_cmd.cfunc = fsuuid_f;
+	fsuuid_cmd.argmin = 0;
+	fsuuid_cmd.argmax = 0;
+	fsuuid_cmd.flags = CMD_FLAG_ONESHOT;
+	fsuuid_cmd.oneline = _("get mounted filesystem UUID");
+
+	add_command(&fsuuid_cmd);
+}
diff --git a/spaceman/init.c b/spaceman/init.c
index cf1ff3cb..efe1bf9b 100644
--- a/spaceman/init.c
+++ b/spaceman/init.c
@@ -35,6 +35,7 @@ init_commands(void)
 	trim_init();
 	freesp_init();
 	health_init();
+	fsuuid_init();
 }
 
 static int
diff --git a/spaceman/space.h b/spaceman/space.h
index 723209ed..dcbdca08 100644
--- a/spaceman/space.h
+++ b/spaceman/space.h
@@ -33,5 +33,6 @@ extern void	freesp_init(void);
 #endif
 extern void	info_init(void);
 extern void	health_init(void);
+extern void	fsuuid_init(void);
 
 #endif /* XFS_SPACEMAN_SPACE_H_ */
-- 
2.25.1

Re: [PATCH v1] xfs_spaceman: add fsuuid command

[Dave Chinner]

On Wed, Nov 09, 2022 at 02:23:35PM -0800, Catherine Hoang wrote:
> Add support for the fsuuid command to retrieve the UUID of a mounted
> filesystem.
> 
> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>

Not looking at the code, just the high level CLI interface issues.

That is, xfs_admin already has user interfaces to get/set the
filesystem UUID on XFS filesystems. Further, xfs_spaceman is for XFS
filesystem SPACE MANagement, not filesystem identification
operations.

xfs_admin is CLI interface that aggregates various admin utilities
such as identification (UUID and label) management.  If we need to
implement a generic VFS ioctl for online UUID retreival, xfs_io is
generally the place to put it because then it can be used by fstests
to run on other filesytems.

We can then wrap xfs_admin around the xfs_io command as needed. i.e.
use 'xfs_io -c fsuuid /mnt/pt' if the filesystem is mounted, 'xfs_db
-c uuid /path/to/device' if the filesystem is not mounted, and we
can make sure that both the xfs_db and xfs_io commands have the same
output....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v1] xfs_spaceman: add fsuuid command

[Catherine Hoang]

> On Nov 11, 2022, at 1:05 PM, Dave Chinner <david@fromorbit.com> wrote:
> 
> On Wed, Nov 09, 2022 at 02:23:35PM -0800, Catherine Hoang wrote:
>> Add support for the fsuuid command to retrieve the UUID of a mounted
>> filesystem.
>> 
>> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> 
> Not looking at the code, just the high level CLI interface issues.
> 
> That is, xfs_admin already has user interfaces to get/set the
> filesystem UUID on XFS filesystems. Further, xfs_spaceman is for XFS
> filesystem SPACE MANagement, not filesystem identification
> operations.
> 
> xfs_admin is CLI interface that aggregates various admin utilities
> such as identification (UUID and label) management.  If we need to
> implement a generic VFS ioctl for online UUID retreival, xfs_io is
> generally the place to put it because then it can be used by fstests
> to run on other filesytems.

Ok, that makes sense. I can move this to xfs_io
> 
> We can then wrap xfs_admin around the xfs_io command as needed. i.e.
> use 'xfs_io -c fsuuid /mnt/pt' if the filesystem is mounted, 'xfs_db
> -c uuid /path/to/device' if the filesystem is not mounted, and we
> can make sure that both the xfs_db and xfs_io commands have the same
> output....

Yes, that is the eventual goal for all of this. Just figured I’d send out
this fsuuid command first to get some feedback before making any more
changes. Thanks!
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH v1] xfs_spaceman: add fsuuid command

[Allison Henderson]

On Wed, 2022-11-09 at 14:23 -0800, Catherine Hoang wrote:
> Add support for the fsuuid command to retrieve the UUID of a mounted
> filesystem.
> 
> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
I think it looks good
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  spaceman/Makefile |  4 +--
>  spaceman/fsuuid.c | 63
> +++++++++++++++++++++++++++++++++++++++++++++++
>  spaceman/init.c   |  1 +
>  spaceman/space.h  |  1 +
>  4 files changed, 67 insertions(+), 2 deletions(-)
>  create mode 100644 spaceman/fsuuid.c
> 
> diff --git a/spaceman/Makefile b/spaceman/Makefile
> index 1f048d54..901e4e6d 100644
> --- a/spaceman/Makefile
> +++ b/spaceman/Makefile
> @@ -7,10 +7,10 @@ include $(TOPDIR)/include/builddefs
>  
>  LTCOMMAND = xfs_spaceman
>  HFILES = init.h space.h
> -CFILES = info.c init.c file.c health.c prealloc.c trim.c
> +CFILES = info.c init.c file.c health.c prealloc.c trim.c fsuuid.c
>  LSRCFILES = xfs_info.sh
>  
> -LLDLIBS = $(LIBXCMD) $(LIBFROG)
> +LLDLIBS = $(LIBXCMD) $(LIBFROG) $(LIBUUID)
>  LTDEPENDENCIES = $(LIBXCMD) $(LIBFROG)
>  LLDFLAGS = -static
>  
> diff --git a/spaceman/fsuuid.c b/spaceman/fsuuid.c
> new file mode 100644
> index 00000000..be12c1ad
> --- /dev/null
> +++ b/spaceman/fsuuid.c
> @@ -0,0 +1,63 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Oracle.
> + * All Rights Reserved.
> + */
> +
> +#include "libxfs.h"
> +#include "libfrog/fsgeom.h"
> +#include "libfrog/paths.h"
> +#include "command.h"
> +#include "init.h"
> +#include "space.h"
> +#include <sys/ioctl.h>
> +
> +#ifndef FS_IOC_GETFSUUID
> +#define FS_IOC_GETFSUUID       _IOR('f', 44, struct fsuuid)
> +#define UUID_SIZE 16
> +struct fsuuid {
> +    __u32   fsu_len;
> +    __u32   fsu_flags;
> +    __u8    fsu_uuid[];
> +};
> +#endif
> +
> +static cmdinfo_t fsuuid_cmd;
> +
> +static int
> +fsuuid_f(
> +       int             argc,
> +       char            **argv)
> +{
> +       struct fsuuid   fsuuid;
> +       int             error;
> +       char            bp[40];
> +
> +       fsuuid.fsu_len = UUID_SIZE;
> +       fsuuid.fsu_flags = 0;
> +
> +       error = ioctl(file->xfd.fd, FS_IOC_GETFSUUID, &fsuuid);
> +
> +       if (error) {
> +               perror("fsuuid");
> +               exitcode = 1;
> +       } else {
> +               platform_uuid_unparse((uuid_t *)fsuuid.fsu_uuid, bp);
> +               printf("UUID = %s\n", bp);
> +       }
> +
> +       return 0;
> +}
> +
> +void
> +fsuuid_init(void)
> +{
> +       fsuuid_cmd.name = "fsuuid";
> +       fsuuid_cmd.cfunc = fsuuid_f;
> +       fsuuid_cmd.argmin = 0;
> +       fsuuid_cmd.argmax = 0;
> +       fsuuid_cmd.flags = CMD_FLAG_ONESHOT;
> +       fsuuid_cmd.oneline = _("get mounted filesystem UUID");
> +
> +       add_command(&fsuuid_cmd);
> +}
> diff --git a/spaceman/init.c b/spaceman/init.c
> index cf1ff3cb..efe1bf9b 100644
> --- a/spaceman/init.c
> +++ b/spaceman/init.c
> @@ -35,6 +35,7 @@ init_commands(void)
>         trim_init();
>         freesp_init();
>         health_init();
> +       fsuuid_init();
>  }
>  
>  static int
> diff --git a/spaceman/space.h b/spaceman/space.h
> index 723209ed..dcbdca08 100644
> --- a/spaceman/space.h
> +++ b/spaceman/space.h
> @@ -33,5 +33,6 @@ extern void   freesp_init(void);
>  #endif
>  extern void    info_init(void);
>  extern void    health_init(void);
> +extern void    fsuuid_init(void);
>  
>  #endif /* XFS_SPACEMAN_SPACE_H_ */

Re: [PATCH v1] xfs_spaceman: add fsuuid command

[Darrick J. Wong]

On Wed, Nov 09, 2022 at 02:23:35PM -0800, Catherine Hoang wrote:
> Add support for the fsuuid command to retrieve the UUID of a mounted
> filesystem.
> 
> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> ---
>  spaceman/Makefile |  4 +--
>  spaceman/fsuuid.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
>  spaceman/init.c   |  1 +
>  spaceman/space.h  |  1 +
>  4 files changed, 67 insertions(+), 2 deletions(-)
>  create mode 100644 spaceman/fsuuid.c
> 
> diff --git a/spaceman/Makefile b/spaceman/Makefile
> index 1f048d54..901e4e6d 100644
> --- a/spaceman/Makefile
> +++ b/spaceman/Makefile
> @@ -7,10 +7,10 @@ include $(TOPDIR)/include/builddefs
>  
>  LTCOMMAND = xfs_spaceman
>  HFILES = init.h space.h
> -CFILES = info.c init.c file.c health.c prealloc.c trim.c
> +CFILES = info.c init.c file.c health.c prealloc.c trim.c fsuuid.c
>  LSRCFILES = xfs_info.sh
>  
> -LLDLIBS = $(LIBXCMD) $(LIBFROG)
> +LLDLIBS = $(LIBXCMD) $(LIBFROG) $(LIBUUID)
>  LTDEPENDENCIES = $(LIBXCMD) $(LIBFROG)
>  LLDFLAGS = -static
>  
> diff --git a/spaceman/fsuuid.c b/spaceman/fsuuid.c
> new file mode 100644
> index 00000000..be12c1ad
> --- /dev/null
> +++ b/spaceman/fsuuid.c
> @@ -0,0 +1,63 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Oracle.
> + * All Rights Reserved.
> + */
> +
> +#include "libxfs.h"
> +#include "libfrog/fsgeom.h"
> +#include "libfrog/paths.h"
> +#include "command.h"
> +#include "init.h"
> +#include "space.h"
> +#include <sys/ioctl.h>
> +
> +#ifndef FS_IOC_GETFSUUID
> +#define FS_IOC_GETFSUUID	_IOR('f', 44, struct fsuuid)
> +#define UUID_SIZE 16
> +struct fsuuid {
> +    __u32   fsu_len;
> +    __u32   fsu_flags;
> +    __u8    fsu_uuid[];

This is a flex array   ^^ which has no size.  struct fsuuid therefore
has a size of 8 bytes (i.e. enough to cover the two u32 fields) and no
more.  It's assumed that the caller will allocate the memory for
fsu_uuid...

> +};
> +#endif
> +
> +static cmdinfo_t fsuuid_cmd;
> +
> +static int
> +fsuuid_f(
> +	int		argc,
> +	char		**argv)
> +{
> +	struct fsuuid	fsuuid;
> +	int		error;

...which makes this usage a problem, because we've not reserved any
space on the stack to hold the UUID.  The kernel will blindly assume
that there are fsuuid.fsu_len bytes after fsuuid and write to them,
which will clobber something on the stack.

If you're really unlucky, the C compiler will put the fsuuid right
before the call frame, which is how stack smashing attacks work.  It
might also lay out bp[] immediately afterwards, which will give you
weird results as the unparse function overwrites its source buffer.  The
C compiler controls the stack layout, which means this can go bad in
subtle ways.

Either way, gcc complains about this (albeit in an opaque manner)...

In file included from ../include/xfs.h:9,
                 from ../include/libxfs.h:15,
                 from fsuuid.c:7:
In function ‘platform_uuid_unparse’,
    inlined from ‘fsuuid_f’ at fsuuid.c:45:3:
../include/xfs/linux.h:100:9: error: ‘uuid_unparse’ reading 16 bytes from a region of size 0 [-Werror=stringop-overread]
  100 |         uuid_unparse(*uu, buffer);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~
../include/xfs/linux.h: In function ‘fsuuid_f’:
../include/xfs/linux.h:100:9: note: referencing argument 1 of type ‘const unsigned char *’
In file included from ../include/xfs/linux.h:13,
                 from ../include/xfs.h:9,
                 from ../include/libxfs.h:15,
                 from fsuuid.c:7:
/usr/include/uuid/uuid.h:107:13: note: in a call to function ‘uuid_unparse’
  107 | extern void uuid_unparse(const uuid_t uu, char *out);
      |             ^~~~~~~~~~~~
cc1: all warnings being treated as errors

...so please allocate the struct fsuuid object dynamically.

--D

> +	char		bp[40];
> +
> +	fsuuid.fsu_len = UUID_SIZE;
> +	fsuuid.fsu_flags = 0;
> +
> +	error = ioctl(file->xfd.fd, FS_IOC_GETFSUUID, &fsuuid);
> +
> +	if (error) {
> +		perror("fsuuid");
> +		exitcode = 1;
> +	} else {
> +		platform_uuid_unparse((uuid_t *)fsuuid.fsu_uuid, bp);
> +		printf("UUID = %s\n", bp);
> +	}
> +
> +	return 0;
> +}
> +
> +void
> +fsuuid_init(void)
> +{
> +	fsuuid_cmd.name = "fsuuid";
> +	fsuuid_cmd.cfunc = fsuuid_f;
> +	fsuuid_cmd.argmin = 0;
> +	fsuuid_cmd.argmax = 0;
> +	fsuuid_cmd.flags = CMD_FLAG_ONESHOT;
> +	fsuuid_cmd.oneline = _("get mounted filesystem UUID");
> +
> +	add_command(&fsuuid_cmd);
> +}
> diff --git a/spaceman/init.c b/spaceman/init.c
> index cf1ff3cb..efe1bf9b 100644
> --- a/spaceman/init.c
> +++ b/spaceman/init.c
> @@ -35,6 +35,7 @@ init_commands(void)
>  	trim_init();
>  	freesp_init();
>  	health_init();
> +	fsuuid_init();
>  }
>  
>  static int
> diff --git a/spaceman/space.h b/spaceman/space.h
> index 723209ed..dcbdca08 100644
> --- a/spaceman/space.h
> +++ b/spaceman/space.h
> @@ -33,5 +33,6 @@ extern void	freesp_init(void);
>  #endif
>  extern void	info_init(void);
>  extern void	health_init(void);
> +extern void	fsuuid_init(void);
>  
>  #endif /* XFS_SPACEMAN_SPACE_H_ */
> -- 
> 2.25.1
> 

Re: [PATCH v1] xfs_spaceman: add fsuuid command

[Dave Chinner]

On Thu, Nov 17, 2022 at 12:37:33PM -0800, Darrick J. Wong wrote:
> On Wed, Nov 09, 2022 at 02:23:35PM -0800, Catherine Hoang wrote:
> > Add support for the fsuuid command to retrieve the UUID of a mounted
> > filesystem.
> > 
> > Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> > ---
> >  spaceman/Makefile |  4 +--
> >  spaceman/fsuuid.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
> >  spaceman/init.c   |  1 +
> >  spaceman/space.h  |  1 +
> >  4 files changed, 67 insertions(+), 2 deletions(-)
> >  create mode 100644 spaceman/fsuuid.c
> > 
> > diff --git a/spaceman/Makefile b/spaceman/Makefile
> > index 1f048d54..901e4e6d 100644
> > --- a/spaceman/Makefile
> > +++ b/spaceman/Makefile
> > @@ -7,10 +7,10 @@ include $(TOPDIR)/include/builddefs
> >  
> >  LTCOMMAND = xfs_spaceman
> >  HFILES = init.h space.h
> > -CFILES = info.c init.c file.c health.c prealloc.c trim.c
> > +CFILES = info.c init.c file.c health.c prealloc.c trim.c fsuuid.c
> >  LSRCFILES = xfs_info.sh
> >  
> > -LLDLIBS = $(LIBXCMD) $(LIBFROG)
> > +LLDLIBS = $(LIBXCMD) $(LIBFROG) $(LIBUUID)
> >  LTDEPENDENCIES = $(LIBXCMD) $(LIBFROG)
> >  LLDFLAGS = -static
> >  
> > diff --git a/spaceman/fsuuid.c b/spaceman/fsuuid.c
> > new file mode 100644
> > index 00000000..be12c1ad
> > --- /dev/null
> > +++ b/spaceman/fsuuid.c
> > @@ -0,0 +1,63 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2022 Oracle.
> > + * All Rights Reserved.
> > + */
> > +
> > +#include "libxfs.h"
> > +#include "libfrog/fsgeom.h"
> > +#include "libfrog/paths.h"
> > +#include "command.h"
> > +#include "init.h"
> > +#include "space.h"
> > +#include <sys/ioctl.h>
> > +
> > +#ifndef FS_IOC_GETFSUUID
> > +#define FS_IOC_GETFSUUID	_IOR('f', 44, struct fsuuid)
> > +#define UUID_SIZE 16
> > +struct fsuuid {
> > +    __u32   fsu_len;
> > +    __u32   fsu_flags;
> > +    __u8    fsu_uuid[];
> 
> This is a flex array   ^^ which has no size.  struct fsuuid therefore
> has a size of 8 bytes (i.e. enough to cover the two u32 fields) and no
> more.  It's assumed that the caller will allocate the memory for
> fsu_uuid...
> 
> > +};
> > +#endif
> > +
> > +static cmdinfo_t fsuuid_cmd;
> > +
> > +static int
> > +fsuuid_f(
> > +	int		argc,
> > +	char		**argv)
> > +{
> > +	struct fsuuid	fsuuid;
> > +	int		error;
> 
> ...which makes this usage a problem, because we've not reserved any
> space on the stack to hold the UUID.  The kernel will blindly assume
> that there are fsuuid.fsu_len bytes after fsuuid and write to them,
> which will clobber something on the stack.
> 
> If you're really unlucky, the C compiler will put the fsuuid right
> before the call frame, which is how stack smashing attacks work.  It
> might also lay out bp[] immediately afterwards, which will give you
> weird results as the unparse function overwrites its source buffer.  The
> C compiler controls the stack layout, which means this can go bad in
> subtle ways.
> 
> Either way, gcc complains about this (albeit in an opaque manner)...
> 
> In file included from ../include/xfs.h:9,
>                  from ../include/libxfs.h:15,
>                  from fsuuid.c:7:
> In function ‘platform_uuid_unparse’,
>     inlined from ‘fsuuid_f’ at fsuuid.c:45:3:
> ../include/xfs/linux.h:100:9: error: ‘uuid_unparse’ reading 16 bytes from a region of size 0 [-Werror=stringop-overread]
>   100 |         uuid_unparse(*uu, buffer);
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~
> ../include/xfs/linux.h: In function ‘fsuuid_f’:
> ../include/xfs/linux.h:100:9: note: referencing argument 1 of type ‘const unsigned char *’
> In file included from ../include/xfs/linux.h:13,
>                  from ../include/xfs.h:9,
>                  from ../include/libxfs.h:15,
>                  from fsuuid.c:7:
> /usr/include/uuid/uuid.h:107:13: note: in a call to function ‘uuid_unparse’
>   107 | extern void uuid_unparse(const uuid_t uu, char *out);
>       |             ^~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> ...so please allocate the struct fsuuid object dynamically.

So, follow common convention and you'll get it wrong, eh? That a
score of -4 on Rusty's API Design scale.

http://sweng.the-davies.net/Home/rustys-api-design-manifesto

Flex arrays in user APIs like this just look plain dangerous to me.

Really, this says that the FSUUID API should have a fixed length
buffer size defined in the API and the length used can be anything
up to the maximum.

We already have this being added for the ioctl API:

#define UUID_SIZE 16

So why isn't the API definition this:

struct fsuuid {
    __u32   fsu_len;
    __u32   fsu_flags;
    __u8    fsu_uuid[UUID_SIZE];
};

Or if we want to support larger ID structures:

#define MAX_FSUUID_SIZE 256

struct fsuuid {
    __u32   fsu_len;
    __u32   fsu_flags;
    __u8    fsu_uuid[MAX_FSUUID_SIZE];
};

Then the structure can be safely placed on the stack, which means
"the obvious use is (probably) the correct one" (a score of 7 on
Rusty's API Design scale). It also gives the kernel a fixed upper
bound that it can use to validate the incoming fsu_len variable
against...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v1] xfs_spaceman: add fsuuid command

[Darrick J. Wong]

On Fri, Nov 18, 2022 at 08:51:25AM +1100, Dave Chinner wrote:
> On Thu, Nov 17, 2022 at 12:37:33PM -0800, Darrick J. Wong wrote:
> > On Wed, Nov 09, 2022 at 02:23:35PM -0800, Catherine Hoang wrote:
> > > Add support for the fsuuid command to retrieve the UUID of a mounted
> > > filesystem.
> > > 
> > > Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> > > ---
> > >  spaceman/Makefile |  4 +--
> > >  spaceman/fsuuid.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  spaceman/init.c   |  1 +
> > >  spaceman/space.h  |  1 +
> > >  4 files changed, 67 insertions(+), 2 deletions(-)
> > >  create mode 100644 spaceman/fsuuid.c
> > > 
> > > diff --git a/spaceman/Makefile b/spaceman/Makefile
> > > index 1f048d54..901e4e6d 100644
> > > --- a/spaceman/Makefile
> > > +++ b/spaceman/Makefile
> > > @@ -7,10 +7,10 @@ include $(TOPDIR)/include/builddefs
> > >  
> > >  LTCOMMAND = xfs_spaceman
> > >  HFILES = init.h space.h
> > > -CFILES = info.c init.c file.c health.c prealloc.c trim.c
> > > +CFILES = info.c init.c file.c health.c prealloc.c trim.c fsuuid.c
> > >  LSRCFILES = xfs_info.sh
> > >  
> > > -LLDLIBS = $(LIBXCMD) $(LIBFROG)
> > > +LLDLIBS = $(LIBXCMD) $(LIBFROG) $(LIBUUID)
> > >  LTDEPENDENCIES = $(LIBXCMD) $(LIBFROG)
> > >  LLDFLAGS = -static
> > >  
> > > diff --git a/spaceman/fsuuid.c b/spaceman/fsuuid.c
> > > new file mode 100644
> > > index 00000000..be12c1ad
> > > --- /dev/null
> > > +++ b/spaceman/fsuuid.c
> > > @@ -0,0 +1,63 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2022 Oracle.
> > > + * All Rights Reserved.
> > > + */
> > > +
> > > +#include "libxfs.h"
> > > +#include "libfrog/fsgeom.h"
> > > +#include "libfrog/paths.h"
> > > +#include "command.h"
> > > +#include "init.h"
> > > +#include "space.h"
> > > +#include <sys/ioctl.h>
> > > +
> > > +#ifndef FS_IOC_GETFSUUID
> > > +#define FS_IOC_GETFSUUID	_IOR('f', 44, struct fsuuid)
> > > +#define UUID_SIZE 16
> > > +struct fsuuid {
> > > +    __u32   fsu_len;
> > > +    __u32   fsu_flags;
> > > +    __u8    fsu_uuid[];
> > 
> > This is a flex array   ^^ which has no size.  struct fsuuid therefore
> > has a size of 8 bytes (i.e. enough to cover the two u32 fields) and no
> > more.  It's assumed that the caller will allocate the memory for
> > fsu_uuid...
> > 
> > > +};
> > > +#endif
> > > +
> > > +static cmdinfo_t fsuuid_cmd;
> > > +
> > > +static int
> > > +fsuuid_f(
> > > +	int		argc,
> > > +	char		**argv)
> > > +{
> > > +	struct fsuuid	fsuuid;
> > > +	int		error;
> > 
> > ...which makes this usage a problem, because we've not reserved any
> > space on the stack to hold the UUID.  The kernel will blindly assume
> > that there are fsuuid.fsu_len bytes after fsuuid and write to them,
> > which will clobber something on the stack.
> > 
> > If you're really unlucky, the C compiler will put the fsuuid right
> > before the call frame, which is how stack smashing attacks work.  It
> > might also lay out bp[] immediately afterwards, which will give you
> > weird results as the unparse function overwrites its source buffer.  The
> > C compiler controls the stack layout, which means this can go bad in
> > subtle ways.
> > 
> > Either way, gcc complains about this (albeit in an opaque manner)...
> > 
> > In file included from ../include/xfs.h:9,
> >                  from ../include/libxfs.h:15,
> >                  from fsuuid.c:7:
> > In function ‘platform_uuid_unparse’,
> >     inlined from ‘fsuuid_f’ at fsuuid.c:45:3:
> > ../include/xfs/linux.h:100:9: error: ‘uuid_unparse’ reading 16 bytes from a region of size 0 [-Werror=stringop-overread]
> >   100 |         uuid_unparse(*uu, buffer);
> >       |         ^~~~~~~~~~~~~~~~~~~~~~~~~
> > ../include/xfs/linux.h: In function ‘fsuuid_f’:
> > ../include/xfs/linux.h:100:9: note: referencing argument 1 of type ‘const unsigned char *’
> > In file included from ../include/xfs/linux.h:13,
> >                  from ../include/xfs.h:9,
> >                  from ../include/libxfs.h:15,
> >                  from fsuuid.c:7:
> > /usr/include/uuid/uuid.h:107:13: note: in a call to function ‘uuid_unparse’
> >   107 | extern void uuid_unparse(const uuid_t uu, char *out);
> >       |             ^~~~~~~~~~~~
> > cc1: all warnings being treated as errors
> > 
> > ...so please allocate the struct fsuuid object dynamically.
> 
> So, follow common convention and you'll get it wrong, eh? That a
> score of -4 on Rusty's API Design scale.
> 
> http://sweng.the-davies.net/Home/rustys-api-design-manifesto
> 
> Flex arrays in user APIs like this just look plain dangerous to me.
> 
> Really, this says that the FSUUID API should have a fixed length
> buffer size defined in the API and the length used can be anything
> up to the maximum.
> 
> We already have this being added for the ioctl API:
> 
> #define UUID_SIZE 16
> 
> So why isn't the API definition this:
> 
> struct fsuuid {
>     __u32   fsu_len;
>     __u32   fsu_flags;
>     __u8    fsu_uuid[UUID_SIZE];
> };
> 
> Or if we want to support larger ID structures:
> 
> #define MAX_FSUUID_SIZE 256
> 
> struct fsuuid {
>     __u32   fsu_len;
>     __u32   fsu_flags;
>     __u8    fsu_uuid[MAX_FSUUID_SIZE];
> };
> 
> Then the structure can be safely placed on the stack, which means
> "the obvious use is (probably) the correct one" (a score of 7 on
> Rusty's API Design scale). It also gives the kernel a fixed upper
> bound that it can use to validate the incoming fsu_len variable
> against...

Too late now, this already shipped in 6.0.  Changing the struct size
would change the ioctl number, which is a totally new API.  This was
already discussed back in July on fsdevel/api.

--D

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

Re: [PATCH v1] xfs_spaceman: add fsuuid command

[Dave Chinner]

On Thu, Nov 17, 2022 at 03:58:06PM -0800, Darrick J. Wong wrote:
> On Fri, Nov 18, 2022 at 08:51:25AM +1100, Dave Chinner wrote:
> > On Thu, Nov 17, 2022 at 12:37:33PM -0800, Darrick J. Wong wrote:
> > > On Wed, Nov 09, 2022 at 02:23:35PM -0800, Catherine Hoang wrote:
> > > > Add support for the fsuuid command to retrieve the UUID of a mounted
> > > > filesystem.
> > > > 
> > > > Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> > > > ---
> > > >  spaceman/Makefile |  4 +--
> > > >  spaceman/fsuuid.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
> > > >  spaceman/init.c   |  1 +
> > > >  spaceman/space.h  |  1 +
> > > >  4 files changed, 67 insertions(+), 2 deletions(-)
> > > >  create mode 100644 spaceman/fsuuid.c
> > > > 
> > > > diff --git a/spaceman/Makefile b/spaceman/Makefile
> > > > index 1f048d54..901e4e6d 100644
> > > > --- a/spaceman/Makefile
> > > > +++ b/spaceman/Makefile
> > > > @@ -7,10 +7,10 @@ include $(TOPDIR)/include/builddefs
> > > >  
> > > >  LTCOMMAND = xfs_spaceman
> > > >  HFILES = init.h space.h
> > > > -CFILES = info.c init.c file.c health.c prealloc.c trim.c
> > > > +CFILES = info.c init.c file.c health.c prealloc.c trim.c fsuuid.c
> > > >  LSRCFILES = xfs_info.sh
> > > >  
> > > > -LLDLIBS = $(LIBXCMD) $(LIBFROG)
> > > > +LLDLIBS = $(LIBXCMD) $(LIBFROG) $(LIBUUID)
> > > >  LTDEPENDENCIES = $(LIBXCMD) $(LIBFROG)
> > > >  LLDFLAGS = -static
> > > >  
> > > > diff --git a/spaceman/fsuuid.c b/spaceman/fsuuid.c
> > > > new file mode 100644
> > > > index 00000000..be12c1ad
> > > > --- /dev/null
> > > > +++ b/spaceman/fsuuid.c
> > > > @@ -0,0 +1,63 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (c) 2022 Oracle.
> > > > + * All Rights Reserved.
> > > > + */
> > > > +
> > > > +#include "libxfs.h"
> > > > +#include "libfrog/fsgeom.h"
> > > > +#include "libfrog/paths.h"
> > > > +#include "command.h"
> > > > +#include "init.h"
> > > > +#include "space.h"
> > > > +#include <sys/ioctl.h>
> > > > +
> > > > +#ifndef FS_IOC_GETFSUUID
> > > > +#define FS_IOC_GETFSUUID	_IOR('f', 44, struct fsuuid)
> > > > +#define UUID_SIZE 16
> > > > +struct fsuuid {
> > > > +    __u32   fsu_len;
> > > > +    __u32   fsu_flags;
> > > > +    __u8    fsu_uuid[];
> > > 
> > > This is a flex array   ^^ which has no size.  struct fsuuid therefore
> > > has a size of 8 bytes (i.e. enough to cover the two u32 fields) and no
> > > more.  It's assumed that the caller will allocate the memory for
> > > fsu_uuid...
> > > 
> > > > +};
> > > > +#endif
> > > > +
> > > > +static cmdinfo_t fsuuid_cmd;
> > > > +
> > > > +static int
> > > > +fsuuid_f(
> > > > +	int		argc,
> > > > +	char		**argv)
> > > > +{
> > > > +	struct fsuuid	fsuuid;
> > > > +	int		error;
> > > 
> > > ...which makes this usage a problem, because we've not reserved any
> > > space on the stack to hold the UUID.  The kernel will blindly assume
> > > that there are fsuuid.fsu_len bytes after fsuuid and write to them,
> > > which will clobber something on the stack.
> > > 
> > > If you're really unlucky, the C compiler will put the fsuuid right
> > > before the call frame, which is how stack smashing attacks work.  It
> > > might also lay out bp[] immediately afterwards, which will give you
> > > weird results as the unparse function overwrites its source buffer.  The
> > > C compiler controls the stack layout, which means this can go bad in
> > > subtle ways.
> > > 
> > > Either way, gcc complains about this (albeit in an opaque manner)...
> > > 
> > > In file included from ../include/xfs.h:9,
> > >                  from ../include/libxfs.h:15,
> > >                  from fsuuid.c:7:
> > > In function ‘platform_uuid_unparse’,
> > >     inlined from ‘fsuuid_f’ at fsuuid.c:45:3:
> > > ../include/xfs/linux.h:100:9: error: ‘uuid_unparse’ reading 16 bytes from a region of size 0 [-Werror=stringop-overread]
> > >   100 |         uuid_unparse(*uu, buffer);
> > >       |         ^~~~~~~~~~~~~~~~~~~~~~~~~
> > > ../include/xfs/linux.h: In function ‘fsuuid_f’:
> > > ../include/xfs/linux.h:100:9: note: referencing argument 1 of type ‘const unsigned char *’
> > > In file included from ../include/xfs/linux.h:13,
> > >                  from ../include/xfs.h:9,
> > >                  from ../include/libxfs.h:15,
> > >                  from fsuuid.c:7:
> > > /usr/include/uuid/uuid.h:107:13: note: in a call to function ‘uuid_unparse’
> > >   107 | extern void uuid_unparse(const uuid_t uu, char *out);
> > >       |             ^~~~~~~~~~~~
> > > cc1: all warnings being treated as errors
> > > 
> > > ...so please allocate the struct fsuuid object dynamically.
> > 
> > So, follow common convention and you'll get it wrong, eh? That a
> > score of -4 on Rusty's API Design scale.
> > 
> > http://sweng.the-davies.net/Home/rustys-api-design-manifesto
> > 
> > Flex arrays in user APIs like this just look plain dangerous to me.
> > 
> > Really, this says that the FSUUID API should have a fixed length
> > buffer size defined in the API and the length used can be anything
> > up to the maximum.
> > 
> > We already have this being added for the ioctl API:
> > 
> > #define UUID_SIZE 16
> > 
> > So why isn't the API definition this:
> > 
> > struct fsuuid {
> >     __u32   fsu_len;
> >     __u32   fsu_flags;
> >     __u8    fsu_uuid[UUID_SIZE];
> > };
> > 
> > Or if we want to support larger ID structures:
> > 
> > #define MAX_FSUUID_SIZE 256
> > 
> > struct fsuuid {
> >     __u32   fsu_len;
> >     __u32   fsu_flags;
> >     __u8    fsu_uuid[MAX_FSUUID_SIZE];
> > };
> > 
> > Then the structure can be safely placed on the stack, which means
> > "the obvious use is (probably) the correct one" (a score of 7 on
> > Rusty's API Design scale). It also gives the kernel a fixed upper
> > bound that it can use to validate the incoming fsu_len variable
> > against...
> 
> Too late now, this already shipped in 6.0.  Changing the struct size
> would change the ioctl number, which is a totally new API.  This was
> already discussed back in July on fsdevel/api.

It is certainly not too late - if we are going to lift this to the
VFS, then we can simply make it a new ioctl. The horrible ext4 ioctl
can ber left to rot in ext4 and nobody else ever needs to care that
it exists.

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

Re: [PATCH v1] xfs_spaceman: add fsuuid command

[Darrick J. Wong]

[adding Ted, the ext4 list, fsdevel, and api, because why not?]

On Tue, Nov 22, 2022 at 10:33:57AM +1100, Dave Chinner wrote:
> On Thu, Nov 17, 2022 at 03:58:06PM -0800, Darrick J. Wong wrote:
> > On Fri, Nov 18, 2022 at 08:51:25AM +1100, Dave Chinner wrote:
> > > On Thu, Nov 17, 2022 at 12:37:33PM -0800, Darrick J. Wong wrote:
> > > > On Wed, Nov 09, 2022 at 02:23:35PM -0800, Catherine Hoang wrote:
> > > > > Add support for the fsuuid command to retrieve the UUID of a mounted
> > > > > filesystem.
> > > > > 
> > > > > Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> > > > > ---

<snip to the good part>

> > > > > diff --git a/spaceman/fsuuid.c b/spaceman/fsuuid.c
> > > > > new file mode 100644
> > > > > index 00000000..be12c1ad
> > > > > --- /dev/null
> > > > > +++ b/spaceman/fsuuid.c
> > > > > @@ -0,0 +1,63 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright (c) 2022 Oracle.
> > > > > + * All Rights Reserved.
> > > > > + */
> > > > > +
> > > > > +#include "libxfs.h"
> > > > > +#include "libfrog/fsgeom.h"
> > > > > +#include "libfrog/paths.h"
> > > > > +#include "command.h"
> > > > > +#include "init.h"
> > > > > +#include "space.h"
> > > > > +#include <sys/ioctl.h>
> > > > > +
> > > > > +#ifndef FS_IOC_GETFSUUID
> > > > > +#define FS_IOC_GETFSUUID	_IOR('f', 44, struct fsuuid)
> > > > > +#define UUID_SIZE 16
> > > > > +struct fsuuid {
> > > > > +    __u32   fsu_len;
> > > > > +    __u32   fsu_flags;
> > > > > +    __u8    fsu_uuid[];
> > > > 
> > > > This is a flex array   ^^ which has no size.  struct fsuuid therefore
> > > > has a size of 8 bytes (i.e. enough to cover the two u32 fields) and no
> > > > more.  It's assumed that the caller will allocate the memory for
> > > > fsu_uuid...
> > > > 
> > > > > +};
> > > > > +#endif
> > > > > +
> > > > > +static cmdinfo_t fsuuid_cmd;
> > > > > +
> > > > > +static int
> > > > > +fsuuid_f(
> > > > > +	int		argc,
> > > > > +	char		**argv)
> > > > > +{
> > > > > +	struct fsuuid	fsuuid;
> > > > > +	int		error;
> > > > 
> > > > ...which makes this usage a problem, because we've not reserved any
> > > > space on the stack to hold the UUID.  The kernel will blindly assume
> > > > that there are fsuuid.fsu_len bytes after fsuuid and write to them,
> > > > which will clobber something on the stack.
> > > > 
> > > > If you're really unlucky, the C compiler will put the fsuuid right
> > > > before the call frame, which is how stack smashing attacks work.  It
> > > > might also lay out bp[] immediately afterwards, which will give you
> > > > weird results as the unparse function overwrites its source buffer.  The
> > > > C compiler controls the stack layout, which means this can go bad in
> > > > subtle ways.
> > > > 
> > > > Either way, gcc complains about this (albeit in an opaque manner)...
> > > > 
> > > > In file included from ../include/xfs.h:9,
> > > >                  from ../include/libxfs.h:15,
> > > >                  from fsuuid.c:7:
> > > > In function ‘platform_uuid_unparse’,
> > > >     inlined from ‘fsuuid_f’ at fsuuid.c:45:3:
> > > > ../include/xfs/linux.h:100:9: error: ‘uuid_unparse’ reading 16 bytes from a region of size 0 [-Werror=stringop-overread]
> > > >   100 |         uuid_unparse(*uu, buffer);
> > > >       |         ^~~~~~~~~~~~~~~~~~~~~~~~~
> > > > ../include/xfs/linux.h: In function ‘fsuuid_f’:
> > > > ../include/xfs/linux.h:100:9: note: referencing argument 1 of type ‘const unsigned char *’
> > > > In file included from ../include/xfs/linux.h:13,
> > > >                  from ../include/xfs.h:9,
> > > >                  from ../include/libxfs.h:15,
> > > >                  from fsuuid.c:7:
> > > > /usr/include/uuid/uuid.h:107:13: note: in a call to function ‘uuid_unparse’
> > > >   107 | extern void uuid_unparse(const uuid_t uu, char *out);
> > > >       |             ^~~~~~~~~~~~
> > > > cc1: all warnings being treated as errors
> > > > 
> > > > ...so please allocate the struct fsuuid object dynamically.
> > > 
> > > So, follow common convention and you'll get it wrong, eh? That a
> > > score of -4 on Rusty's API Design scale.
> > > 
> > > http://sweng.the-davies.net/Home/rustys-api-design-manifesto
> > > 
> > > Flex arrays in user APIs like this just look plain dangerous to me.
> > > 
> > > Really, this says that the FSUUID API should have a fixed length
> > > buffer size defined in the API and the length used can be anything
> > > up to the maximum.
> > > 
> > > We already have this being added for the ioctl API:
> > > 
> > > #define UUID_SIZE 16
> > > 
> > > So why isn't the API definition this:
> > > 
> > > struct fsuuid {
> > >     __u32   fsu_len;
> > >     __u32   fsu_flags;
> > >     __u8    fsu_uuid[UUID_SIZE];
> > > };
> > > 
> > > Or if we want to support larger ID structures:
> > > 
> > > #define MAX_FSUUID_SIZE 256
> > > 
> > > struct fsuuid {
> > >     __u32   fsu_len;
> > >     __u32   fsu_flags;
> > >     __u8    fsu_uuid[MAX_FSUUID_SIZE];
> > > };
> > > 
> > > Then the structure can be safely placed on the stack, which means
> > > "the obvious use is (probably) the correct one" (a score of 7 on
> > > Rusty's API Design scale). It also gives the kernel a fixed upper
> > > bound that it can use to validate the incoming fsu_len variable
> > > against...
> > 
> > Too late now, this already shipped in 6.0.  Changing the struct size
> > would change the ioctl number, which is a totally new API.  This was
> > already discussed back in July on fsdevel/api.
> 
> It is certainly not too late - if we are going to lift this to the
> VFS, then we can simply make it a new ioctl. The horrible ext4 ioctl
> can ber left to rot in ext4 and nobody else ever needs to care that
> it exists.

You're wrong.  This was discussed **multiple times** this summer on
the fsdevel and API lists.  You had plenty of opportunity to make these
suggestions about the design, and yet you did not:

https://lore.kernel.org/linux-api/20220701201123.183468-1-bongiojp@gmail.com/
https://lore.kernel.org/linux-api/20220719065551.154132-1-bongiojp@gmail.com/
https://lore.kernel.org/linux-api/20220719234131.235187-1-bongiojp@gmail.com/
https://lore.kernel.org/linux-api/20220721224422.438351-1-bongiojp@gmail.com/

Jeremy built the functionality and followed the customary process,
sending four separate revisions for reviews.  He adapted his code based
on our feedback about how to future-proof it by adding an explicit
length parameter, and got it merged into ext4 in 6.0-rc1.

Now you want Catherine and I to tear down his work and initiate a design
review of YET ANOTHER NEW IOCTL just so the API can hit this one design
point you care about, and then convince Ted to go back and redo all the
work that has already been done.  All this to extract 16 bytes from the
kernel in a slightly different style than the existing XFS fsgeometry
ioctl.

This was /supposed/ to be a simple way for a less experienced staffer to
gain some experience wiring up an existing ioctl.  And, well, I hope she
doesn't take away that developing for Linux is institutionally broken
and frustrating, because that's what I've taken away from the last 2+
years of being here.

--D

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

Re: [PATCH v1] xfs_spaceman: add fsuuid command

[Dave Chinner]

On Mon, Nov 21, 2022 at 10:21:57PM -0800, Darrick J. Wong wrote:
> [adding Ted, the ext4 list, fsdevel, and api, because why not?]
> 
> On Tue, Nov 22, 2022 at 10:33:57AM +1100, Dave Chinner wrote:
> > On Thu, Nov 17, 2022 at 03:58:06PM -0800, Darrick J. Wong wrote:
> > > On Fri, Nov 18, 2022 at 08:51:25AM +1100, Dave Chinner wrote:
> > > > On Thu, Nov 17, 2022 at 12:37:33PM -0800, Darrick J. Wong wrote:
> > > > > On Wed, Nov 09, 2022 at 02:23:35PM -0800, Catherine Hoang wrote:
> > > > > > Add support for the fsuuid command to retrieve the UUID of a mounted
> > > > > > filesystem.
> > > > > > 
> > > > > > Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> > > > > > ---
> 
> <snip to the good part>
> > > > > If you're really unlucky, the C compiler will put the fsuuid right
> > > > > before the call frame, which is how stack smashing attacks work.  It
> > > > > might also lay out bp[] immediately afterwards, which will give you
> > > > > weird results as the unparse function overwrites its source buffer.  The
> > > > > C compiler controls the stack layout, which means this can go bad in
> > > > > subtle ways.
> > > > > 
> > > > > Either way, gcc complains about this (albeit in an opaque manner)...
> > > > > 
> > > > > In file included from ../include/xfs.h:9,
> > > > >                  from ../include/libxfs.h:15,
> > > > >                  from fsuuid.c:7:
> > > > > In function ‘platform_uuid_unparse’,
> > > > >     inlined from ‘fsuuid_f’ at fsuuid.c:45:3:
> > > > > ../include/xfs/linux.h:100:9: error: ‘uuid_unparse’ reading 16 bytes from a region of size 0 [-Werror=stringop-overread]
> > > > >   100 |         uuid_unparse(*uu, buffer);
> > > > >       |         ^~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > ../include/xfs/linux.h: In function ‘fsuuid_f’:
> > > > > ../include/xfs/linux.h:100:9: note: referencing argument 1 of type ‘const unsigned char *’
> > > > > In file included from ../include/xfs/linux.h:13,
> > > > >                  from ../include/xfs.h:9,
> > > > >                  from ../include/libxfs.h:15,
> > > > >                  from fsuuid.c:7:
> > > > > /usr/include/uuid/uuid.h:107:13: note: in a call to function ‘uuid_unparse’
> > > > >   107 | extern void uuid_unparse(const uuid_t uu, char *out);
> > > > >       |             ^~~~~~~~~~~~
> > > > > cc1: all warnings being treated as errors
> > > > > 
> > > > > ...so please allocate the struct fsuuid object dynamically.
> > > > 
> > > > So, follow common convention and you'll get it wrong, eh? That a
> > > > score of -4 on Rusty's API Design scale.
> > > > 
> > > > http://sweng.the-davies.net/Home/rustys-api-design-manifesto
> > > > 
> > > > Flex arrays in user APIs like this just look plain dangerous to me.
> > > > 
> > > > Really, this says that the FSUUID API should have a fixed length
> > > > buffer size defined in the API and the length used can be anything
> > > > up to the maximum.
> > > > 
> > > > We already have this being added for the ioctl API:
> > > > 
> > > > #define UUID_SIZE 16
> > > > 
> > > > So why isn't the API definition this:
> > > > 
> > > > struct fsuuid {
> > > >     __u32   fsu_len;
> > > >     __u32   fsu_flags;
> > > >     __u8    fsu_uuid[UUID_SIZE];
> > > > };
> > > > 
> > > > Or if we want to support larger ID structures:
> > > > 
> > > > #define MAX_FSUUID_SIZE 256
> > > > 
> > > > struct fsuuid {
> > > >     __u32   fsu_len;
> > > >     __u32   fsu_flags;
> > > >     __u8    fsu_uuid[MAX_FSUUID_SIZE];
> > > > };
> > > > 
> > > > Then the structure can be safely placed on the stack, which means
> > > > "the obvious use is (probably) the correct one" (a score of 7 on
> > > > Rusty's API Design scale). It also gives the kernel a fixed upper
> > > > bound that it can use to validate the incoming fsu_len variable
> > > > against...
> > > 
> > > Too late now, this already shipped in 6.0.  Changing the struct size
> > > would change the ioctl number, which is a totally new API.  This was
> > > already discussed back in July on fsdevel/api.
> > 
> > It is certainly not too late - if we are going to lift this to the
> > VFS, then we can simply make it a new ioctl. The horrible ext4 ioctl
> > can ber left to rot in ext4 and nobody else ever needs to care that
> > it exists.
> 
> You're wrong.  This was discussed **multiple times** this summer on
> the fsdevel and API lists.  You had plenty of opportunity to make these
> suggestions about the design, and yet you did not:
> 
> https://lore.kernel.org/linux-api/20220701201123.183468-1-bongiojp@gmail.com/
> https://lore.kernel.org/linux-api/20220719065551.154132-1-bongiojp@gmail.com/
> https://lore.kernel.org/linux-api/20220719234131.235187-1-bongiojp@gmail.com/
> https://lore.kernel.org/linux-api/20220721224422.438351-1-bongiojp@gmail.com/


There's good reason for that: this was posted and reviewed as *an
EXT4 specific API*.  Why are you expecting XFS developers to closely
review a patchset that was titled "Add ioctls to get/set the ext4
superblock uuid."?

There was -no reasons- for me to pay attention to it, and I have
enough to keep up with without having to care about the minutae of
what ext4 internal information is being exposing to userspace.

However, now it's being proposed as a *generic VFS API*, and so it's
now important enough for developers from other filesystems to look
at this ioctl API.

> Jeremy built the functionality and followed the customary process,
> sending four separate revisions for reviews.  He adapted his code based
> on our feedback about how to future-proof it by adding an explicit
> length parameter, and got it merged into ext4 in 6.0-rc1.

*As an EXT4 modification*, not a generic VFS ioctl.

> Now you want Catherine and I to tear down his work and initiate a design
> review of YET ANOTHER NEW IOCTL just so the API can hit this one design
> point you care about, and then convince Ted to go back and redo all the
> work that has already been done.  All this to extract 16 bytes from the
> kernel in a slightly different style than the existing XFS fsgeometry
> ioctl.

I'm not asking you to tear anything down. Just leave the ext4 ioctl
as it is currently defined and nothing existing breaks or needs
reworking.

All I'm asking is that instead of lifting the ext4 ioctl verbatim,
you lift it with a fixed maximum size for the uuid data array to
replace the flex array. It's a *trivial change to make*, and yes, I
know that this means it's not the same as the ext4 ioctl.

But, really, who cares that it will be a different ioctl? Nobody but
ext4 utilities will be using the ext4 ioctl, and we expect generic
block/fs utilities and applications to use the VFS definition of the
ioctl, not the ext4 specific one.

> This was /supposed/ to be a simple way for a less experienced staffer to
> gain some experience wiring up an existing ioctl.  And, well, I hope she
> doesn't take away that developing for Linux is institutionally broken
> and frustrating, because that's what I've taken away from the last 2+
> years of being here.

When we lift stuff from filesystem specific scope (where few people
care about API warts) to generic VFS scope that the whole world is
expected to see, use and understand, you should expect a larger
number of experienced developers to scrutinise it.  The wider scope
of the API means the "acceptibility bar" is set higher.

Just because the code change is simple, it doesn't mean the issues
surrounding the code change are simple or straight forward. Just
because it went through a review on the ext4 list it doesn't mean
the API or implementation is flawless.

The point I'm making is that lifting fs ioctl APIs verbatim is a
*known broken process* that leads to future pain fixing all the
problems inherited from the original fs specific API and
implementation.  If we want to lift functionality to be generic VFS
UAPI and at the time of lifting we find problems with the UAPI
and/or implementation, then we need to fix the problems before we
expose the new VFS API to the entire world.

Repeat past mistakes, or learn from them. Your choice...

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