gdb/linux-nat.c: Add "Accessing inferior memory" section

  This commit adds a new "Accessing inferior memory" comment section to
gdb/linux-nat.c that explains why we prefer /proc/pid/mem over
alternatives.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30453

Change-Id: I575b21ed697a85f3ff4c0ec58c04812db5005b76
---
 gdb/linux-nat.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)


base-commit: 23acbfee6a82cc147b04b74a89d5b34b47c150f4
  

Internal Use - Confidential
> -----Original Message-----
> From: Pedro Alves <pedro@palves.net>
> Sent: Wednesday, February 21, 2024 3:29 PM
> To: gdb-patches@sourceware.org
> Subject: [PATCH] gdb/linux-nat.c: Add "Accessing inferior memory" section
>
>
> [EXTERNAL EMAIL]
>
> This commit adds a new "Accessing inferior memory" comment section to
> gdb/linux-nat.c that explains why we prefer /proc/pid/mem over
> alternatives.
>
> Bug:
> https://urldefense.com/v3/__https://sourceware.org/bugzilla/show_bug.cg
> i?id=30453__;!!LpKI!kgnQq3OesZh1u3bZMiMlYFRFw8wKO2JalIjNt8elbNGzCR
> ybBEhn7fUGW68WD9BZqTSpxUNpMS92G2nfHQ$ [sourceware[.]org]
>
> Change-Id: I575b21ed697a85f3ff4c0ec58c04812db5005b76
> ---
>  gdb/linux-nat.c | 58
> ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c index e91c57ba239..21928c681ef
> 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -180,7 +180,63 @@ execing thread, the leader will be zombie, and the
> execing thread will  be in `D (disc sleep)' state.  As soon as all other threads
> are  reaped, the execing thread changes its tid to the tgid, and the  previous
> (zombie) leader vanishes, giving place to the "new"
> -leader.  */
> +leader.
> +
> +Accessing inferior memory
> +=========================
> +
> +To access inferior memory, we strongly prefer /proc/PID/mem.  We
> +fallback to ptrace if and only if /proc/PID/mem is not writable, as a
> +concession for obsolescent kernels (such as found in RHEL6).  For
> +modern kernels, the fallback shouldn't trigger.  GDBserver does not
> +have the ptrace fallback already, and at some point, we'll consider
> +removing it from native GDB too.
> +
> +/proc/PID/mem has a few advantages over alternatives like
> +PTRACE_PEEKTEXT/PTRACE_POKETEXT or
> process_vm_readv/process_vm_writev:
> +
> +- Because we can use a single read/write call, /proc/PID/mem can be
> +  much more efficient than banging away at
> +  PTRACE_PEEKTEXT/PTRACE_POKETEXT, one word at a time.
> +
> +- /proc/PID/mem allows writing to read-only pages, which we need to
> +  e.g., plant breakpoint instructions.  process_vm_writev does not
> +  allow this.
> +
> +- /proc/PID/mem allows memory access even if all threads are running.
> +  OTOH, PTRACE_PEEKTEXT/PTRACE_POKETEXT require passing down the
> tid
> +  of a stopped task.  This lets us e.g., install breakpoints while the
> +  inferior is running, clear a displaced stepping scratch pad when the
> +  thread that was displaced stepping exits, print inferior globals,
> +  etc., all without having to worry about temporarily pausing some
> +  thread.
> +
> +- /proc/PID/mem does not suffer from a race that could cause us to
> +  access memory of the wrong address space when the inferior execs.
> +
> +  process_vm_readv/process_vm_writev have this problem.
> +
> +  E.g., say GDB decides to write to memory just while the inferior
> + execs.  In this scenario, GDB could write memory to the post-exec
> + address space thinking it was writing to the pre-exec address space,
> + with high probability of corrupting the inferior.  Or of GDB decides

You probably mean "Or if GDB decides" here.

> + instead to read memory just while the inferior execs, it could read
> + bogus contents out of the wrong address space.
> +
> +  ptrace used to have this problem too, but no longer has since Linux
> + commit dbb5afad100a ("ptrace: make ptrace() fail if the tracee
> + changed its pid unexpectedly"), in Linux 5.13.  (And if ptrace were
> + ever changed to allow access memory via zombie or running threads,  it
> + would better not forget to consider this scenario.)
> +
> +  We avoid this race with /proc/PID/mem, by opening the file as soon
> + as we start debugging the inferior, when it is known the inferior is
> + stopped, and holding on to the open file descriptor, to be used
> + whenever we need to access inferior memory.  If the inferior execs  or
> + exits, reading/writing from/to the file returns 0 (EOF),  indicating
> + the address space is gone, and so we return  TARGET_XFER_EOF to the
> + core.  We close the old file and open a new  one when we finally see
> + the PTRACE_EVENT_EXEC event.  */
>
>  #ifndef O_LARGEFILE
>  #define O_LARGEFILE 0
>
> base-commit: 23acbfee6a82cc147b04b74a89d5b34b47c150f4
> --
> 2.43.2
  

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

Pedro> This commit adds a new "Accessing inferior memory" comment section to
Pedro> gdb/linux-nat.c that explains why we prefer /proc/pid/mem over
Pedro> alternatives.

Pedro> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30453

Aside from the typo pointed out by Mikhail, this looks good.
Thank you for writing this, this kind of commentary is very valuable.

Tom
  

On 2024-02-21 20:55, Terekhov, Mikhail wrote:

>> +  E.g., say GDB decides to write to memory just while the inferior
>> + execs.  In this scenario, GDB could write memory to the post-exec
>> + address space thinking it was writing to the pre-exec address space,
>> + with high probability of corrupting the inferior.  Or of GDB decides
> 
> You probably mean "Or if GDB decides" here.

Indeed I did.  Thank you.
  

On 2024-02-23 16:16, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> This commit adds a new "Accessing inferior memory" comment section to
> Pedro> gdb/linux-nat.c that explains why we prefer /proc/pid/mem over
> Pedro> alternatives.
> 
> Pedro> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30453
> 
> Aside from the typo pointed out by Mikhail, this looks good.
> Thank you for writing this, this kind of commentary is very valuable.

Thank you.  I've fixed the typo and merged this.