From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 105388 invoked by alias); 29 Aug 2019 19:27:27 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 105378 invoked by uid 89); 29 Aug 2019 19:27:27 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=compose, existed X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 29 Aug 2019 19:27:22 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0CCB231752AE; Thu, 29 Aug 2019 19:27:21 +0000 (UTC) Received: from localhost (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id AC7AA5D713; Thu, 29 Aug 2019 19:27:20 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: GDB Patches , Eli Zaretskii , Ruslan Kabatsayev Subject: Re: [PATCH v2] Improve ptrace-error detection on Linux targets References: <20190819032918.3536-1-sergiodj@redhat.com> <20190826183205.19008-1-sergiodj@redhat.com> <28c4f743-91f1-59c3-83ff-3f791811f996@redhat.com> Date: Thu, 29 Aug 2019 19:27:00 -0000 In-Reply-To: <28c4f743-91f1-59c3-83ff-3f791811f996@redhat.com> (Pedro Alves's message of "Thu, 29 Aug 2019 15:40:17 +0100") Message-ID: <87mufrai1z.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2019-08/txt/msg00650.txt.bz2 On Thursday, August 29 2019, Pedro Alves wrote: > Hi Sergio, > > This is looking quite good to me. Some comments below. Thanks for the review, Pedro. > On 8/26/19 7:32 PM, Sergio Durigan Junior wrote: >> Changes from v1: >> >> - Addressed Pedro's comments re. internal organization and gdbserver >> support. >> >> - Addressed Eli's comments (doc fixes). >> >> - New ways of detecting what's wrong with ptrace. >> >> >> >> In Fedora GDB, we carry the following patch: >> >> https://src.fedoraproject.org/rpms/gdb/blob/master/f/gdb-attach-fail-reasons-5of5.patch > > This link will soon be dead, right? (thinking about the commit log) > Maybe point at some reference other than master. Yeah. I will provide a link to a commit. >> 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_ME 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. >> >> My first thought (and attempt) was to make GDB print a generic warning >> when a ptrace error happened; this message would just point the user >> to our documentation, where she could find more information about >> possible causes for the error (and try to diagnose/fix the problem). >> This proved to be too simple, and I was convinced that it is actually >> a good idea to go the extra kilometre and try to pinpoint the specific >> problem (or problems) preventing ptrace from working, as well as >> provide useful suggestions on how the user can fix things. >> >> Here is the patch I came up with. It implements 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" and checks if the "deny_ptrace" option >> is enabled. > > Update reference to "libselinux.so.1" here too. Done. >> >> - 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 /bin/true >> ... >> Starting program: /usr/bin/true >> warning: Could not trace the inferior process. >> Error: >> warning: ptrace: Permission denied > > Curious, that: > > warning: > Error: > warning: > > looks a bit odd, specifically the "Error:" line. Do you know where is > that coming from? Not offhand; I'll investigate and come back with the results. >> The SELinux 'deny_ptrace' option is enabled and preventing GDB >> from using 'ptrace'. Please, disable it by executing (as root): > > I'm really not a fan of these "Please, ". Kind of sounds like > gdb is begging, to me. I'd rather use an informational tone, like: > > The SELinux 'deny_ptrace' option is enabled and preventing GDB > from using 'ptrace'. You can disable it by executing (as root): Fair enough. Changed as requested. >> >> setsebool deny_ptrace off >> >> During startup program exited with code 127. >> >> In case "/proc/sys/kernel/yama/ptrace_scope" is > 0: >> >> # gdb /bin/true >> ... >> Starting program: /usr/bin/true >> warning: Could not trace the inferior process. >> Error: >> warning: ptrace: Operation not permitted >> The Linux kernel's Yama ptrace scope is in effect, which can prevent >> GDB from using 'ptrace'. Please, disable it by executing (as root): > > Ditto. Changed. >> echo 0 > /proc/sys/kernel/yama/ptrace_scope >> >> During startup program exited with code 127. >> >> 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 /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 /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 >> gdbserver: Could not trace the inferior process. >> Error: >> gdbserver: ptrace: Operation not permitted >> The Linux kernel's Yama ptrace scope is in effect, which can prevent >> GDB from using 'ptrace'. Please, disable it by executing (as root): >> >> echo 0 > /proc/sys/kernel/yama/ptrace_scope >> >> linux_check_ptrace_features: waitpid: unexpected status 32512. >> Exiting >> >> (I decided to keep all the other messages, even though I find them a >> bit distracting). > > Yeah, the other messages are implementor-speak, showing gdb function > names. We should ideally clean this all up. Agreed. I can propose a patch later to clean them. >> >> 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. >> >> This means that the patch expands our documentation and creates a new >> appendix section named "Linux kernel ptrace restrictions", with >> sub-sections for each possible restriction that might be in place. >> >> 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. >> >> I tested this patch locally, on my Fedora 30 machine (actually, a >> Fedora Rawhide VM), but I'm not proposing a testcase for it because of >> the difficulty of writing one. >> >> WDYT? >> >> gdb/doc/ChangeLog: >> yyyy-mm-dd Sergio Durigan Junior >> >> * gdb.texinfo (Linux kernel ptrace restrictions): New appendix >> section. >> >> gdb/ChangeLog: >> yyyy-mm-dd Sergio Durigan Junior >> Jan Kratochvil >> >> * gdbsupport/gdb-dlfcn.c (gdb_dlopen): Add argument 'dont_throw' >> Don't throw if it's true. >> * gdbsupport/gdb-dlfcn.h (gdb_dlopen): Add optional argument >> 'dont_throw'. Update comment. >> * inf-ptrace.c (default_inf_ptrace_me_fail_reason): New >> function. >> (inf_ptrace_me_fail_reason): New variable. >> (inf_ptrace_me): Update call to 'trace_start_error_with_name'. >> * inf-ptrace.h (inf_ptrace_me_fail_reason): New variable. >> * linux-nat.c (linux_nat_target::attach): Update call to >> 'linux_ptrace_attach_fail_reason'. >> (_initialize_linux_nat): Set 'inf_ptrace_me_fail_reason'. >> * linux-nat.h (linux_nat_target) : New >> method. >> * nat/fork-inferior.c (trace_start_error_with_name): Add >> optional 'append' argument. >> * nat/fork-inferior.h (trace_start_error_with_name): Update >> prototype. >> * nat/linux-ptrace.c: Include "gdbsupport/gdb-dlfcn.h" and >> "nat/fork-inferior.h". >> (selinux_ftype): New typedef. >> (linux_ptrace_restricted_fail_reason): New function. >> (linux_ptrace_attach_fail_reason): Add optional 'err' >> argument. Call 'linux_ptrace_restricted_fail_reason'. >> (linux_ptrace_me_fail_reason): New function. >> (linux_child_function): Handle ptrace error. >> * nat/linux-ptrace.h (linux_ptrace_attach_fail_reason): Update >> prototype. >> (linux_ptrace_me_fail_reason): New function. >> * target-delegates.c: Regenerate. >> * target.h (struct target_ops) : New >> method. >> (target_ptrace_me_fail_reason): New define. >> >> gdb/gdbserver/ChangeLog: >> yyyy-mm-dd Sergio Durigan Junior >> >> * linux-low.c (linux_ptrace_fun): Call >> 'linux_ptrace_me_fail_reason'. >> --- >> gdb/doc/gdb.texinfo | 138 +++++++++++++++++++++++++++++++++++++ >> gdb/gdbserver/linux-low.c | 3 +- >> gdb/gdbsupport/gdb-dlfcn.c | 4 +- >> gdb/gdbsupport/gdb-dlfcn.h | 7 +- >> gdb/inf-ptrace.c | 19 ++++- >> gdb/inf-ptrace.h | 10 +++ >> gdb/linux-nat.c | 7 +- >> gdb/nat/fork-inferior.c | 4 +- >> gdb/nat/fork-inferior.h | 7 +- >> gdb/nat/linux-ptrace.c | 86 +++++++++++++++++++++-- >> gdb/nat/linux-ptrace.h | 15 +++- >> 11 files changed, 282 insertions(+), 18 deletions(-) >> >> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo >> index e1bc8143e6..43f749c6f1 100644 >> --- a/gdb/doc/gdb.texinfo >> +++ b/gdb/doc/gdb.texinfo >> @@ -182,6 +182,9 @@ software in general. We will miss him. >> @value{GDBN} >> * Operating System Information:: Getting additional information from >> the operating system >> +* Linux kernel @code{ptrace} restrictions:: Restrictions sometimes >> + imposed by the Linux >> + kernel on @code{ptrace} >> * Trace File Format:: GDB trace file format >> * Index Section Format:: .gdb_index section format >> * Man Pages:: Manual pages >> @@ -44682,6 +44685,141 @@ should contain a comma-separated list of cores that this process >> is running on. Target may provide additional columns, >> which @value{GDBN} currently ignores. >> >> +@node Linux kernel ptrace restrictions >> +@appendix Linux kernel @code{ptrace} restrictions >> +@cindex linux kernel ptrace restrictions, attach >> + >> +The @code{ptrace} system call is used by @value{GDBN} on GNU/Linux to, >> +among other things, attach to a new or existing inferior in order to >> +start debugging it. Due to security concerns, some distributions and >> +vendors disable or severily restrict the ability to perform these > > typo: severily -> severely Fixed. > > >> +operations, which can make @value{GDBN} malfunction. In this section, >> +we will expand on how this malfunction can manifest itself, and how to >> +modify the system's settings in order to be able to use @value{GDBN} >> +properly. >> + >> +@menu >> +* The GDB error message:: The error message displayed when the >> + system prevents @value{GDBN} from using >> + @code{ptrace} >> +* SELinux's @code{deny_ptrace}:: SELinux and the @code{deny_ptrace} option >> +* Yama's @code{ptrace_scope}:: Yama and the @code{ptrace_scope} setting >> +* Docker and @code{seccomp}:: Docker and the @code{seccomp} >> + infrastructure >> +@end menu >> + >> +@node The GDB error message >> +@appendixsection The @value{GDBN} error message >> + >> +When the system prevents @value{GDBN} from using the @code{ptrace} >> +system call, you will likely see a descriptive error message >> +explaining what is wrong and how to attempt to fix the problem. For >> +example, when SELinux's @code{deny_ptrace} option is enabled, you can >> +see: >> + >> +@smallexample >> +$ gdb program >> +... >> +(@value{GDBP}) run >> +Starting program: program >> +warning: Could not trace the inferior process. >> +Error: >> +warning: ptrace: Permission denied >> +The SELinux 'deny_ptrace' option is enabled and preventing GDB >> +from using 'ptrace'. Please, disable it by executing (as root): >> + >> + setsebool deny_ptrace off >> + >> +During startup program exited with code 127. >> +(@value{GDBP}) >> +@end smallexample >> + >> +Sometimes, it may not be possible to acquire the necessary data to >> +determine the root cause of the failure. In this case, you will see a >> +generic error message pointing you to this section: >> + >> +@smallexample >> +$ gdb program >> +... >> +Starting program: program >> +warning: Could not trace the inferior process. >> +Error: >> +warning: ptrace: Permission denied >> +There might be restrictions preventing ptrace from working. Please see >> +the appendix "Linux kernel ptrace restrictions" in the GDB documentation >> +for more details. >> +During startup program exited with code 127. >> +(@value{GDBP}) >> +@end smallexample >> + >> +@node SELinux's deny_ptrace >> +@appendixsection SELinux's @code{deny_ptrace} >> +@cindex SELinux >> +@cindex deny_ptrace >> + >> +If you are using SELinux, you might want to check whether the >> +@code{deny_ptrace} option is enabled by doing: >> + >> +@smallexample >> +$ getsebool deny_ptrace >> +deny_ptrace --> on >> +@end smallexample >> + >> +If the option is enabled, you can disable it by doing, as root: >> + >> +@smallexample >> +# setsebool deny_ptrace off >> +@end smallexample >> + >> +The option will be disabled until the next reboot. If you would like >> +to disable it permanently, you can do (as root): >> + >> +@smallexample >> +# setsebool -P deny_ptrace off >> +@end smallexample >> + >> +@node Yama's ptrace_scope >> +@appendixsection Yama's @code{ptrace_scope} >> +@cindex yama, ptrace_scope >> + >> +If your system has Yama enabled, you might want to check whether the >> +@code{ptrace_scope} setting is enabled by checking the value of >> +@file{/proc/sys/kernel/yama/ptrace_scope}: >> + >> +@smallexample >> +$ cat /proc/sys/kernel/yama/ptrace_scope >> +0 >> +@end smallexample >> + >> +If you see anything other than @code{0}, @value{GDBN} can be affected >> +by it. You can temporarily disable the feature by doing, as root: >> + >> +@smallexample >> +# sysctl kernel.yama.ptrace_scope=0 >> +kernel.yama.ptrace_scope = 0 >> +@end smallexample >> + >> +You can make this permanent by doing, as root: >> + >> +@smallexample >> +# sysctl -w kernel.yama.ptrace_scope=0 >> +kernel.yama.ptrace_scope = 0 >> +@end smallexample >> + >> +@node Docker and seccomp >> +@appendixsection Docker and @code{seccomp} >> +@cindex docker, seccomp >> + >> +If you are using Docker (@uref{https://www.docker.com/}) containers, >> +you will probably have to disable its @code{seccomp} protections in >> +order to be able to use @value{GDBN}. To do that, you can use the >> +options @code{--cap-add=SYS_PTRACE --security-opt seccomp=unconfined} >> +when invoking Docker: >> + >> +@smallexample >> +$ docker run --cap-add=SYS_PTRACE --security-opt seccomp=unconfined >> +@end smallexample >> + >> @node Trace File Format >> @appendix Trace File Format >> @cindex trace file format >> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c >> index 3113017ae6..1e0a5cbf54 100644 >> --- a/gdb/gdbserver/linux-low.c >> +++ b/gdb/gdbserver/linux-low.c >> @@ -971,7 +971,8 @@ linux_ptrace_fun () >> { >> if (ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0, >> (PTRACE_TYPE_ARG4) 0) < 0) >> - trace_start_error_with_name ("ptrace"); >> + trace_start_error_with_name ("ptrace", >> + linux_ptrace_me_fail_reason (errno).c_str ()); >> >> if (setpgid (0, 0) < 0) >> trace_start_error_with_name ("setpgid"); >> diff --git a/gdb/gdbsupport/gdb-dlfcn.c b/gdb/gdbsupport/gdb-dlfcn.c >> index 921f10f3d8..9e5a992c17 100644 >> --- a/gdb/gdbsupport/gdb-dlfcn.c >> +++ b/gdb/gdbsupport/gdb-dlfcn.c >> @@ -58,7 +58,7 @@ is_dl_available (void) >> #else /* NO_SHARED_LIB */ >> >> gdb_dlhandle_up >> -gdb_dlopen (const char *filename) >> +gdb_dlopen (const char *filename, bool dont_throw) >> { >> void *result; >> #ifdef HAVE_DLFCN_H >> @@ -66,7 +66,7 @@ gdb_dlopen (const char *filename) >> #elif __MINGW32__ >> result = (void *) LoadLibrary (filename); >> #endif >> - if (result != NULL) >> + if (dont_throw || result != NULL) >> return gdb_dlhandle_up (result); >> >> #ifdef HAVE_DLFCN_H >> diff --git a/gdb/gdbsupport/gdb-dlfcn.h b/gdb/gdbsupport/gdb-dlfcn.h >> index 6a39d38941..a8ddbc03da 100644 >> --- a/gdb/gdbsupport/gdb-dlfcn.h >> +++ b/gdb/gdbsupport/gdb-dlfcn.h >> @@ -32,10 +32,11 @@ struct dlclose_deleter >> typedef std::unique_ptr gdb_dlhandle_up; >> >> /* Load the dynamic library file named FILENAME, and return a handle >> - for that dynamic library. Return NULL if the loading fails for any >> - reason. */ >> + for that dynamic library. If the loading fails, return NULL if >> + DONT_THROW is true, or throw an exception otherwise (default >> + behaviour). */ >> >> -gdb_dlhandle_up gdb_dlopen (const char *filename); >> +gdb_dlhandle_up gdb_dlopen (const char *filename, bool dont_throw = false); >> >> /* Return the address of the symbol named SYMBOL inside the shared >> library whose handle is HANDLE. Return NULL when the symbol could >> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c >> index 4a8e732373..b6cfd803cf 100644 >> --- a/gdb/inf-ptrace.c >> +++ b/gdb/inf-ptrace.c >> @@ -94,6 +94,22 @@ inf_ptrace_target::remove_fork_catchpoint (int pid) >> #endif /* PT_GET_PROCESS_STATE */ >> >> >> +/* Default method for "inf_ptrace_me_fail_reason", which returns an >> + empty string. */ >> + >> +static std::string >> +default_inf_ptrace_me_fail_reason (int err) >> +{ >> + return {}; >> +} >> + >> +/* Point to "inf_ptrace_me_fail_reason", > > Did you mean "Pointer"? This reads strange because > "inf_ptrace_me_fail_reason" is the name of the pointer itself, > so how to point _to_ it? Yes, I meant "Pointer", thanks. Fixed. > which implements a function >> + that can be called by "inf_ptrace_me" in order to obtain the reason >> + for failure. */ >> + >> +std::string (*inf_ptrace_me_fail_reason) (int err) >> + = default_inf_ptrace_me_fail_reason; >> + >> /* Prepare to be traced. */ >> >> static void >> @@ -101,7 +117,8 @@ inf_ptrace_me (void) >> { >> /* "Trace me, Dr. Memory!" */ >> if (ptrace (PT_TRACE_ME, 0, (PTRACE_TYPE_ARG3) 0, 0) < 0) >> - trace_start_error_with_name ("ptrace"); >> + trace_start_error_with_name ("ptrace", >> + inf_ptrace_me_fail_reason (errno).c_str ()); >> } >> >> /* Start a new inferior Unix child process. EXEC_FILE is the file to >> diff --git a/gdb/inf-ptrace.h b/gdb/inf-ptrace.h >> index 98b5d2e09e..7cdab9af89 100644 >> --- a/gdb/inf-ptrace.h >> +++ b/gdb/inf-ptrace.h >> @@ -83,4 +83,14 @@ protected: >> >> extern pid_t get_ptrace_pid (ptid_t); >> >> +/* Pointer to "inf_ptrace_me_fail_reason", which implements a function > > Ah, here it's "pointer". But still reads strange. I think the comment > in the .c file should just say the usual "See inf-ptrace.h.". Indeed. I'll update the comment there. > >> + that can be called by "inf_ptrace_me" in order to obtain the reason >> + for a ptrace failure. ERR is the ERRNO value set by the failing >> + ptrace call. >> + >> + This pointer can be overriden by targets that want to personalize >> + the error message printed when ptrace fails (see linux-nat.c, for >> + example). */ >> +extern std::string (*inf_ptrace_me_fail_reason) (int err); >> + >> #endif >> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c >> index 945c19f666..b5a9eaf72e 100644 >> --- a/gdb/linux-nat.c >> +++ b/gdb/linux-nat.c >> @@ -1191,8 +1191,9 @@ linux_nat_target::attach (const char *args, int from_tty) >> } >> catch (const gdb_exception_error &ex) >> { >> + int saved_errno = errno; >> 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, saved_errno); >> >> if (!reason.empty ()) >> throw_error (ex.error, "warning: %s\n%s", reason.c_str (), >> @@ -4696,6 +4697,10 @@ Enables printf debugging output."), >> sigemptyset (&blocked_mask); >> >> lwp_lwpid_htab_create (); >> + >> + /* Set the proper function to generate a message when ptrace >> + fails. */ >> + inf_ptrace_me_fail_reason = linux_ptrace_me_fail_reason; >> } >> >> >> diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c >> index 68b51aa814..72ac623e20 100644 >> --- a/gdb/nat/fork-inferior.c >> +++ b/gdb/nat/fork-inferior.c >> @@ -591,7 +591,7 @@ trace_start_error (const char *fmt, ...) >> /* See nat/fork-inferior.h. */ >> >> void >> -trace_start_error_with_name (const char *string) >> +trace_start_error_with_name (const char *string, const char *append) >> { >> - trace_start_error ("%s: %s", string, safe_strerror (errno)); >> + trace_start_error ("%s: %s%s", string, safe_strerror (errno), append); >> } >> diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h >> index 1d0519fb26..7e6b889210 100644 >> --- a/gdb/nat/fork-inferior.h >> +++ b/gdb/nat/fork-inferior.h >> @@ -98,9 +98,10 @@ extern void trace_start_error (const char *fmt, ...) >> ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2); >> >> /* Like "trace_start_error", but the error message is constructed by >> - combining STRING with the system error message for errno. This >> - function does not return. */ >> -extern void trace_start_error_with_name (const char *string) >> + combining STRING with the system error message for errno, and >> + (optionally) with APPEND. This function does not return. */ >> +extern void trace_start_error_with_name (const char *string, >> + const char *append = "") >> ATTRIBUTE_NORETURN; >> >> #endif /* NAT_FORK_INFERIOR_H */ >> diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c >> index c1ebc0a032..599d9cfb55 100644 >> --- a/gdb/nat/linux-ptrace.c >> +++ b/gdb/nat/linux-ptrace.c >> @@ -21,6 +21,8 @@ >> #include "linux-procfs.h" >> #include "linux-waitpid.h" >> #include "gdbsupport/buffer.h" >> +#include "gdbsupport/gdb-dlfcn.h" >> +#include "nat/fork-inferior.h" >> #ifdef HAVE_SYS_PROCFS_H >> #include >> #endif >> @@ -30,11 +32,70 @@ >> 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 *); >> + >> +/* 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 = gdb_dlopen ("libselinux.so", true); >> + >> + if (handle.get () != NULL) > > No need for .get() here. This: > > if (handle != NULL) > > works the same. While writing this code I thought I remembered something like that, but I wasn't sure. Thanks for clarifying. Updated. > (I'd write nullptr throughout instead of NULL in new code.) OK. Updated. >> + { >> + 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'. Please, disable it by executing (as root):\n\ >> +\n\ >> + setsebool deny_ptrace off\n")); >> + } >> + >> + FILE *f = fopen ("/proc/sys/kernel/yama/ptrace_scope", "r"); >> + > > Use gdb_fopen_cloexec ? Ah, I hadn't realized that this existed. Much better, thanks! >> + if (f != NULL) >> + { >> + char yama_scope = fgetc (f); >> + >> + fclose (f); >> + >> + 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'. Please, disable it by executing (as root):\n\ >> +\n\ >> + echo 0 > /proc/sys/kernel/yama/ptrace_scope\n")); >> + } >> + >> + if (ret.empty ()) >> + 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.\n"); >> + >> + return ret; >> +} >> + >> +/* See declaration in linux-ptrace.h. */ >> >> std::string >> -linux_ptrace_attach_fail_reason (pid_t pid) >> +linux_ptrace_attach_fail_reason (pid_t pid, int err) >> { >> pid_t tracerpid = linux_proc_get_tracerpid_nowarn (pid); >> std::string result; >> @@ -50,6 +111,11 @@ linux_ptrace_attach_fail_reason (pid_t pid) >> "terminated"), >> (int) pid); >> >> + std::string ptrace_restrict = linux_ptrace_restricted_fail_reason (err); >> + >> + if (!ptrace_restrict.empty ()) >> + result += "\n" + ptrace_restrict; >> + >> return result; >> } >> >> @@ -68,6 +134,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. */ >> @@ -321,7 +395,11 @@ 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); >> + if (ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0, >> + (PTRACE_TYPE_ARG4) 0) != 0) >> + trace_start_error_with_name ("ptrace", >> + linux_ptrace_me_fail_reason (errno).c_str ()); >> + >> kill (getpid (), SIGSTOP); >> >> /* Fork a grandchild. */ >> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h >> index fd2f12a342..04ada53bf6 100644 >> --- a/gdb/nat/linux-ptrace.h >> +++ b/gdb/nat/linux-ptrace.h >> @@ -176,13 +176,26 @@ struct buffer; >> # define TRAP_HWBKPT 4 >> #endif >> >> -extern std::string linux_ptrace_attach_fail_reason (pid_t pid); >> +/* 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 = -1); > > If ERR is an errno number, then it's a bit odd to use -1 for default, > since errno == 0 is the traditional "no error" number. Pedantically, I believe > there's no garantee that a valid error number must be a positive integer. > > But, why the default argument in the first place? What calls this > without passing an error? So, the only place that calls linux_ptrace_attach_fail_reason without passing the ERR argument is linux_ptrace_attach_fail_reason_string: std::string linux_ptrace_attach_fail_reason_string (ptid_t ptid, int err) { long lwpid = ptid.lwp (); std::string reason = linux_ptrace_attach_fail_reason (lwpid); if (!reason.empty ()) return string_printf ("%s (%d), %s", safe_strerror (err), err, reason.c_str ()); else return string_printf ("%s (%d)", safe_strerror (err), err); } In this case, I opted to keep it as is because the function will compose a string contaning like: A (B)[: C] Where: A = safe_strerror B = errno C = fail reason (optional) This function (linux_ptrace_attach_fail_reason_string) is called in three places: gdb/linux-nat.c: std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err); warning (_("Cannot attach to lwp %d: %s"), lwpid, reason.c_str ()); gdb/gdbserver/linux-low.c: std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err); warning (_("Cannot attach to lwp %d: %s"), lwpid, reason.c_str ()); and std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err); error ("Cannot attach to process %ld: %s", pid, reason.c_str ()); It seems to me like these error messages are expecting a short string to just append to their existing strings, so I didn't think it made much sense to extend the ptrace error checking here as well. That's why I didn't extend linux_ptrace_attach_fail_reason_string to pass ERR down to linux_ptrace_attach_fail_reason. -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/