Re: [RFC PATCH glibc 1/3] glibc: Perform rseq(2) registration at C startup and thread creation (v18)

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- On Apr 30, 2020, at 1:46 PM, Florian Weimer fweimer@redhat.com wrote:

> * Mathieu Desnoyers:
> 
>> ----- On Apr 30, 2020, at 1:07 PM, Florian Weimer fweimer@redhat.com wrote:
>> [...]
>>> __libc_fatal does not attribute the error to glibc, so I suggest to
>>> start the error messages with “glibc fatal error: ”, so that people know
>>> where to look.
>>
>> OK. Is there a strict requirement on limiting to 80 columns for code
>> including an error message string in glibc ? IOW:
>>
>>   if (__rseq_abi.cpu_id != RSEQ_CPU_ID_UNINITIALIZED)
>>     __libc_fatal ("glibc fatal error: rseq already initialized for this thread\n");
>>
>> or
>>
>>   if (__rseq_abi.cpu_id != RSEQ_CPU_ID_UNINITIALIZED)
>>     __libc_fatal ("glibc fatal error: "
>>                   "rseq already initialized for this thread\n");
>>
>> ?
> 
> The latter, please.  Some code also uses
> 
>  if (__rseq_abi.cpu_id != RSEQ_CPU_ID_UNINITIALIZED)
>     __libc_fatal ("\
> glibc fatal error: rseq already initialized for this thread\n");
> 
> But that's not really my preference.
> 
> (Trimmed the Cc: list a bit, we are really down to glibc specifics at
> this point.)

One last question with respect to handling of rseq errno values. We currently
have (based on my own rseq(2) man page, not upstream yet):

ERRORS
       EINVAL Either flags contains an invalid value, or rseq contains an address which is not appropriately  aligned,
              or rseq_len contains a size that does not match the size received on registration.

       ENOSYS The rseq() system call is not implemented by this kernel.

       EFAULT rseq is an invalid address.

       EBUSY  Restartable sequence is already registered for this thread.

       EPERM  The sig argument on unregistration does not match the signature received on registration.

So with the current suggestions, we basically treat "EBUSY" as a __libc_fatal (),
which is fine, and all other errno values (EINVAL, ENOSYS, EFAULT, EPERM) as
conditions which will just disable rseq for the thread by marking cpu_id as
RSEQ_CPU_ID_REGISTRATION_FAILED.

I'm hesitant to treat "EINVAL", and "EFAULT" in this way, as those errno should IMHO
really abort libc as well with an appropriate __libc_fatal () message, because something
is clearly going wrong and we don't want to hide it under the carpet by just
disabling rseq support silently.

Also, I personally consider that adding an additional errno value
to an existing system call for a given set of supported system call
parameters is an ABI breakage, but I _know_ the Linux kernel community
as a whole does not feel that way, and they are known to have pretty much
silently added additional errno values to existing system calls as long
as nobody complains.

Considering this, I wonder if we should be strict and e.g. do:

const char *msg = NULL;

switch (INTERNAL_SYSCALL_ERRNO (ret))
  {
  case ENOSYS:
  case EPERM:
    /* rseq system call is unavailable or not permitted.  */
    __rseq_abi.cpu_id = RSEQ_CPU_ID_REGISTRATION_FAILED;
    break;
  case EINVAL:
    msg = "glibc fatal error: rseq already registered for this thread\n";
    break;
  case EBUSY:
    msg = "glibc fatal error: rseq parameters are invalid";
  case EFAULT:
    msg = "glibc fatal error: rseq is an invalid address";
    break;
  default:
    msg = "glibc fatal error: unexpected rseq errno";
    break;
  }
if (msg)
  __libc_fatal (msg);

Also considering that __libc_fatal only takes a string as parameter,
I wonder if there is a facility to print the errno string I could use
instead of __libc_fatal () ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com