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
