Re: [PATCH v2 5/7] fs: FS_IOC_GETSYSFSNAME

[Kent Overstreet]

On Wed, Feb 07, 2024 at 09:26:40AM +1100, Dave Chinner wrote:
> On Tue, Feb 06, 2024 at 03:18:53PM -0500, Kent Overstreet wrote:
> > Add a new ioctl for getting the sysfs name of a filesystem - the path
> > under /sys/fs.
> > 
> > This is going to let us standardize exporting data from sysfs across
> > filesystems, e.g. time stats.
> > 
> > The returned path will always be of the form "$FSTYP/$SYSFS_IDENTIFIER",
> > where the sysfs identifier may be a UUID (for bcachefs) or a device name
> > (xfs).
> > 
> > Cc: Christian Brauner <brauner@xxxxxxxxxx>
> > Cc: Jan Kara <jack@xxxxxxx>
> > Cc: Dave Chinner <dchinner@xxxxxxxxxx>
> > Cc: "Darrick J. Wong" <djwong@xxxxxxxxxx>
> > Cc: Theodore Ts'o <tytso@xxxxxxx>
> > Cc: Josef Bacik <josef@xxxxxxxxxxxxxx>
> > Signed-off-by: Kent Overstreet <kent.overstreet@xxxxxxxxx>
> > ---
> >  fs/ioctl.c              | 17 +++++++++++++++++
> >  include/linux/fs.h      |  1 +
> >  include/uapi/linux/fs.h | 10 ++++++++++
> >  3 files changed, 28 insertions(+)
> > 
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index 046c30294a82..7c37765bd04e 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -776,6 +776,20 @@ static int ioctl_getfsuuid(struct file *file, void __user *argp)
> >  	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
> >  }
> >  
> > +static int ioctl_get_fs_sysfs_path(struct file *file, void __user *argp)
> > +{
> > +	struct super_block *sb = file_inode(file)->i_sb;
> > +
> > +	if (!strlen(sb->s_sysfs_name))
> > +		return -ENOIOCTLCMD;
> 
> This relies on the kernel developers getting string termination
> right and not overflowing buffers.
> 
> We can do better, I think, and I suspect that starts with a
> super_set_sysfs_name() helper....

I don't think that's needed here; a standard snprintf() ensures that the
buffer is null terminated, even if the result overflowed.

> > +	struct fssysfspath u = {};
> 
> Variable names that look like the cat just walked over the keyboard
> are difficult to read. Please use some separators here! 
> 
> Also, same comment as previous about mixing code and declarations.
> 
> > +
> > +	snprintf(u.name, sizeof(u.name), "%s/%s", sb->s_type->name, sb->s_sysfs_name);
> 
> What happens if the combined path overflows the fssysfspath
> buffer?

It won't; s_sysfs_name is going to be either a UUID or a short device
name. It can't be a longer device name (like we show in /proc/mounts) -
here that would lead to ambiguouty.

> > +	char			s_sysfs_name[UUID_STRING_LEN + 1];
> 
> Can we just kstrdup() the sysfs name and keep a {ptr, len} pair
> in the sb for this? Then we can treat them as an opaque identifier
> that isn't actually a string, and we don't have to make up some
> arbitrary maximum name size, either.

What if we went in a different direction - take your previous suggestion
to have a helper for setting the name, and then enforce through the API
that the only valid identifiers are a UUID or a short device name.

super_set_sysfs_identifier_uuid(sb);
super_set_sysfs_identifier_device(sb);

then we can enforce that the identifier comes from either sb->s_uuid or
sb->s_dev.

I'll have to check existing filesystems before we commit to that,
though.

> 
> >  	unsigned int		s_max_links;
> >  
> > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> > index 16a6ecadfd8d..c0f7bd4b6350 100644
> > --- a/include/uapi/linux/fs.h
> > +++ b/include/uapi/linux/fs.h
> > @@ -77,6 +77,10 @@ struct fsuuid2 {
> >  	__u8	uuid[16];
> >  };
> >  
> > +struct fssysfspath {
> > +	__u8			name[128];
> > +};
> 
> Undocumented magic number in a UAPI. Why was this chosen?

In this case, I think the magic number is fine - though it could use a
comment; since it only needs to be used in one place giving it a name is
a bit pointless.

As to why it was chosen - 64 is the next power of two size up from the
length of a uuid string length, and 64 should fit any UUID + filesystem
name. So took that and doubled it.

> IMO, we shouldn't be returning strings that require the the kernel
> to place NULLs correctly and for the caller to detect said NULLs to
> determine their length, so something like:
> 
> struct fs_sysfs_path {
> 	__u32			name_len;
> 	__u8			name[NAME_MAX];
> };
> 
> Seems better to me...

I suppose modern languages are getting away from the whole
nul-terminated string thing, heh