Re: [PATCH] linux: Add openat2 (BZ 31664)

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 29/04/24 10:38, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> diff --git a/sysdeps/unix/sysv/linux/bits/openat2.h b/sysdeps/unix/sysv/linux/bits/openat2.h
>> new file mode 100644
>> index 0000000000..a07984dace
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/bits/openat2.h
> 
>> +#ifndef __glibc_has_open_how
>> +/* Arguments for how openat2 should open the target path.  */
>> +struct open_how
>> +{
>> +  __uint64_t flags;
>> +  __uint64_t mode;
>> +  __uint64_t resolve;
>> +};
>> +#endif
> 
> I think this needs better documentation that this is an extensible
> struct which must only be used with the openat2 function, and must not
> be nested in other structs or exposed in any way where the exact struct
> size matters.
> 
> Symbol versioning is not able to solve this problem because the only
> symbol involved here (openat2) does not actually depend on the struct
> size.

Another possibility would to not use the kernel definition and instead
add a 'open_how' with some spare size (kernel will only reject large
sizes if is not zero-initialized and it does not support the extra fields).
It will require glibc to update the new fields on kernel updates tough.

Indeed we don't need symbol versioning, we can infer the struct layout
by the size alone.

> 
>> diff --git a/sysdeps/unix/sysv/linux/tst-openat2.c b/sysdeps/unix/sysv/linux/tst-openat2.c
>> new file mode 100644
>> index 0000000000..d0c26f2bce
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/tst-openat2.c
> 
>> +static int
>> +do_test_struct (void)
>> +{
>> +  static struct struct_test
>> +  {
>> +    struct open_how_ext
>> +    {
>> +      struct open_how inner;
>> +      int extra1;
>> +      int extra2;
>> +      int extra3;
>> +    } arg;
>> +    size_t size;
>> +    int err;
>> +  } tests[] =
>> +  {
>> +    {
>> +      /* Zero size.  */
>> +      .arg.inner.flags = O_RDONLY,
>> +      .size = 0,
>> +      .err = EINVAL,
>> +    },
>> +    {
>> +      /* Normal struct.  */
>> +      .arg.inner.flags = O_RDONLY,
>> +      .size = sizeof (struct open_how),
>> +    },
>> +    {
>> +      /* Larger struct, zeroed out the unused values.  */
>> +      .arg.inner.flags = O_RDONLY,
>> +      .size = sizeof (struct open_how_ext),
>> +    },
>> +    {
>> +      /* Larger struct, non-zeroed out the unused values.  */
>> +      .arg.inner.flags = O_RDONLY,
>> +      .arg.extra1 = 0xdeadbeef,
>> +      .size = sizeof (struct open_how_ext),
>> +      .err = E2BIG,
>> +    },
>> +    {
>> +      /* Larger struct, non-zeroed out the unused values.  */
>> +      .arg.inner.flags = O_RDONLY,
>> +      .arg.extra2 = 0xdeadbeef,
>> +      .size = sizeof (struct open_how_ext),
>> +      .err = E2BIG,
>> +    },
> 
> These tests will require constant maintenance on kernel upgrades.  Are
> they really helpful?

It would if the idea is to try to keep the fallback interface in sync
with kernel (or if we skip kernel inclusion altogether).  

I don't have a strong opinion which strategy we should use for these new
kernel interfaces.