Message-ID: <20230920132606.187860-1-mattlloydhouse@gmail.com>
Date:   Wed, 20 Sep 2023 09:26:03 -0400
From:   Matthew House <mattlloydhouse@...il.com>
To:     Miklos Szeredi <miklos@...redi.hu>
Cc:     Christian Brauner <brauner@...nel.org>,
        Miklos Szeredi <mszeredi@...hat.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-api@...r.kernel.org, linux-man@...r.kernel.org,
        linux-security-module@...r.kernel.org, Karel Zak <kzak@...hat.com>,
        Ian Kent <raven@...maw.net>,
        David Howells <dhowells@...hat.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <christian@...uner.io>,
        Amir Goldstein <amir73il@...il.com>
Subject: Re: [RFC PATCH 2/3] add statmnt(2) syscall

On Wed, Sep 20, 2023 at 5:42 AM Miklos Szeredi <miklos@...redi.hu> wrote:
> On Tue, 19 Sept 2023 at 23:28, Matthew House <mattlloydhouse@...il.com> wrote:
>
> > More generally speaking, the biggest reason I dislike the current single-
> > buffer interface is that the output is "all or nothing": either the caller
> > has enough space in the buffer to store every single string, or it's unable
> > to get any fields at all, just an -EOVERFLOW. There's no room for the
> > caller to say that it just wants the integer fields and doesn't care about
> > the strings. Thus, to reliably call statmnt() on an arbitrary mount, the
> > ability to dynamically allocate memory is effectively mandatory. The only
> > real solution to this would be additional statx-like flags to select the
> > returned strings.
>
> It's already there:
>
> #define STMT_MNT_ROOT 0x00000008U /* Want/got mnt_root  */
> #define STMT_MNT_POINT 0x00000010U /* Want/got mnt_point */
> #define STMT_FS_TYPE 0x00000020U /* Want/got fs_type */
>
> For example, it's perfectly fine to do the following, and it's
> guaranteed not to return EOVERFLOW:
> 
>         struct statmnt st;
>         unsigned int mask = STMT_SB_BASIC | STMT_MNT_BASIC;
> 
>         ret = statmount(mnt_id, mask, &st, sizeof(st), flags);

Whoops, my apologies; perhaps I should try to learn to read for once. (I
just saw the undecorated sequence of stmt_numeric() and stmt_string() calls
and didn't notice the early exits within the functions.) I withdraw that
particular objection.

> > Besides that, if the caller is written in standard C but doesn't want to
> > use malloc(3) to allocate the buffer, then its helper function must be
> > written very carefully (with a wrapper struct around the header and data)
> > to satisfy the aliasing rules, which forbid programs from using a struct
> > statmnt * pointer to read from a declared char[N] array.
>
> I think you interpret aliasing rules incorrectly.  The issue with
> aliasing is if you access the same piece of memory though different
> types.  Which is not the case here.  In fact with the latest
> incarnation of the interface[1] there's no need to access the
> underlying buffer at all:
>
>         printf("mnt_root: <%s>\n", st->str + st->mnt_root);
>
> So the following is perfectly safe to do (as long as you don't care
> about buffer overflow):
>
>         char buf[10000];
>         struct statmnt *st = (void *) buf;
>
>         ret = statmount(mnt_id, mask, st, sizeof(buf), flags);

The declared type of a variable *is* one of the different types, as far as
the aliasing rules are concerned. In C17, section 6.5 ("Expressions"):

> The *effective type* of an object for an access to its stored value is
> the declared type of the object, if any. [More rules about objects with
> no declared type, i.e., those created with malloc(3) or realloc(3)...]
>
> An object shall have its stored value accessed only by an lvalue
> expression that has one of the following types:
>
> -- a type compatible with the effective type of the object,
>
> -- a qualified version of a type compatible with the effective type of
>    the object,
>
> -- a type that is the signed or unsigned type corresponding to the
>    effective type of the object,
>
> -- a type that is the signed or unsigned type corresponding to a
>    qualified version of the effective type of the object,
>
> -- an aggregate or union type that includes one of the aforementioned
>    types among its members (including, recursively, a member of a
>    subaggregate or contained union), or
>
> -- a character type.

In this case, buf is declared in the program as a char[10000] array, so the
declared type of each element is char, and the effective type of each
element is also char. If we want to access, say, st->mnt_id, the lvalue
expression has type __u64, and it tries to access 8 of the char objects.
However, the integer type that __u64 expands to doesn't meet any of those
criteria, so the aliasing rules are violated and the behavior is undefined.

(The statmount() helper could in theory avoid UB by saying the struct
statmnt object is stored in the buffer as if by memcpy(3), but it would
still be UB for the caller to access the fields of that pointer directly
instead of memcpy'ing them back out of the buffer. And practically no one
does that in the real world.)

It's a common misconception that the aliasing rules as written are about
accessing the same object through two different pointer types. That
corollary is indeed what compilers mainly care about, but the C/C++
standards further say that objects in memory "remember" the types they were
created with, and they demand that programs respect objects' original types
when trying to access them (except when accessing their raw representations
via a pointer of character type).

> If you do care about handling buffer overflows, then dynamic
> allocation is the only sane way.
>
> And before you dive into how this is going to be horrible because the
> buffer size needs to be doubled an unknown number of times, think a
> bit:  have you *ever* seen a line in /proc/self/mountinfo longer than
> say 1000 characters?   So if the buffer starts out at 64k, how often
> will this doubling happen?   Right: practically never.  Adding
> complexity to handle this case is nonsense, as I've said many times.
> And there is definitely nonzero complexity involved (just see the
> special casing in getxattr and listxattr implementations all over the
> place).
>
> Thanks,
> Miklos

I've always felt that capacity doubling is a bit wasteful, but it's
definitely something I can live with, especially if providing size feedback
is as complex as you suggest. Still, I'm not a big fan of single-buffer
interfaces in general, with how poorly they tend to interact with C's
aliasing rules. (Also, those kinds of interfaces also invite alignment
errors: for instance, your snippet above is missing the necessary union to
prevent the buffer from being misaligned, which would cause UB when you
cast it to a struct statmnt *.)

Thank you,
Matthew House

Powered by blists - more mailing lists