Re: [PATCH] RFC: avoid cancellable I/O primitives in ld.so

[Zack Weinberg <zackw at panix dot com>]

On Thu, Apr 5, 2018 at 8:32 AM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:

Thanks for reviewing.  I believe I have addressed all of your
comments, and build-many-glibcs testing found a few more problems
which I have also fixed; revised patch to follow.

>> +  /* fcntl(F_GETFD) is not a cancellation point, but we still use
>> +     __fcntl_nocancel so that it is *statically* not a cancellation
>> +     point, which matters to the code that constructs ld.so.  */
>> +  if (__builtin_expect (__fcntl_nocancel (fd, F_GETFD), 0) == -1
>>        && errno == EBADF)
>>      {
>>        const char *name;
>
> I think we could just omit the comment, the function name is explicit enough.

I dropped the comment (here and elsewhere).

>> -    fd = __open (name, O_RDONLY | O_CLOEXEC);
>> +    fd = __open_nocancel (name, O_RDONLY | O_CLOEXEC);
>
> I think there is no reason to continue using non-LFS interface on loader, so
> change to __open64_nocancel (so eventually we can get rid of non-LFS no cancel
> implementations).

Done (here and elsewhere).

>> +#if IS_IN (rtld)
>> +# error "Cancellation should not occur within rtld"
>> +#endif
>
> Not sure if this check is really required, since cancellation hooks for
> enable/disable will be empty definitions for loader.

I dropped this.

>> -      && __builtin_expect (__fcntl (fd, F_SETFD, FD_CLOEXEC), 0) < 0)
>> +      && __builtin_expect (__fcntl_nocancel (fd, F_SETFD, FD_CLOEXEC), 0) < 0)
>
> Use __glibc_likely.

Done.

>> -/* Copyright (C) 2000-2018 Free Software Foundation, Inc.
>> +/* Linux close syscall implementation.
>
> I think it is worth to add it has no cancellable semantic.

Done (here and elsewhere).  Also I added one-line header comments to
all the files that didn't have them, corrected the copyright dates,
and removed stale 'Contributed by' lines.

> Ok, although it will incur in a slight code increase due __open_nocancel and
> __open64_nocancel not being alias anymore for __OFF_T_MATCHES_OFF64_T
> architectures (same applies for openat64).

No, they are still aliases in that case.

>> +/* In the PowerPC64 ABI, the unadorned F_GETLK* opcodes should be used
>> +   even by largefile64 code.  */
>> +#define FCNTL_ADJUST_CMD(__cmd)                      \
>> +  ({ int cmd_ = (__cmd);                     \
>> +     cmd_ == F_GETLK64 ? F_GETLK :           \
>> +       cmd_ == F_SETLK64 ? F_SETLK :         \
>> +       cmd_ == F_SETLKW64 ? F_SETLKW : cmd_ })
>> +
>> +
>
> I am getting a:
>
> error: expected ‘;’ before ‘}’ token
>         cmd_ == F_SETLKW64 ? F_SETLKW : cmd_ })
>
> with this snippet

Fixed.

> and I am not sure it is worth to change this code on this patch.

I think this is an appropriate change because, with this patch,
FCNTL_ADJUST_CMD is called twice for ordinary fcntl (because fcntl now
calls __fcntl_nocancel, and both of them need to apply
FCNTL_ADJUST_CMD).  So it is important to make it *obviously* an
idempotent operation.  The old code was idempotent but not obviously so.

> Also, I think a better option would to place it on kernel-features.h
> (since it usually the place to define architecture kernel abi
> idiossiacrassies).

Done.

zw