diff mbox series

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

Message ID 20241220200501.324191-3-christina.schimpe@intel.com
State New
Headers
Series Add CET shadow stack support |

Commit Message

Schimpe, Christina Dec. 20, 2024, 8:04 p.m. UTC 
  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(-)

Comments

Guinevere Larsen Jan. 28, 2025, 1:35 p.m. UTC | #1 
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.
Schimpe, Christina Jan. 30, 2025, 10:28 a.m. UTC | #2 
> -----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
Guinevere Larsen Jan. 30, 2025, 1:53 p.m. UTC | #3 
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.
Schimpe, Christina Jan. 30, 2025, 5:43 p.m. UTC | #4 
> -----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
Thiago Jung Bauermann Feb. 6, 2025, 2:59 a.m. UTC | #5 
"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
Schimpe, Christina Feb. 6, 2025, 12:15 p.m. UTC | #6 
> -----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
diff mbox series

Patch

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);
 	    }
-	  else if (errno == ENODATA)
+	  else if (errno == ENODATA
+		   || (errno == ENODEV
+		       && regset->type == OPTIONAL_RUNTIME_REGS)
+		   || errno == ESRCH)
 	    {
-	      /* ENODATA may be returned if the regset is currently
-		 not "active".  This can happen in normal operation,
-		 so suppress the warning in this case.  */
-	    }
-	  else if (errno == ESRCH)
-	    {
-	      /* At this point, ESRCH should mean the process is
-		 already gone, in which case we simply ignore attempts
-		 to read its registers.  */
+	      /* 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.
+		 ESRCH should mean the process is already gone at this
+		 point, in which case we simply ignore attempts to read
+		 its registers.  */
 	    }
 	  else
 	    {
@@ -5111,6 +5119,14 @@  regsets_store_inferior_registers (struct regsets_info *regsets_info,
 		 optional, do not try it again for this process mode.  */
 	      disable_regset (regsets_info, regset);
 	    }
+	  else if (errno == ENODEV
+		   && regset->type == OPTIONAL_RUNTIME_REGS)
+	    {
+	      /* If we get ENODEV on a regset and the regset can be
+		 enabled at runtime try it again for this process mode.
+		 This can happen in normal operation, so suppress the
+		 warning in this case.  */
+	    }
 	  else if (errno == ESRCH)
 	    {
 	      /* At this point, ESRCH should mean the process is
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 5be00b8c98c..da5aa26a993 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -42,7 +42,12 @@  enum regset_type {
   GENERAL_REGS,
   FP_REGS,
   EXTENDED_REGS,
-  OPTIONAL_REGS, /* Do not error if the regset cannot be accessed.  */
+  OPTIONAL_REGS, /* Do not error if the regset cannot be accessed.
+		    Disable the regset instead.  */
+  OPTIONAL_RUNTIME_REGS, /* Some optional regsets can only be accessed
+			    dependent on the execution flow.  For such
+			    access errors don't show a warning and don't
+			    disable the regset.  */
 };
 
 /* The arch's regsets array initializer must be terminated with a NULL