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

[David Howells]

Jann Horn <jannh@xxxxxxxxxx> wrote:

> > +int fsinfo_string(const char *s, struct fsinfo_context *ctx)
> ...
> Please add a check here to ensure that "ret" actually fits into the
> buffer (and use WARN_ON() if you think the check should never fire).
> Otherwise I think this is too fragile.

How about:

	int fsinfo_string(const char *s, struct fsinfo_context *ctx)
	{
		unsigned int len;
		char *p = ctx->buffer;
		int ret = 0;
		if (s) {
			len = strlen(s);
			if (len > ctx->buf_size - 1)
				len = ctx->buf_size;
			if (!ctx->want_size_only) {
				memcpy(p, s, len);
				p[len] = 0;
			}
			ret = len;
		}
		return ret;
	}

I've also added a check to eliminate the copy if userspace didn't actually
supply a buffer.

> > +       ret = vfs_statfs(path, &buf);
> > +       if (ret < 0 && ret != -ENOSYS)
> > +               return ret;
> ...
> > +       memcpy(&p->f_fsid, &buf.f_fsid, sizeof(p->f_fsid));
> 
> What's going on here? If vfs_statfs() returns -ENOSYS, we just use the
> (AFAICS uninitialized) buf.f_fsid anyway in the memcpy() below and
> return it to userspace?

Good point.  I've made the access to the buffer contingent on ret==0.  If I
don't set it, it will just be left pre-cleared.

> > +       return sizeof(*attr);
> 
> I think you meant sizeof(*info).

Yes.  I've renamed the buffer point to "p" in all cases so that it's more
obvious.

> > +       return ctx->usage;
> 
> It is kind of weird that you have to return the ctx->usage everywhere
> even though the caller already has ctx...

At this point, it's only used and returned by fsinfo_attributes() and really
is only for the use of the attribute getter function.

I could, I suppose, return the amount of data in ctx->usage and then preset it
for VSTRUCT-type objects.  Unfortunately, I can't make the getter return void
since it might have to return an error.

> > +               ctx->buffer = kvmalloc(ctx->buf_size, GFP_KERNEL);
> 
> ctx->buffer is _almost_ always pre-zeroed (see vfs_do_fsinfo() below),
> except if we have FSINFO_TYPE_OPAQUE or FSINFO_TYPE_LIST with a size
> bigger than what the attribute's ->size field said? Is that
> intentional?

Fixed.

> > +struct fsinfo_attribute {
> > +       unsigned int            attr_id;        /* The ID of the attribute */
> > +       enum fsinfo_value_type  type:8;         /* The type of the attribute's value(s) */
> > +       unsigned int            flags:8;
> > +       unsigned int            size:16;        /* - Value size (FSINFO_STRUCT) */
> > +       unsigned int            element_size:16; /* - Element size (FSINFO_LIST) */
> > +       int (*get)(struct path *path, struct fsinfo_context *params);
> > +};
> 
> Why the bitfields? It doesn't look like that's going to help you much,
> you'll just end up with 6 bytes of holes on x86-64:

Expanding them to non-bitfields will require an extra 10 bytes, making the
struct 8 bytes bigger with 4 bytes of padding.  I can do that if you'd rather.

David