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

[Guinevere Larsen <guinevere@redhat.com>]

On 1/30/25 7:28 AM, Schimpe, Christina wrote:
>> -----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.  */
> 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?

Yeah, ok, so the commit message needs an update.

Maybe something like:

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.  When
reading them, we shouldn't deactivate these regsets and should not show a
warning if they are deactivated, but should deactivate them if they are
unsupported by the kernel. That can be deciphered based on the error
returned by the ptrace call, if we fail to read the registered.

Or something similar.

I think it is important to explain in the commit message that one error 
means "inactive" while other means "unsupported", so that in 5-10 years 
we can look back at this commit and be sure the disable wasn't added 
incorrectly.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers