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
