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

["Schimpe, Christina" <christina.schimpe@intel.com>]

> -----Original Message-----
> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Sent: Thursday, February 6, 2025 4:00 AM
> To: Schimpe, Christina <christina.schimpe@intel.com>
> Cc: Guinevere Larsen <guinevere@redhat.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH 02/12] gdbserver: Add optional runtime register set type.
> 
> 
> "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.  */

Hi Thiago, 

Thank you for the review.
I think we can add that sentence "which means that it's unsupported by the kernel", if it is generic behaviour for any regset.
This seems to be the case: https://github.com/torvalds/linux/blob/master/kernel/ptrace.c#L895

So I'd enhance the commit message but also the comment for the next version.

Christina

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928