gdb/linux-nat: Fix mem access ptrace fallback (PR threads/31579)

  Old RHEL systems have a kernel that does not support writing memory
via /proc/pid/mem.  On such systems, we fallback to accessing memory
via ptrace.  That has a few downsides described in the "Accessing
inferior memory" section at the top of linux-nat.c.

The target_xfer interface for memory access uses inferior_ptid as
sideband argument to indicate which process to access.  Memory access
is process-wide, it is not thread-specific, so inferior_ptid is
sometimes pointed at a process-wide ptid_t for the memory access
(i.e., a ptid that looks like {pid, 0, 0}).  That is the case for any
code that uses scoped_restore_current_inferior_for_memory, for
example.

That is what causes the issue described in PR 31579, where thread_db
calls into the debugger to read memory, which reaches our
ps_xfer_memory function, which does:

  static ps_err_e
  ps_xfer_memory (const struct ps_prochandle *ph, psaddr_t addr,
		  gdb_byte *buf, size_t len, int write)
  {
    scoped_restore_current_inferior_for_memory save_inferior (ph->thread->inf);

    ...
      ret = target_read_memory (core_addr, buf, len);
    ...
  }

If linux_nat_target::xfer_partial falls back to inf_ptrace_target with
a pid-ptid, then the ptrace code will do the ptrace call targeting
pid, the leader LWP.  That may fail with ESRCH if the leader is
currently running, or zombie.  That is the case in the scenario in
question, because thread_db is consulted for an event of a non-leader
thread, before we've stopped the whole process.

Fix this by having the ptrace fallback code try to find a stopped LWP
to use with ptrace.

I chose to handle this in the linux-nat target instead of in common
code because (global) memory is a process-wide property, and this
avoids having to teach all the code paths that use
scoped_restore_current_inferior_for_memory to find some stopped thread
to access memory through, which is a ptrace quirk.  That is
effectively what we used to do before we started relying on writable
/proc/pid/mem.  I'd rather not go back there.

To trigger this on modern kernels you have to hack linux-nat.c to
force the ptrace fallback code, like so:

 --- a/gdb/linux-nat.c
 +++ b/gdb/linux-nat.c
 @@ -3921,7 +3921,7 @@ linux_nat_target::xfer_partial (enum target_object object,
	  poke would incorrectly write memory to the post-exec address
	  space, while the core was trying to write to the pre-exec
	  address space.  */
 -      if (proc_mem_file_is_writable ())
 +      if (0 && proc_mem_file_is_writable ())

With that hack, I was able to confirm that the fix fixes hundreds of
testsuite failures.  Compared to a test run with pristine master, the
hack above + this commit's fix shows that some non-stop-related tests
fail, but that is expected, because those are tests that need to write
memory while the program is running.  (I made no effort to temporarily
pause an lwp if no ptrace-stopped lwp is found.)

Change-Id: I24a4f558e248aff7bc7c514a88c698f379f23180
---
 gdb/linux-nat.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 58 insertions(+), 3 deletions(-)


base-commit: cafca5eaa068eaaa3e3a2ffab356efb4714c2968
  

Am Donnerstag, 11. April 2024 um 13:40:25 MESZ hat Pedro Alves <pedro@palves.net> Folgendes geschrieben:

> Old RHEL systems have a kernel that does not support writing memory
> via /proc/pid/mem.  On such systems, we fallback to accessing memory
> via ptrace.  That has a few downsides described in the "Accessing
> inferior memory" section at the top of linux-nat.c.
>
> The target_xfer interface for memory access uses inferior_ptid as
> sideband argument to indicate which process to access.  Memory access
> is process-wide, it is not thread-specific, so inferior_ptid is
> sometimes pointed at a process-wide ptid_t for the memory access
> (i.e., a ptid that looks like {pid, 0, 0}).  That is the case for any
> code that uses scoped_restore_current_inferior_for_memory, for
> example.
>
> That is what causes the issue described in PR 31579, where thread_db
> calls into the debugger to read memory, which reaches our
> ps_xfer_memory function, which does:
>
>   static ps_err_e
>   ps_xfer_memory (const struct ps_prochandle *ph, psaddr_t addr,
>           gdb_byte *buf, size_t len, int write)
>   {
>     scoped_restore_current_inferior_for_memory save_inferior (ph->thread->inf);
>
>     ...
>       ret = target_read_memory (core_addr, buf, len);
>     ...
>   }
>
> If linux_nat_target::xfer_partial falls back to inf_ptrace_target with
> a pid-ptid, then the ptrace code will do the ptrace call targeting
> pid, the leader LWP.  That may fail with ESRCH if the leader is
> currently running, or zombie.  That is the case in the scenario in
> question, because thread_db is consulted for an event of a non-leader
> thread, before we've stopped the whole process.
>
> Fix this by having the ptrace fallback code try to find a stopped LWP
> to use with ptrace.
>
> I chose to handle this in the linux-nat target instead of in common
> code because (global) memory is a process-wide property, and this
> avoids having to teach all the code paths that use
> scoped_restore_current_inferior_for_memory to find some stopped thread
> to access memory through, which is a ptrace quirk.  That is
> effectively what we used to do before we started relying on writable
> /proc/pid/mem.  I'd rather not go back there.
>
> To trigger this on modern kernels you have to hack linux-nat.c to
> force the ptrace fallback code, like so:
>
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -3921,7 +3921,7 @@ linux_nat_target::xfer_partial (enum target_object object,
>       poke would incorrectly write memory to the post-exec address
>       space, while the core was trying to write to the pre-exec
>       address space.  */
> -      if (proc_mem_file_is_writable ())
> +      if (0 && proc_mem_file_is_writable ())
>
> With that hack, I was able to confirm that the fix fixes hundreds of
> testsuite failures.  Compared to a test run with pristine master, the
> hack above + this commit's fix shows that some non-stop-related tests
> fail, but that is expected, because those are tests that need to write
> memory while the program is running.  (I made no effort to temporarily
> pause an lwp if no ptrace-stopped lwp is found.)

I can confirm that this fixes the problem for me, thanks.

Tested-By: Hannes Domani <ssbssa@yahoo.de>
  

Pedro Alves <pedro@palves.net> writes:

>
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 2602e1f240d..d2b9aea724d 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c

> @@ -2199,6 +2199,21 @@ mark_lwp_dead (lwp_info *lp, int status)
>    lp->stopped = 1;
>  }
>  
> +/* Return true if LP is dead, with a pending exit/signalled event.  */
> +
> +static bool
> +is_lwp_marked_dead (lwp_info *lp)
> +{
> +  switch (lp->waitstatus.kind ())
> +    {
> +    case TARGET_WAITKIND_EXITED:
> +    case TARGET_WAITKIND_THREAD_EXITED:
> +    case TARGET_WAITKIND_SIGNALLED:
> +      return true;
> +    }
> +  return false;
> +}

I wonder if this would be better as a method on waitstatus?  There's at
least one place in infrun.c (in handle_one) where we check for these
three statuses, and there's a few places where we check
TARGET_WAITKIND_EXITED and TARGET_WAITKIND_SIGNALLED ... and I suspect
we either _should_ be checking for TARGET_WAITKIND_THREAD_EXITED, or it
wouldn't matter if we did.

Not that I'm suggesting you should look at all those ... just I'm
wondering if it would be better to make is_lwp_marked_dead a waitstatus
method now, and we can then clean up other users later?

Consider this an optional suggestion, we can always make this a method
later if needed.

> +
>  /* Wait for LP to stop.  Returns the wait status, or 0 if the LWP has
>     exited.  */
>  
> @@ -3879,6 +3894,20 @@ linux_proc_xfer_memory_partial (int pid, gdb_byte *readbuf,
>  				const gdb_byte *writebuf, ULONGEST offset,
>  				LONGEST len, ULONGEST *xfered_len);
>  
> +/* Look for an LWP of PID that we know is ptrace-stopped.  Returns
> +   NULL if none is found.  */

s/NULL/nullptr/ ?

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew
  

On 2024-04-12 17:05, Andrew Burgess wrote:
> Pedro Alves <pedro@palves.net> writes:
> 
>>
>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>> index 2602e1f240d..d2b9aea724d 100644
>> --- a/gdb/linux-nat.c
>> +++ b/gdb/linux-nat.c
> 
>> @@ -2199,6 +2199,21 @@ mark_lwp_dead (lwp_info *lp, int status)
>>    lp->stopped = 1;
>>  }
>>  
>> +/* Return true if LP is dead, with a pending exit/signalled event.  */
>> +
>> +static bool
>> +is_lwp_marked_dead (lwp_info *lp)
>> +{
>> +  switch (lp->waitstatus.kind ())
>> +    {
>> +    case TARGET_WAITKIND_EXITED:
>> +    case TARGET_WAITKIND_THREAD_EXITED:
>> +    case TARGET_WAITKIND_SIGNALLED:
>> +      return true;
>> +    }
>> +  return false;
>> +}
> 
> I wonder if this would be better as a method on waitstatus?  There's at
> least one place in infrun.c (in handle_one) where we check for these
> three statuses, and there's a few places where we check
> TARGET_WAITKIND_EXITED and TARGET_WAITKIND_SIGNALLED ... and I suspect
> we either _should_ be checking for TARGET_WAITKIND_THREAD_EXITED, or it
> wouldn't matter if we did.
> 
> Not that I'm suggesting you should look at all those ... just I'm
> wondering if it would be better to make is_lwp_marked_dead a waitstatus
> method now, and we can then clean up other users later?
> 

I've named the function is_lwp_marked_dead because we have the corresponding
mark_lwp_dead just above.  If we add a method to waitstatus, we'd naturally want
to call it something else, like ... I don't know, it's hard, maybe ws.is_exit_like()
or some such.  I think we'd still want the lwp function wrapping that, as the fact
that we mark is in the pending status field is a bit of a lower level implementation
detail than the semantics of the is_lwp_marked_dead function.

> Consider this an optional suggestion, we can always make this a method
> later if needed.

Thanks, seems worth it of exploration, but I'll take the option and go with what
I have in this patch.

> 
>> +
>>  /* Wait for LP to stop.  Returns the wait status, or 0 if the LWP has
>>     exited.  */
>>  
>> @@ -3879,6 +3894,20 @@ linux_proc_xfer_memory_partial (int pid, gdb_byte *readbuf,
>>  				const gdb_byte *writebuf, ULONGEST offset,
>>  				LONGEST len, ULONGEST *xfered_len);
>>  
>> +/* Look for an LWP of PID that we know is ptrace-stopped.  Returns
>> +   NULL if none is found.  */
> 
> s/NULL/nullptr/ ?

Personally, I prefer writing NULL in comments than nullptr, because saying (I mean,
imagine reading the comment aloud) "null ptr" is kind of awkward, in my mind.
It see it more as NULL meaning "the null pointer value", i.e., the word "null"
uppercased, than literally the NULL macro.  I've seen other patches from others
using NULL in comments so I thought it was other's preference as well.

> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks.
  

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

>> s/NULL/nullptr/ ?

Pedro> Personally, I prefer writing NULL in comments than nullptr, because saying (I mean,
Pedro> imagine reading the comment aloud) "null ptr" is kind of awkward, in my mind.
Pedro> It see it more as NULL meaning "the null pointer value", i.e., the word "null"
Pedro> uppercased, than literally the NULL macro.  I've seen other patches from others
Pedro> using NULL in comments so I thought it was other's preference as well.

Yeah, I prefer this in comments as well.  I'm not really sure why but
maybe because the all-caps makes it more distinct.

Tom