Re: [PATCH v2 4/5] Extend GNU/Linux to check for ptrace error

[Pedro Alves <palves@redhat.com>]

On 3/17/20 3:47 PM, Sergio Durigan Junior via Gdb-patches wrote:
> This patch implements the ptrace-errno-checking on the GNU/Linux
> target (both native and remote).  It builds on top of the previous
> 'fork_inferior' extension patch.
> 
> The idea is to provide a new 'traceme_fun' for each ptrace backend,
> which will accept a new integer argument representing the write end of
> the ptrace status pipe (that was created in 'fork_inferior').  This
> function will invoke the actual tracing syscall (which is 'ptrace' in
> this case), get its errno value and write it back via the pipe.  You
> can see examples of this new approach by looking at
> 'inf_ptrace_me' (GDB) or 'linux_ptrace_fun' (gdbserver).
> 
> The rest of the patch implements the necessary machinery to do
> something useful with the errno information that we received from
> 'ptrace'.
> 
> In Fedora GDB, we carry the following patch:
> 
>   https://src.fedoraproject.org/rpms/gdb/blob/8ac06474ff1e2aa4920d14e0666b083eeaca8952/f/gdb-attach-fail-reasons-5of5.patch
> 
> Its purpose is to try to detect a specific scenario where SELinux's
> 'deny_ptrace' option is enabled, which prevents GDB from ptrace'ing in
> order to debug the inferior (PTRACE_ATTACH and PTRACE_TRACEME will
> fail with EACCES in this case).
> 
> I like the idea of improving error detection and providing more
> information to the user (a simple "Permission denied" can be really
> frustrating), but I don't fully agree with the way the patch was
> implemented: it makes GDB link against libselinux only for the sake of
> consulting the 'deny_ptrace' setting, and then prints a warning if
> ptrace failed and this setting is on.
> 
> There is now a new function, 'linux_ptrace_restricted_fail_reason',
> which does a few things to check what's wrong with ptrace:
> 
>   - It dlopen's "libselinux.so.1" and checks if the "deny_ptrace"
>     option is enabled.
> 
>   - It reads the contents of "/proc/sys/kernel/yama/ptrace_scope" and
>     checks if it's different than 0.
> 
> For each of these checks, if it succeeds, the user will see a message
> informing about the restriction in place, and how it can be disabled.
> For example, if "deny_ptrace" is enabled, the user will see:
> 
>   # gdb /usr/bin/true
>   ...
>   (gdb) run
>   Starting program: /usr/bin/true
>   warning: Could not trace the inferior process.
>   warning: ptrace: Permission denied
> 
>   The SELinux 'deny_ptrace' option is enabled and preventing GDB
>   from using 'ptrace'.  You can disable it by executing (as root):
> 
>     setsebool deny_ptrace off
> 
>   If you are debugging the inferior remotely, the ptrace restriction(s) must
>   be disabled in the target system (e.g., where GDBserver is running).
> 
> In case "/proc/sys/kernel/yama/ptrace_scope" is > 0:
> 
>   # gdb /usr/bin/true
>   ...
>   (gdb) run
>   Starting program: /usr/bin/true
>   warning: Could not trace the inferior process.
>   warning: ptrace: Operation not permitted
> 
>   The Linux kernel's Yama ptrace scope is in effect, which can prevent
>   GDB from using 'ptrace'.  You can disable it by executing (as root):
> 
>     echo 0 > /proc/sys/kernel/yama/ptrace_scope
> 
>   If you are debugging the inferior remotely, the ptrace restriction(s) must
>   be disabled in the target system (e.g., where GDBserver is running).
> 
> If both restrictions are enabled, both messages will show up.
> 
> This works for gdbserver as well, and actually fixes a latent bug I
> found: when ptrace is restricted, gdbserver would hang due to an
> unchecked ptrace call:
> 
>   # gdbserver :9988 /usr/bin/true
>   gdbserver: linux_ptrace_test_ret_to_nx: Cannot PTRACE_TRACEME: Operation not permitted
>   gdbserver: linux_ptrace_test_ret_to_nx: status 256 is not WIFSTOPPED!
>   gdbserver: linux_ptrace_test_ret_to_nx: failed to kill child pid 2668100 No such process
>   [ Here you would have to issue a C-c ]
> 
> Now, you will see:
> 
>   # gdbserver :9988 /usr/bin/true
>   gdbserver: linux_ptrace_test_ret_to_nx: Cannot PTRACE_TRACEME: Permission denied
>   gdbserver: linux_ptrace_test_ret_to_nx: status 256 is not WIFSTOPPED!
>   gdbserver: linux_ptrace_test_ret_to_nx: failed to kill child pid 2766868 No such process
>   gdbserver: Could not trace the inferior process.
>   gdbserver: ptrace: Permission denied
> 
>   The SELinux 'deny_ptrace' option is enabled and preventing GDB
>   from using 'ptrace'.  You can disable it by executing (as root):
> 
>     setsebool deny_ptrace off
> 
>   If you are debugging the inferior remotely, the ptrace restriction(s) need
>   to be disabled in the target system (e.g., where GDBserver is running).
>   Exiting.
>   #
> 
> (I decided to keep all the other messages, even though I find them a
> bit distracting).
> 
> If GDB can't determine the cause for the failure, it will still print
> the generic error message which tells the user to check our
> documentation:
> 
>   There might be restrictions preventing ptrace from working.  Please see
>   the appendix "Linux kernel ptrace restrictions" in the GDB documentation
>   for more details.
>   If you are debugging the inferior remotely, the ptrace restriction(s) need
>   to be disabled in the target system (e.g., where GDBserver is running).
> 
> This means that the series expands our documentation (in the next
> patch) and creates a new appendix section named "Linux kernel ptrace
> restrictions", with sub-sections for each possible restriction that
> might be in place.
> 
> Notice how, on every message, we instruct the user to "do the right
> thing" if gdbserver is being used.  This is because if the user
> started gdbserver *before* any ptrace restriction was in place, and
> then, for some reason, one or more restrictions get enabled, then the
> error message will be displayed both on gdbserver *and* on the
> connected GDB.  Since the user will be piloting GDB, it's important to
> explicitly say that the ptrace restrictions are enabled in the target,
> where gdbserver is running.
> 
> The current list of possible restrictions is:
> 
>   - SELinux's 'deny_ptrace' option (detected).
> 
>   - YAMA's /proc/sys/kernel/yama/ptrace_scope setting (detected).
> 
>   - seccomp on Docker containers (I couldn't find how to detect).
> 
> It's important to mention that all of this is Linux-specific; as far
> as I know, SELinux, YAMA and seccomp are Linux-only features.
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* inf-ptrace.c: Include "nat/fork-inferior.h".
> 	(inf_ptrace_me): New parameter "trace_errno_wpipe".  Check
> 	"ptrace" errno.
> 	(inf_ptrace_target::attach): Rewrite to use
> 	"inf_ptrace_target::ptrace_attach".
> 	(inf_ptrace_target::ptrace_attach): New function, almost
> 	identical to the previous "inf_ptrace_target::attach".
> 	* inf-ptrace.h (struct inf_ptrace_target) <int ptrace_attach>:
> 	New method.
> 	* linux-nat.c: Include "nat/fork-inferior.h".
> 	(attach_proc_task_lwp_callback): Call
> 	"linux_ptrace_attach_fail_reason_lwp" instead of
> 	"linux_ptrace_attach_fail_reason_string".
> 	(linux_nat_target::attach): Save "ERRNO".  Pass it to
> 	"linux_ptrace_attach_fail_reason".
> 	(_initialize_linux_nat): Set "trace_me_fail_reason".
> 	* nat/linux-ptrace.c: Include "gdbsupport/gdb-dlfcn.h",
> 	"nat/fork-inferior.h" and "gdbsupport/filestuff.h".
> 	(selinux_ftype): New type.
> 	(linux_ptrace_restricted_fail_reason): New function.
> 	(linux_ptrace_attach_fail_reason_1): New function, renamed
> 	from "linux_ptrace_attach_fail_reason".
> 	(linux_ptrace_attach_fail_reason): New function.
> 	(linux_ptrace_attach_fail_reason_lwp): Likewise.
> 	(linux_ptrace_me_fail_reason): Likewise.
> 	(errno_pipe): New variable.
> 	(linux_child_function): Check "ptrace" errno.  Send it through
> 	the pipe.
> 	(linux_check_ptrace_features): Initialize pipe.  Check
> 	"ptrace" errno sent through the pipe.
> 	* nat/linux-ptrace.h (linux_ptrace_attach_fail_reason): New
> 	function.
> 	(linux_ptrace_attach_fail_reason_lwp): Likewise.
> 	(linux_ptrace_me_fail_reason): Likewise.
> 	* remote.c (extended_remote_target::attach): Check error
> 	message on PACKET_ERROR.
> 	(remote_target::extended_remote_run): Likewise.
> 
> gdbserver/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* linux-low.cc (linux_ptrace_fun): New parameter
> 	"trace_errno_wpipe".  Check "ptrace" errno.
> 	(attach_proc_task_lwp_callback): Call
> 	"linux_ptrace_attach_fail_reason_lwp" instead of
> 	"linux_ptrace_attach_fail_reason_string".
> 	(linux_process_target::attach): Likewise.
> 	(initialize_low): Set "trace_me_fail_reason".
> 	* server.cc (handle_v_attach): Check if "attach_inferior"
> 	succeeded.
> 	(handle_v_run): Likewise.
> 	* thread-db.cc (attach_thread): Call
> 	"linux_ptrace_attach_fail_reason_lwp" instead of
> 	"linux_ptrace_attach_fail_reason_string".
> ---
>  gdb/inf-ptrace.c        |  34 +++++++-
>  gdb/inf-ptrace.h        |   2 +
>  gdb/linux-nat.c         |  24 +++---
>  gdb/nat/fork-inferior.c |   6 +-
>  gdb/nat/fork-inferior.h |   2 +-
>  gdb/nat/linux-ptrace.c  | 178 ++++++++++++++++++++++++++++++++++++++--
>  gdb/nat/linux-ptrace.h  |  27 ++++--
>  gdb/remote.c            |  40 ++++++++-
>  gdbserver/linux-low.cc  |  31 +++++--
>  gdbserver/server.cc     |  38 ++++++++-
>  gdbserver/thread-db.cc  |   2 +-
>  11 files changed, 339 insertions(+), 45 deletions(-)
> 
> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
> index db17a76d94..941f019709 100644
> --- a/gdb/inf-ptrace.c
> +++ b/gdb/inf-ptrace.c
> @@ -34,6 +34,7 @@
>  #include "nat/fork-inferior.h"
>  #include "utils.h"
>  #include "gdbarch.h"
> +#include "nat/fork-inferior.h"
>  
>  \f
>  
> @@ -97,10 +98,23 @@ inf_ptrace_target::remove_fork_catchpoint (int pid)
>  /* Prepare to be traced.  */
>  
>  static void
> -inf_ptrace_me (void)
> +inf_ptrace_me (int trace_errno_wpipe)
>  {
>    /* "Trace me, Dr. Memory!"  */
> -  if (ptrace (PT_TRACE_ME, 0, (PTRACE_TYPE_ARG3) 0, 0) < 0)
> +  int ret = ptrace (PT_TRACE_ME, 0, (PTRACE_TYPE_ARG3) 0, 0);
> +  int ptrace_errno = ret < 0 ? errno : 0;
> +
> +  try
> +    {
> +      write_trace_errno_to_pipe (trace_errno_wpipe, ptrace_errno);
> +    }
> +  catch (const gdb_exception &e)
> +    {
> +      warning ("%s", e.what ());
> +      _exit (0177);
> +    }

We've been avoiding try/catch in an async-signal-safe-only environment
(between fork and exec/exit).  This is another spot leaking in.

AFAICT, all cases of write_trace_errno_to_pipe throwing end up
caught be the immediate caller catching the error, warning and
calling _exit.  So how about doing the warning+_exit directly
within write_trace_errno_to_pipe and thus avoid the try/catch?

> +
> +  if (ret < 0)
>      trace_start_error_with_name ("ptrace");

Seems like errno was already lost when you get here, since
write_trace_errno_to_pipe clobbers errno.

BTW, you could do with a single ret < 0 check, I think?  Like:

  if (ret < 0)
   {
     int ptrace_errno = errno;

     write_trace_errno_to_pipe (trace_errno_wpipe, ptrace_errno);

     errno = ptrace_errno;

     trace_start_error_with_name ("ptrace");
   }

Though you could make (and document) write_trace_errno_to_pipe
preserve errno itself.  I'd even consider removing its
trace_errno parameter, resulting in this on the caller side:

  if (ret < 0)
   {
     write_trace_errno_to_pipe (trace_errno_wpipe);
     trace_start_error_with_name ("ptrace");
   }

Note how trace_start_error_with_name already uses the global
errno, so it wouldn't be strange.


>  }
>  
> @@ -185,6 +199,18 @@ inf_ptrace_target::mourn_inferior ()
>  
>  void
>  inf_ptrace_target::attach (const char *args, int from_tty)
> +{
> +   errno = ptrace_attach (args, from_tty);
> +   if (errno != 0)
> +     perror_with_name (("ptrace"));
> +}
> +
> +/* Attach to the process specified by ARGS.  If FROM_TTY is non-zero,
> +   be chatty about it.  Return ERRNO if the call to ptrace failed; 0
> +   otherwise.  */

This should also mention that we throw an error for other reasons.

Something like:

  Returns ERRNO if the call to ptrace failed; 0 if ptrace succeeded.
  Throws an error if it fails for reasons other than a ptrace failure.

> +
> +int
> +inf_ptrace_target::ptrace_attach (const char *args, int from_tty)
>  {
>    pid_t pid;
>    struct inferior *inf;
> @@ -223,7 +249,7 @@ inf_ptrace_target::attach (const char *args, int from_tty)
>    errno = 0;
>    ptrace (PT_ATTACH, pid, (PTRACE_TYPE_ARG3)0, 0);
>    if (errno != 0)
> -    perror_with_name (("ptrace"));
> +    return errno;
>  #else
>    error (_("This system does not support attaching to a process"));
>  #endif
> @@ -241,6 +267,8 @@ inf_ptrace_target::attach (const char *args, int from_tty)
>    set_executing (this, thr->ptid, true);
>  
>    unpusher.release ();
> +
> +  return 0;
>  }
>  
>  #ifdef PT_GET_PROCESS_STATE
> diff --git a/gdb/inf-ptrace.h b/gdb/inf-ptrace.h
> index dd0733736f..ca36ca3af3 100644
> --- a/gdb/inf-ptrace.h
> +++ b/gdb/inf-ptrace.h
> @@ -31,6 +31,8 @@ struct inf_ptrace_target : public inf_child_target
>  
>    void attach (const char *, int) override;
>  
> +  int ptrace_attach (const char *, int);
> +
>    void detach (inferior *inf, int) override;
>  
>    void resume (ptid_t, int, enum gdb_signal) override;
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 81af83c4ac..cc044ee3ac 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -31,6 +31,7 @@
>  #include "nat/linux-ptrace.h"
>  #include "nat/linux-procfs.h"
>  #include "nat/linux-personality.h"
> +#include "nat/fork-inferior.h"
>  #include "linux-fork.h"
>  #include "gdbthread.h"
>  #include "gdbcmd.h"
> @@ -1136,7 +1137,7 @@ attach_proc_task_lwp_callback (ptid_t ptid)
>  	  else
>  	    {
>  	      std::string reason
> -		= linux_ptrace_attach_fail_reason_string (ptid, err);
> +		= linux_ptrace_attach_fail_reason_lwp (ptid, err);
>  
>  	      warning (_("Cannot attach to lwp %d: %s"),
>  		       lwpid, reason.c_str ());
> @@ -1185,20 +1186,15 @@ linux_nat_target::attach (const char *args, int from_tty)
>    /* Make sure we report all signals during attach.  */
>    pass_signals ({});
>  
> -  try
> -    {
> -      inf_ptrace_target::attach (args, from_tty);
> -    }
> -  catch (const gdb_exception_error &ex)
> +  int err = inf_ptrace_target::ptrace_attach (args, from_tty);
> +
> +  if (err != 0)
>      {
>        pid_t pid = parse_pid_to_attach (args);
> -      std::string reason = linux_ptrace_attach_fail_reason (pid);
> +      std::string reason = linux_ptrace_attach_fail_reason (pid, err);
>  
> -      if (!reason.empty ())
> -	throw_error (ex.error, "warning: %s\n%s", reason.c_str (),
> -		     ex.what ());
> -      else
> -	throw_error (ex.error, "%s", ex.what ());
> +      error (_("warning: ptrace: %s\n%s"),
> +	       safe_strerror (err), reason.c_str ());
>      }
>  
>    /* The ptrace base target adds the main thread with (pid,0,0)
> @@ -4582,6 +4578,10 @@ Enables printf debugging output."),
>    sigemptyset (&blocked_mask);
>  
>    lwp_lwpid_htab_create ();
> +
> +  /* Set the proper function to generate a message when ptrace
> +     fails.  */
> +  trace_me_fail_reason = linux_ptrace_me_fail_reason;
>  }
>  \f
>  
> diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
> index 223ff44195..eb4c4625d7 100644
> --- a/gdb/nat/fork-inferior.c
> +++ b/gdb/nat/fork-inferior.c
> @@ -394,9 +394,9 @@ struct traceme_info
>  
>         This function will usually perform the call to whatever trace
>         function needed to start tracing the inferior, but will also
> -       write its errno value to TRACE_ERRNO_PIPE, so that
> +       write its errno value to TRACE_ERRNO_WPIPE, so that
>         fork_inferior_1 can check whether it suceeded.  */
> -    void (*traceme_fun_check) (int trace_errno_pipe);
> +    void (*traceme_fun_check) (int trace_errno_wpipe);

I was surprised to see this renaming going on this patch.  Why not
name the variable like that in the previous patch, where it was
introduced, to begin with?

>    } u;
>  };
>  
> @@ -626,7 +626,7 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
>  
>  pid_t
>  fork_inferior (const char *exec_file_arg, const std::string &allargs,
> -	       char **env, void (*traceme_fun) (int trace_errno_pipe),
> +	       char **env, void (*traceme_fun) (int trace_errno_wpipe),

Ditto.  Etc.

>  	       gdb::function_view<void (int)> init_trace_fun,
>  	       void (*pre_trace_fun) (),
>  	       const char *shell_file_arg,
> diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h
> index b67215353f..3fbead2e33 100644
> --- a/gdb/nat/fork-inferior.h
> +++ b/gdb/nat/fork-inferior.h
> @@ -66,7 +66,7 @@ extern pid_t fork_inferior (const char *exec_file_arg,
>  extern pid_t fork_inferior (const char *exec_file_arg,
>  			    const std::string &allargs,
>  			    char **env,
> -			    void (*traceme_fun) (int trace_errno_pipe),
> +			    void (*traceme_fun) (int trace_errno_wpipe),
>  			    gdb::function_view<void (int)> init_trace_fun,
>  			    void (*pre_trace_fun) (),
>  			    const char *shell_file_arg,
> diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
> index 5335d69092..b3fcf8bc07 100644
> --- a/gdb/nat/linux-ptrace.c
> +++ b/gdb/nat/linux-ptrace.c
> @@ -21,6 +21,9 @@
>  #include "linux-procfs.h"
>  #include "linux-waitpid.h"
>  #include "gdbsupport/buffer.h"
> +#include "gdbsupport/gdb-dlfcn.h"
> +#include "nat/fork-inferior.h"
> +#include "gdbsupport/filestuff.h"
>  #ifdef HAVE_SYS_PROCFS_H
>  #include <sys/procfs.h>
>  #endif
> @@ -30,11 +33,93 @@
>     of 0 means there are no supported features.  */
>  static int supported_ptrace_options = -1;
>  
> -/* Find all possible reasons we could fail to attach PID and return these
> -   as a string.  An empty string is returned if we didn't find any reason.  */
> +typedef int (*selinux_ftype) (const char *);
>  
> -std::string
> -linux_ptrace_attach_fail_reason (pid_t pid)
> +/* Helper function which checks if ptrace is probably restricted
> +   (i.e., if ERR is either EACCES or EPERM), and returns a string with
> +   possible workarounds.  */
> +
> +static std::string
> +linux_ptrace_restricted_fail_reason (int err)
> +{
> +  if (err != EACCES && err != EPERM)
> +    {
> +      /* It just makes sense to perform the checks below if errno was
> +	 either EACCES or EPERM.  */
> +      return {};
> +    }
> +
> +  std::string ret;
> +  gdb_dlhandle_up handle;
> +
> +  try
> +    {
> +      handle = gdb_dlopen ("libselinux.so.1");
> +    }
> +  catch (const gdb_exception_error &e)
> +    {
> +    }
> +
> +  if (handle != nullptr)
> +    {
> +      selinux_ftype selinux_get_bool
> +	= (selinux_ftype) gdb_dlsym (handle, "security_get_boolean_active");
> +
> +      if (selinux_get_bool != NULL
> +	  && (*selinux_get_bool) ("deny_ptrace") == 1)
> +	string_appendf (ret,
> +			_("\n\
> +The SELinux 'deny_ptrace' option is enabled and preventing GDB\n\
> +from using 'ptrace'.  You can disable it by executing (as root):\n\
> +\n\
> +  setsebool deny_ptrace off\n"));
> +    }
> +
> +  gdb_file_up yama_ptrace_scope
> +    = gdb_fopen_cloexec ("/proc/sys/kernel/yama/ptrace_scope", "r");
> +
> +  if (yama_ptrace_scope != nullptr)
> +    {
> +      char yama_scope = fgetc (yama_ptrace_scope.get ());
> +
> +      if (yama_scope != '0')
> +	string_appendf (ret,
> +			_("\n\
> +The Linux kernel's Yama ptrace scope is in effect, which can prevent\n\
> +GDB from using 'ptrace'.  You can disable it by executing (as root):\n\
> +\n\
> +  echo 0 > /proc/sys/kernel/yama/ptrace_scope\n"));
> +    }
> +
> +  if (ret.empty ())
> +    {
> +      /* It wasn't possible to determine the exact reason for the
> +	 ptrace error.  Let's just emit a generic error message
> +	 pointing the user to our documentation, where she can find
> +	 instructions on how to try to diagnose the problem.  */
> +      ret = _("\n\
> +There might be restrictions preventing ptrace from working.  Please see\n\
> +the appendix \"Linux kernel ptrace restrictions\" in the GDB documentation\n\
> +for more details.");
> +    }
> +
> +  /* The user may be debugging remotely, so we have to warn that
> +     the instructions above should be performed in the target.  */
> +  string_appendf (ret,
> +		  _("\n\
> +If you are debugging the inferior remotely, the ptrace restriction(s) must\n\
> +be disabled in the target system (e.g., where GDBserver is running)."));
> +
> +  return ret;
> +}
> +
> +/* Find all possible reasons we could fail to attach PID and return
> +   these as a string.  An empty string is returned if we didn't find
> +   any reason.  Helper for linux_ptrace_attach_fail_reason and
> +   linux_ptrace_attach_fail_reason_lwp.  */
> +
> +static std::string
> +linux_ptrace_attach_fail_reason_1 (pid_t pid)
>  {
>    pid_t tracerpid = linux_proc_get_tracerpid_nowarn (pid);
>    std::string result;
> @@ -56,10 +141,24 @@ linux_ptrace_attach_fail_reason (pid_t pid)
>  /* See linux-ptrace.h.  */
>  
>  std::string
> -linux_ptrace_attach_fail_reason_string (ptid_t ptid, int err)
> +linux_ptrace_attach_fail_reason (pid_t pid, int err)
> +{
> +  std::string result = linux_ptrace_attach_fail_reason_1 (pid);
> +  std::string ptrace_restrict = linux_ptrace_restricted_fail_reason (err);
> +
> +  if (!ptrace_restrict.empty ())
> +    result += "\n" + ptrace_restrict;
> +
> +  return result;
> +}
> +
> +/* See linux-ptrace.h.  */
> +
> +std::string
> +linux_ptrace_attach_fail_reason_lwp (ptid_t ptid, int err)
>  {
>    long lwpid = ptid.lwp ();
> -  std::string reason = linux_ptrace_attach_fail_reason (lwpid);
> +  std::string reason = linux_ptrace_attach_fail_reason_1 (lwpid);
>  
>    if (!reason.empty ())
>      return string_printf ("%s (%d), %s", safe_strerror (err), err,
> @@ -68,6 +167,14 @@ linux_ptrace_attach_fail_reason_string (ptid_t ptid, int err)
>      return string_printf ("%s (%d)", safe_strerror (err), err);
>  }
>  
> +/* See linux-ptrace.h.  */
> +
> +std::string
> +linux_ptrace_me_fail_reason (int err)
> +{
> +  return linux_ptrace_restricted_fail_reason (err);
> +}
> +
>  #if defined __i386__ || defined __x86_64__
>  
>  /* Address of the 'ret' instruction in asm code block below.  */
> @@ -257,6 +364,12 @@ linux_ptrace_test_ret_to_nx (void)
>  #endif /* defined __i386__ || defined __x86_64__ */
>  }
>  
> +/* If the PTRACE_TRACEME call on linux_child_function errors, we need
> +   to be able to send ERRNO back to the parent so that it can check
> +   whether there are restrictions in place preventing ptrace from
> +   working.  We do that with a pipe.  */
> +static int errno_pipe[2];
> +

We're missing a comment somewhere saying that we do that on the parent side
to avoid doing non-async-signal-safe things in the child.  Here might be
a good place, but on the gdb side we could use some comment about it too.


>  /* Helper function to fork a process and make the child process call
>     the function FUNCTION, passing CHILD_STACK as parameter.
>  
> @@ -321,7 +434,30 @@ linux_grandchild_function (void *child_stack)
>  static int
>  linux_child_function (void *child_stack)
>  {
> -  ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0, (PTRACE_TYPE_ARG4) 0);
> +  /* Close read end.  */
> +  close (errno_pipe[0]);
> +
> +  int ret = ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0,
> +		    (PTRACE_TYPE_ARG4) 0);
> +  int ptrace_errno = ret < 0 ? errno : 0;
> +
> +  /* Write ERRNO to the pipe, even if it's zero, and close the writing
> +     end of the pipe.  */
> +  try
> +    {
> +      write_trace_errno_to_pipe (errno_pipe[1], ptrace_errno);
> +    }
> +  catch (const gdb_exception &e)
> +    {
> +      warning ("%s", e.what ());
> +      _exit (0177);
> +    }

Here's another spot that should do away with try/catch.

> +
> +  close (errno_pipe[1]);
> +
> +  if (ret != 0)
> +    trace_start_error_with_name ("ptrace");
> +
>    kill (getpid (), SIGSTOP);
>  
>    /* Fork a grandchild.  */
> @@ -346,12 +482,40 @@ linux_check_ptrace_features (void)
>    /* Initialize the options.  */
>    supported_ptrace_options = 0;
>  
> +  /* Initialize our pipe.  */
> +  if (gdb_pipe_cloexec (errno_pipe) < 0)
> +    perror_with_name ("gdb_pipe_cloexec");
> +
>    /* Fork a child so we can do some testing.  The child will call
>       linux_child_function and will get traced.  The child will
>       eventually fork a grandchild so we can test fork event
>       reporting.  */
>    child_pid = linux_fork_to_function (NULL, linux_child_function);
>  
> +  /* We don't need the write end of the pipe anymore.  */
> +  close (errno_pipe[1]);
> +
> +  try
> +    {
> +      /* Check whether 'ptrace (PTRACE_ME, ...)' failed when being
> +	 invoked by the child.  If it did, we might get the
> +	 possible reason for it as the exception message.  */
> +      check_child_trace_me_errno (errno_pipe[0]);

This is assuming that close doesn't clober errno, which in general
is not garanteed:

  https://pubs.opengroup.org/onlinepubs/9699919799/functions/errno.html#tag_16_110

"The setting of errno after a successful call to a function is unspecified 
unless the description of that function specifies that errno shall not
be modified."

A quick web search finds this for example:

 https://git.furworks.de/opensourcemirror/git/commit/06121a0a8328c8aaa7a023cf6ebb142e9dc2b45c

> +    }
> +  catch (const gdb_exception &e)
> +    {
> +      /* Close the pipe so we don't leak fd's.  */

fd's -> fds

> +      close (errno_pipe[0]);
> +
> +      /* A failure here means that PTRACE_ME failed, which means that
> +	 GDB/gdbserver will most probably not work correctly.  If we
> +	 want to be pedantic, we could just call 'exit' here.
> +	 However, let's just re-throw the exception.  */
> +      throw;
> +    }
> +
> +  close (errno_pipe[0]);
> +
>    ret = my_waitpid (child_pid, &status, 0);
>    if (ret == -1)
>      perror_with_name (("waitpid"));
> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
> index 65568301f2..7cb77114ca 100644
> --- a/gdb/nat/linux-ptrace.h
> +++ b/gdb/nat/linux-ptrace.h
> @@ -176,12 +176,27 @@ struct buffer;
>  # define TRAP_HWBKPT 4
>  #endif
>  
> -extern std::string linux_ptrace_attach_fail_reason (pid_t pid);
> -
> -/* Find all possible reasons we could have failed to attach to PTID
> -   and return them as a string.  ERR is the error PTRACE_ATTACH failed
> -   with (an errno).  */
> -extern std::string linux_ptrace_attach_fail_reason_string (ptid_t ptid, int err);
> +/* Find all possible reasons we could fail to attach PID and return
> +   these as a string.  An empty string is returned if we didn't find
> +   any reason.  If ERR is EACCES or EPERM, we also add a warning about
> +   possible restrictions to use ptrace.  */
> +extern std::string linux_ptrace_attach_fail_reason (pid_t pid, int err);
> +
> +/* Find all possible reasons we could have failed to attach to PID's
> +   LWPID and return them as a string.  ERR is the error PTRACE_ATTACH
> +   failed with (an errno).  Unlike linux_ptrace_attach_fail_reason,
> +   this function should be used when attaching to an LWP other than
> +   the leader; it does not warn about ptrace restrictions.  */
> +extern std::string linux_ptrace_attach_fail_reason_lwp (ptid_t pid, int err);
> +
> +/* When the call to 'ptrace (PTRACE_TRACEME...' fails, and we have
> +   already forked, this function can be called in order to try to
> +   obtain the reason why ptrace failed.  ERR should be the ERRNO value
> +   returned by ptrace.
> +
> +   This function will return a 'std::string' containing the fail
> +   reason, or an empty string otherwise.  */
> +extern std::string linux_ptrace_me_fail_reason (int err);
>  
>  extern void linux_ptrace_init_warnings (void);
>  extern void linux_check_ptrace_features (void);
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 0f78b1be1b..aacbdf1984 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -5882,9 +5882,26 @@ extended_remote_target::attach (const char *args, int from_tty)
>        break;
>      case PACKET_UNKNOWN:
>        error (_("This target does not support attaching to a process"));
> +    case PACKET_ERROR:
> +      {
> +	std::string errmsg = rs->buf.data ();

This is taking a deep copy which seems unnecessary.

The code would look almost the same without it.  Like:

        const char *errmsg = rs->buf.data ();

	/* Check if we have a specific error (i.e., not a generic
	   "E01") coming from the target.  If there is, we print it
	   here.  */
	if (startswith (errmsg, "E."))
	  {
	    /* Get rid of the "E." prefix.  */
	    errmsg += 2;
	  }

	error (_("Attaching to %s failed%s%s"),
	       target_pid_to_str (ptid_t (pid)).c_str (),
	       errmsg != '\0' ? "\n" : "",
	       errmsg);


> +
> +	/* Check if we have a specific error (i.e., not a generic
> +	   "E01") coming from the target.  If there is, we print it
> +	   here.  */
> +	if (startswith (errmsg.c_str (), "E."))
> +	  {
> +	    /* Get rid of the "E." prefix.  */
> +	    errmsg.erase (0, 2);
> +	  }
> +
> +	error (_("Attaching to %s failed%s%s"),
> +	       target_pid_to_str (ptid_t (pid)).c_str (),
> +	       !errmsg.empty () ? "\n" : "",
> +	       errmsg.c_str ());
> +      }
>      default:
> -      error (_("Attaching to %s failed"),
> -	     target_pid_to_str (ptid_t (pid)).c_str ());
> +      gdb_assert_not_reached (_("bad switch"));
>      }
>  
>    set_current_inferior (remote_add_inferior (false, pid, 1, 0));
> @@ -10024,8 +10041,23 @@ remote_target::extended_remote_run (const std::string &args)
>  	error (_("Running the default executable on the remote target failed; "
>  		 "try \"set remote exec-file\"?"));
>        else
> -	error (_("Running \"%s\" on the remote target failed"),
> -	       remote_exec_file);
> +	{
> +	  std::string errmsg = rs->buf.data ();
> +
> +	  /* Check if we have a specific error (i.e., not a generic
> +	     "E01") coming from the target.  If there is, we print it
> +	     here.  */
> +	  if (startswith (errmsg.c_str (), "E."))
> +	    {
> +	      /* Get rid of the "E." prefix.  */
> +	      errmsg.erase (0, 2);
> +	    }
> +
> +	  error (_("Running \"%s\" on the remote target failed%s%s"),
> +		 remote_exec_file,
> +		 !errmsg.empty () ? "\n" : "",
> +		 errmsg.c_str ());

Ditto.

> +	}
>      default:
>        gdb_assert_not_reached (_("bad switch"));
>      }
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index 2872bc78da..42283802dd 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -968,10 +968,24 @@ add_lwp (ptid_t ptid)
>     actually initiating the tracing of the inferior.  */
>  
>  static void
> -linux_ptrace_fun ()
> +linux_ptrace_fun (int ptrace_errno_wpipe)
>  {
> -  if (ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0,
> -	      (PTRACE_TYPE_ARG4) 0) < 0)
> +  int ret = ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0,
> +		    (PTRACE_TYPE_ARG4) 0);
> +  int ptrace_errno = ret < 0 ? errno : 0;
> +
> +  try
> +    {
> +      write_trace_errno_to_pipe (ptrace_errno_wpipe, ptrace_errno);
> +    }
> +  catch (const gdb_exception &e)
> +    {
> +      warning ("%s", e.what ());
> +      _exit (0177);
> +    }

Another spot that could do without try/catch.

> +
> +  errno = ptrace_errno;
> +  if (ret < 0)
>      trace_start_error_with_name ("ptrace");
>  
>    if (setpgid (0, 0) < 0)
> @@ -1170,7 +1184,7 @@ attach_proc_task_lwp_callback (ptid_t ptid)
>        else if (err != 0)
>  	{
>  	  std::string reason
> -	    = linux_ptrace_attach_fail_reason_string (ptid, err);
> +	    = linux_ptrace_attach_fail_reason_lwp (ptid, err);
>  
>  	  warning (_("Cannot attach to lwp %d: %s"), lwpid, reason.c_str ());
>  	}
> @@ -1202,8 +1216,8 @@ linux_process_target::attach (unsigned long pid)
>      {
>        remove_process (proc);
>  
> -      std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err);
> -      error ("Cannot attach to process %ld: %s", pid, reason.c_str ());
> +      std::string reason = linux_ptrace_attach_fail_reason (pid, err);
> +      error (_("Cannot attach to process %ld: %s"), pid, reason.c_str ());
>      }
>  
>    /* Don't ignore the initial SIGSTOP if we just attached to this
> @@ -7552,5 +7566,10 @@ initialize_low (void)
>  
>    initialize_low_arch ();
>  
> +  /* Initialize the 'trace_me_fail_reason' function pointer.  We will
> +     use this to determine the reason for possible failures when
> +     invoking 'ptrace (PTRACE_ME, ...)'.  */
> +  trace_me_fail_reason = linux_ptrace_me_fail_reason;
> +
>    linux_check_ptrace_features ();
>  }
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index 43962adc86..003385f42a 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -2892,9 +2892,31 @@ handle_v_attach (char *own_buf)
>  {
>    client_state &cs = get_client_state ();
>    int pid;
> +  int ret;
>  
>    pid = strtol (own_buf + 8, NULL, 16);
> -  if (pid != 0 && attach_inferior (pid) == 0)
> +
> +  if (pid <= 0)
> +    {
> +      write_enn (own_buf);
> +      return 0;
> +    }
> +
> +  try
> +    {
> +      /* Attach to the specified PID.  This function can throw, so we
> +	 make sure to catch the exception and send it (as an error
> +	 packet) back to GDB.  */
> +      ret = attach_inferior (pid);
> +    }
> +  catch (const gdb_exception_error &e)
> +    {
> +      fprintf (stderr, "%s\n", e.what ());
> +      snprintf (own_buf, PBUFSIZ, "E.%s", e.what ());
> +      return 0;
> +    }
> +
> +  if (ret == 0)
>      {
>        /* Don't report shared library events after attaching, even if
>  	 some libraries are preloaded.  GDB will always poll the
> @@ -3030,7 +3052,19 @@ handle_v_run (char *own_buf)
>    free_vector_argv (program_args);
>    program_args = new_argv;
>  
> -  target_create_inferior (program_path.get (), program_args);
> +  try
> +    {
> +      /* Create the inferior.  This function can throw, so we make
> +	 sure to catch the exception and send it (as an error packet)
> +	 back to GDB.  */
> +      target_create_inferior (program_path.get (), program_args);
> +    }
> +  catch (const gdb_exception_error &e)
> +    {
> +      fprintf (stderr, "%s\n", e.what ());
> +      snprintf (own_buf, PBUFSIZ, "E.%s", e.what ());
> +      return 0;
> +    }
>  
>    if (cs.last_status.kind == TARGET_WAITKIND_STOPPED)
>      {
> diff --git a/gdbserver/thread-db.cc b/gdbserver/thread-db.cc
> index 2bb6d28820..60ceb7b663 100644
> --- a/gdbserver/thread-db.cc
> +++ b/gdbserver/thread-db.cc
> @@ -224,7 +224,7 @@ attach_thread (const td_thrhandle_t *th_p, td_thrinfo_t *ti_p)
>    err = linux_attach_lwp (ptid);
>    if (err != 0)
>      {
> -      std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err);
> +      std::string reason = linux_ptrace_attach_fail_reason_lwp (ptid, err);
>  
>        warning ("Could not attach to thread %ld (LWP %d): %s",
>  	       (unsigned long) ti_p->ti_tid, ti_p->ti_lid, reason.c_str ());
> 


-- 
Thanks,
Pedro Alves