by Spencer Baugh

[permalink] [raw]

Subject: Re: [PATCH RESEND v11 7/8] open: openat2(2) syscall

Jeff Layton <[email protected]> writes:
> On Mon, 2019-08-26 at 19:50 +0000, [email protected] wrote:
>> Aleksa Sarai <[email protected]> writes:
>> > To this end, we introduce the openat2(2) syscall. It provides all of the
>> > features of openat(2) through the @how->flags argument, but also
>> > also provides a new @how->resolve argument which exposes RESOLVE_* flags
>> > that map to our new LOOKUP_* flags. It also eliminates the long-standing
>> > ugliness of variadic-open(2) by embedding it in a struct.
>>
>> I don't like this usage of a structure in memory to pass arguments that
>> would fit in registers. This would be quite inconvenient for me as a
>> userspace developer.
>>
>> Others have brought up issues with this: the issue of seccomp, and the
>> issue of mismatch between the userspace interface and the kernel
>> interface, are the most important for me. I want to add another,
>> admittedly somewhat niche, concern.
>>
>> This interfaces requires a program to allocate memory (even on the
>> stack) just to pass arguments to the kernel which could be passed
>> without allocating that memory. That makes it more difficult and less
>> efficient to use this syscall in any case where memory is not so easily
>> allocatable: such as early program startup or assembly, where the stack
>> may be limited in size or not even available yet, or when injecting a
>> syscall while ptracing.
>>
>> A struct-passing interface was needed for clone, since we ran out of
>> registers; but we have not run out of registers yet for openat, so it
>> would be nice to avoid this if we can. We can always expand later...
>>
>
> We can't really expand later like you suggest.
>
> Suppose in a couple of years that we need to add some new argument to
> openat2 that isn't just a new flag. If all these values are passed by
> individual arguments, you can't add one later without adding yet another
> syscall.

Sure we can. This new syscall doesn't need to use all 6 available
arguments. It can enforce that the unused ones are 0. Then if we
eventually run out of flags and need to switch to pass arguments via
struct, we can just use one of the unused arguments for that purpose.

Even if we used all 6 arguments, in the worst-case scenario, the last
flag we add could change the interpretation of some other argument so it
can be used to pass a pointer to a struct.

2019-08-29 12:19:02

On 2019-08-29, Rasmus Villemoes <[email protected]> wrote:
> On 29/08/2019 14.15, Aleksa Sarai wrote:
> > On 2019-08-24, Daniel Colascione <[email protected]> wrote:
>
> >> Why pad the structure when new functionality (perhaps accommodated via
> >> a larger structure) could be signaled by passing a new flag? Adding
> >> reserved fields to a structure with a size embedded in the ABI makes a
> >> lot of sense --- e.g., pthread_mutex_t can't grow. But this structure
> >> can grow, so the reservation seems needless to me.
> >
> > Quite a few folks have said that ->reserved is either unnecessary or
> > too big. I will be changing this, though I am not clear what the best
> > way of extending the structure is. If anyone has a strong opinion on
> > this (or an alternative to the ones listed below), please chime in. I
> > don't have any really strong attachment to this aspect of the API.
> >
> > There appear to be a few ways we can do it (that all have precedence
> > with other syscalls):
> >
> > 1. Use O_* flags to indicate extensions.
> > 2. A separate "version" field that is incremented when we change.
> > 3. Add a size_t argument to openat2(2).
> > 4. Reserve space (as in this patchset).
> >
> > (My personal preference would be (3), followed closely by (2).)
>
> 3, definitely, and instead of having to invent a new scheme for every
> new syscall, make that the default pattern by providing a helper

Sure (though hopefully I don't need to immediately go and refactor all
the existing size_t syscalls). I will be presenting about this patchset
at the containers microconference at LPC (in a few weeks), so I'll hold
of on any API-related rewrites until after that.

> int __copy_abi_struct(void *kernel, size_t ksize, const void __user
> *user, size_t usize)
> {
> size_t copy = min(ksize, usize);
>
> if (copy_from_user(kernel, user, copy))
> return -EFAULT;
>
> if (usize > ksize) {
> /* maybe a separate "return user_is_zero(user + ksize, usize -
> ksize);" helper */
> char c;
> user += ksize;
> usize -= ksize;
> while (usize--) {
> if (get_user(c, user++))
> return -EFAULT;
> if (c)
> return -EINVAL;

This part would probably be better done with memchr_inv() and
copy_from_user() (and probably should put an upper limit on usize), but
I get what you mean.

> }
> } else if (ksize > usize) {
> memset(kernel + usize, 0, ksize - usize);
> }
> return 0;
> }
> #define copy_abi_struct(kernel, user, usize) \
> __copy_abi_struct(kernel, sizeof(*kernel), user, usize)
>
> > Both (1) and (2) have the problem that the "struct version" is inside
> > the struct so we'd need to copy_from_user() twice. This isn't the end of
> > the world, it just feels a bit less clean than is ideal. (3) fixes that
> > problem, at the cost of making the API slightly more cumbersome to use
> > directly (though again glibc could wrap that away).
>
> I don't see how 3 is cumbersome to use directly. Userspace code does
> struct openat_of_the_day args = {.field1 = x, .field3 = y} and passes
> &args, sizeof(args). What does glibc need to do beyond its usual munging
> of the userspace ABI registers to the syscall ABI registers?

I'd argue that

ret = openat2(AT_FDCWD, "foo", &how, sizeof(how)); // (3)

is slightly less pretty than

ret = openat2(AT_FDCWD, "foo", &how); // (1), (2), (4)

But it's not really that bad. Forget I said anything.

--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Attachments:

(No filename) (3.52 kB)
signature.asc (235.00 B)
Download all attachments