Re: [PATCH 02/12] gdbserver: Add optional runtime register set type.

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

"Schimpe, Christina" <christina.schimpe@intel.com> writes:

>> -----Original Message-----
>> From: Guinevere Larsen <guinevere@redhat.com>
>> Sent: Tuesday, January 28, 2025 2:35 PM
>> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
>> patches@sourceware.org
>> Subject: Re: [PATCH 02/12] gdbserver: Add optional runtime register set type.
>>
>> On 12/20/24 5:04 PM, Schimpe, Christina wrote:
>> > Some register sets can be activated and deactivated by the OS during
>> > the runtime of a process.  One example register is the Intel CET
>> > shadow stack pointer.  This adds a new type of register set to handle
>> > such cases.  We shouldn't deactivate these regsets and should not show
>> > a warning if we fail to read them.
>> > ---
>> >   gdbserver/linux-low.cc | 40 ++++++++++++++++++++++++++++------------
>> >   gdbserver/linux-low.h  |  7 ++++++-
>> >   2 files changed, 34 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc index
>> > 50ce2b44927..355b28d9fe4 100644
>> > --- a/gdbserver/linux-low.cc
>> > +++ b/gdbserver/linux-low.cc
>> > @@ -5007,23 +5007,31 @@ regsets_fetch_inferior_registers (struct
>> regsets_info *regsets_info,
>> >         if (res < 0)
>> >   	{
>> >   	  if (errno == EIO
>> > -	      || (errno == EINVAL && regset->type == OPTIONAL_REGS))
>> > +	      || (errno == EINVAL
>> > +		  && (regset->type == OPTIONAL_REGS
>> > +		      || regset->type == OPTIONAL_RUNTIME_REGS)))
>> >   	    {
>> >   	      /* If we get EIO on a regset, or an EINVAL and the regset is
>> > -		 optional, do not try it again for this process mode.  */
>> > +		 optional, do not try it again for this process mode.
>> > +		 Even if the regset can be enabled at runtime it is safe
>> > +		 to deactivate the regset in case of EINVAL, as we know
>> > +		 the regset itself was the invalid argument of the ptrace
>> > +		 call.  */
>> >   	      disable_regset (regsets_info, regset);
>>
>> I'm somewhat confused by this patch.
>>
>> The commit message and other comments here say that optional_runtime_regs
>> shouldn't be disabled. However, in here, if we get EINVAL we *will* disable the
>> regset. Did you mean to use != instead of == ?
>>
>> I'll be honest, I don't know enough about the regset subsystem to know which is
>> the correct option, I just think it has to be consistent.
>>
>
> Hi Guinevere,
>
> Thank you for the review.
>
> For the errno EINVAL we want to disable the regset, as we don't want to call ptrace
> using NT_X86_SHSTK again. This specific errno can happen if the kernel does not
> support ptrace using NT_X86_SHSTK (older linux kernels). I tried to explain that here:
>
>> > +		 Even if the regset can be enabled at runtime it is safe
>> > +		 to deactivate the regset in case of EINVAL, as we know
>> > +		 the regset itself was the invalid argument of the ptrace
>> > +		 call.  */

I agree with Guinevere's remarks, and also with the suggestion of
improving the commit message.

But I would also like to suggest being a bit more direct in the comment
above. At least the way I read it, I thought that EINVAL was a normal
thing for ptrace to return if shadow stack was disabled for the
process. What about something like:

  Even if the regset can be enabled at runtime it is safe
  to deactivate the regset in case of EINVAL, as we know
  the regset itself was the invalid argument of the ptrace
  call which means that it's unsupported by the kernel.  */

> In case shadow stack is just not active but supported by the kernel we see
> ENODEV and we don't disable the regset, which I explained in another
> comment for the corresponding code area:
>
> /* ENODATA or ENODEV may be returned if the regset is
>      currently not "active".  For ENODEV we additionally check
>      if the register set is of type OPTIONAL_RUNTIME_REGS.
>      This can happen in normal operation, so suppress the
>      warning in this case.
>
> I didn't want to be too specific here to make the commit generic.
>
> Is there something I could add to make it more understandable or maybe
> there is just some information missing in the commit message?

--
Thiago