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: Thursday, January 30, 2025 2:54 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 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.

I agree, I should improve the commit message.  I like your suggestion for a new version, 
but I also noticed that we can probably deactivate the register set if we try to write them, too
(on a system with older kernel) or I don't remember why I didn't cover this scenario yet.

So I will double check and apply this in the next version of this series, where I will also enhance the
commit message.

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