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

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

> -----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?

BR,
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