Received-SPF: pass (google.com: domain of linux-kernel+bounces-55773-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 6 Feb 2024 19:52:45 -0500
From: Kent Overstreet <kent.overstreet@linux.dev>
To: Dave Chinner <david@fromorbit.com>
Cc: brauner@kernel.org, linux-fsdevel@vger.kernel.org, 
	linux-kernel@vger.kernel.org, Jan Kara <jack@suse.cz>, Dave Chinner <dchinner@redhat.com>, 
	"Darrick J. Wong" <djwong@kernel.org>, Theodore Ts'o <tytso@mit.edu>, 
	Josef Bacik <josef@toxicpanda.com>
Subject: Re: [PATCH v2 5/7] fs: FS_IOC_GETSYSFSNAME
Message-ID: <kx372qkpaasuba7nnyk2u6g2plqc5nkojft4hn3vfbkauaxjkl@pphn5hoo7ixg>
References: <20240206201858.952303-1-kent.overstreet@linux.dev>
 <20240206201858.952303-6-kent.overstreet@linux.dev>
 <ZcKyIMPy1D1o5Yla@dread.disaster.area>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZcKyIMPy1D1o5Yla@dread.disaster.area>

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@kernel.org>
> > Cc: Jan Kara <jack@suse.cz>
> > Cc: Dave Chinner <dchinner@redhat.com>
> > Cc: "Darrick J. Wong" <djwong@kernel.org>
> > Cc: Theodore Ts'o <tytso@mit.edu>
> > Cc: Josef Bacik <josef@toxicpanda.com>
> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > ---
> >  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