Re: [PATCH 01/25] vfs: syscall: Add fsinfo() to query filesystem information [ver #14]

[Christian Brauner]

On Wed, Jun 26, 2019 at 10:49:12AM +0100, David Howells wrote:
> Christian Brauner <christian@xxxxxxxxxx> wrote:
> 
> > > +	return sizeof(*p);
> > 
> > Hm, the discrepancy between the function signature returning int and
> > the sizeof operator most likely being size_t is bothering me. It
> > probably doesn't matter but maybe we can avoid that.
> 
> If sizeof(*p) exceeds 4096, the buffer is going to have been overrun by this
> point anyway.

Ok.

> 
> The function can't return size_t, though it could return ssize_t.  I could
> switch it to return long or even store the result in fsinfo_kparams::usage and
> return 0.
> 
> > > +	strlcpy(p->f_fs_name, path->dentry->d_sb->s_type->name,
> > > +		sizeof(p->f_fs_name));
> > 
> > Truncation is acceptable or impossible I assume?
> 
> I'm hoping that file_system_type::name isn't going to exceed 15 chars plus
> NUL.  If it does, it will be truncated.  I don't really want to add an
> individual attribute just for the filesystem driver name.
> 
> > > +#define _gen(X, Y) FSINFO_ATTR_##X: return fsinfo_generic_##Y(path, params->buffer)
> > 
> > I'm really not sure that this helps readability in the switch below... :)
> > 
> > > +
> > > +	switch (params->request) {
> > > +	case _gen(STATFS,		statfs);
> > > +	case _gen(IDS,			ids);
> > > +	case _gen(LIMITS,		limits);
> > > +	case _gen(SUPPORTS,		supports);
> > > +	case _gen(CAPABILITIES,		capabilities);
> > > +	case _gen(TIMESTAMP_INFO,	timestamp_info);
> > > ...
> 
> I'm trying to avoid having to spend multiple lines per case and tabulation
> makes things easier to read.  So
> 
> 	case FSINFO_ATTR_SUPPORTS:		return fsinfo_generic_supports(path, params->buffer);
> 	case FSINFO_ATTR_CAPABILITIES:		return fsinfo_generic_capabilities(path, params->buffer);
> 	case FSINFO_ATTR_TIMESTAMP_INFO:	return fsinfo_generic_timestamp_info(path, params->buffer);
> 
> is a bit on the long side per line, whereas:
> 
> 	case FSINFO_ATTR_SUPPORTS:
> 		return fsinfo_generic_supports(path, params->buffer);
> 	case FSINFO_ATTR_CAPABILITIES:
> 		return fsinfo_generic_capabilities(path, params->buffer);
> 	case FSINFO_ATTR_TIMESTAMP_INFO:
> 		return fsinfo_generic_timestamp_info(path, params->buffer);
> 
> is less readable by interleaving two of the three columns.  (Note that _gen is
> a actually third column as I introduce alternatives later).
> 
> > > +		if (ret <= (int)params->buf_size)
> > 
> > He, and this is where the return value discrepancy hits again. Just
> > doesn't look nice tbh. :)
> 
> No.  That's dealing with signed/unsigned comparison.  It might be better if I
> change this to:
> 
> 		if (IS_ERR_VALUE(ret))
> 			return ret; /* Error */
> 		if ((unsigned int)ret <= params->buf_size)
> 			return ret; /* It fitted */
> 
> In any case, buf_size isn't permitted to be larger than INT_MAX due to a check
> later in the loop.
> 
> > > +		kvfree(params->buffer);
> > 
> > That means callers should always memset fsinfo_kparams or this is an
> > invalid free...
> 
> vfs_info() isn't a public function.  And, in any case, the caller *must*
> provide a buffer here.
> 
> > > + * Return buffer information by requestable attribute.
> > > + *
> > > + * STRUCT indicates a fixed-size structure with only one instance.
> > > ...
> > I honestly have a hard time following the documentation here
> 
> How about:
> 
>  * STRUCT	- a fixed-size structure with only one instance.
>  * STRUCT_N	- a sequence of STRUCTs, indexed by Nth
>  * STRUCT_NM	- a sequence of sequences of STRUCTs, indexed by Nth, Mth
>  * STRING	- a string with only one instance.
>  * STRING_N	- a sequence of STRING, indexed by Nth
>  * STRING_NM	- a sequence of sequences of STRING, indexed by Nth, Mth
>  * OPAQUE	- a blob that can be larger than 4K.
>  * STRUCT_ARRAY - an array of structs that can be larger than 4K
> 
> > and that monster table/macro thing below.  For example, STRUCT_NM
> > corresponds to __FSINFO_NM or what?
> 
> STRUCT_NM -> .type = __FSINFO_STRUCT, .flags = __FSINFO_NM, .size = ...
> 
> If you think this is bad, you should try looking at the device ID tables used
> by the drivers and the attribute tables;-)
> 
> I could spell out the flag and type in the macro defs (such as the body of
> FSINFO_STRING(X,Y) for instance).  It would make it harder to compare macros
> as it wouldn't then tabulate, though.
> 
> > And is this uapi as you're using this in your samples/test below?
> 
> Not exactly.  Each attribute is defined as being a certain type in the
> documentation in the UAPI header, but this is not coded there.  The assumption
> being that if you're using a particular attribute, you'll know what the type
> of the attribute is and you'll structure your code appropriately.
> 
> The reason the sample code has this replicated is that it doesn't really
> attempt to interpret the type per se.  It has a dumper for an individual
> attribute value, but the table tells it whether there should be one of those,
> N of those or N of M(0), M(1), M(2), ... of those so that it can report an
> error if it doesn't see what it expects.
> 
> I could even cheaply provide a meta attribute that dumps the contents of the
> table (just the type info, not the names).
> 
> > > ...
> > > +	FSINFO_STRING		(NAME_ENCODING,		-),
> > > +	FSINFO_STRING		(NAME_CODEPAGE,		-),
> > > +};
> > 
> > Can I complain again that this is really annoying to parse.
> 
> Apparently you can;-)  What would you prefer?  This:
> 
> static const struct fsinfo_attr_info fsinfo_buffer_info[FSINFO_ATTR__NR] = {
> 	[FSINFO_STATFS] = {
> 		.type	= __FSINFO_STRUCT,
> 		.size	= sizeof(struct fsinfo_statfs),
> 	},
> 	[FSINFO_SERVERS] = {
> 		.type	= __FSINFO_STRUCT,
> 		.flags	= __FSINFO_NM,
> 		.size	= sizeof(struct fsinfo_server),
> 	},
> 	...
> };	
> 
> That has 3-5 lines for each 1 in the current code and isn't a great deal more
> readable.

Really, I find this more readable because parsing structs and arrays of
structs is probably still even more common for C programmers then
deciphering nested macros. :) But I won't enforce my own pov. :)

> 
> > if (copy_to_user()) and if (clear_user()) and not if (clear_user() != 0)
> 
> Better "if (copy_to_user() != 0)" since it's not a boolean return value in
> either case.
> 
> > Nit: There's a bunch of name inconsistency for the arguments between the
> > stub and the definition:
> > 
> > SYSCALL_DEFINE5(fsinfo,
> > 		int, dfd, const char __user *, filename,
> > 		struct fsinfo_params __user *, _params,
> > 		void __user *, user_buffer, size_t, user_buf_size)
> 
> Yeah.  C just doesn't care.
> 
> I'll change filename to pathname throughout.  That's at least consistent with
> various glibc manpages for other vfs syscalls.
> 
> _params I can change to params and params as-was to kparams.
> 
> But user_buffer and user_buf_size, I'll keep as I've named them such to avoid
> confusion with kparams->buffer and kparams->scratch_buffer.  However, I
> wouldn't want to call them that in the UAPI.

Yep, it's really just that I prefer the naming to be consistent. :)

> 
> > Do we do SPDX that way? Or isn't this just supposed to be:
> > // <spdxy stuff>
> 
> Look in, say, include/uapi/linux/stat.h or .../fs.h.
> 
> > > +	FSINFO_ATTR__NR
> > 
> > Nit/Bikeshed: FSINFO_ATTR_MAX? Seems more intuitive.
> 
> No.  That would imply a limit that it will never exceed.
> 
> > > +struct fsinfo_u128 {
> > > +#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN)
> > > +	__u64	hi;
> > > +	__u64	lo;
> > > +#elif defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN : defined(__LITTLE_ENDIAN)
> > > +	__u64	lo;
> > > +	__u64	hi;
> > > +#endif
> > > +};
> > 
> > Hm, I know why you do this custom fsinfo_u128 thingy but for userspace
> > that is going to be annoying to operate with, e.g. comparing the
> > size/space of two filesystems etc.
> 
> We don't have a __u128 in the UAPI, and I'm reluctant to use __uint128_t.
> 
> Do you have a better suggestion?
> 
> > > +struct fsinfo_ids {
> > > +	char	f_fs_name[15 + 1];	/* Filesystem name */
> > 
> > You should probably make this a macro so userspace can use it in fs-name
> > length checks too.
> 
> The name length, you mean?  Well, you can use sizeof...
> 
> > > +	FSINFO_CAP__NR
> > 
> > Hm, again, maybe better to use FSINFO_CAP_MAX?
> 
> It's not a limit.

Well, in both cases it's giving the limit of currently supported
attributes. Other places in the kernel do the same (netlink for
example). Anyway, it probably doesn't matter that much.

Christian