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

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

[Pedro Alves]

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(-)

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

RE: [PATCH] gdb/linux-nat.c: Add "Accessing inferior memory" section

[Terekhov, Mikhail]

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

Re: [PATCH] gdb/linux-nat.c: Add "Accessing inferior memory" section

[Tom Tromey]

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

Re: [PATCH] gdb/linux-nat.c: Add "Accessing inferior memory" section

[Pedro Alves]

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.

Re: [PATCH] gdb/linux-nat.c: Add "Accessing inferior memory" section

[Pedro Alves]

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.