Re: [PATCHv3] gdb: linux-namespaces: enter user namespace when appropriate

[Andrew Burgess <aburgess@redhat.com>]

Tom de Vries <tdevries@suse.de> writes:

> On 6/25/25 12:34, Andrew Burgess wrote:
>> Tom de Vries <tdevries@suse.de> writes:
>> 
>>> On 6/23/25 15:56, Andrew Burgess wrote:
>>>> Andrew Burgess <aburgess@redhat.com> writes:
>>>>
>>>>> From: Benjamin Berg <benjamin@sipsolutions.net>
>>>>>
>>>>> In v2:
>>>>>
>>>>>     - Update the test to ignore a warning seen when running the test on
>>>>>       a machine with libc debug information installed, but without the
>>>>>       libc source being available, e.g.:
>>>>>
>>>>>       warning: 46     ../sysdeps/unix/sysv/linux/arm/libc-do-syscall.S: No such file or directory
>>>>>
>>>>>       This was causing some CI failures to be reported from Linaro.
>>>>>
>>>>>     - Rebased to current upstream/master.
>>>>>
>>>>> In v3:
>>>>>
>>>>>     - Same as V2, but fix the test pattern correctly this time.
>>>>>
>>>>> --
>>>>>
>>>>> The use of user namespaces is required for normal users to use mount
>>>>> namespaces.  Consider trying this as an unprivileged user:
>>>>>
>>>>>     $ unshare --mount /bin/true
>>>>>     unshare: unshare failed: Operation not permitted
>>>>>
>>>>> The problem here is that an unprivileged user doesn't have the
>>>>> required permissions to create a new mount namespace.  If, instead, we
>>>>> do this:
>>>>>
>>>>>     $ unshare --mount --map-root-user /bin/true
>>>>>
>>>>> then this will succeed.  The new option causes unshare to create a
>>>>> user namespace in which the unprivileged user is mapped to UID/GID 0,
>>>>> and so gains all privileges (inside the namespace), the user is then
>>>>> able to create the mount namespace as required.
>>>>>
>>>>> So, how does this relate to GDB?
>>>>>
>>>>> When a user attaches to a process running in a separate mount
>>>>> namespace, GDB makes use of a separate helper process (see
>>>>> linux_mntns_get_helper in nat/linux-namespaces.c), which will then use
>>>>> the `setns` function to enter (or try to enter) the mount namespace of
>>>>> the process GDB is attaching too.  The helper process will then handle
>>>>> file I/O requests received from GDB, and return the results back to
>>>>> GDB, this allows GDB to access files within the mount namespace.
>>>>>
>>>>> The problem here is that, switching to a mount namespace requires that
>>>>> a process hold CAP_SYS_CHROOT and CAP_SYS_ADMIN capabilities within
>>>>> its user namespace (actually it's a little more complex, see 'man 2
>>>>> setns').  Assuming GDB is running as an unprivileged user, then GDB
>>>>> will not have the required permissions.
>>>>>
>>>>> However, if GDB enters the user namespace that the `unshare` process
>>>>> created, then the current user will be mapped to UID/GID 0, and will
>>>>> have the required permissions.
>>>>>
>>>>> And so, this patch extends linux_mntns_access_fs (in
>>>>> nat/linux-namespace.c) to first try and switch to the user namespace
>>>>> of the inferior before trying to switch to the mount namespace.  If
>>>>> the inferior does have a user namespace, and does have elevated
>>>>> privileges within that namespace, then this first switch by GDB will
>>>>> mean that the second step, into the mount namespace, will succeed.
>>>>>
>>>>> If there is no user namespace, or the inferior doesn't have elevated
>>>>> privileges within the user namespace, then the switch into the mount
>>>>> namespace will fail, just as it currently does, and the user will need
>>>>> to give elevated privileges to GDB via some other mechanism (e.g. run
>>>>> as root).
>>>>>
>>>>> This work was originally posted here:
>>>>>
>>>>>     https://inbox.sourceware.org/gdb-patches/20230321120126.1418012-1-benjamin@sipsolutions.net
>>>>>
>>>>> I (Andrew Burgess) have made some cleanups to the code to comply with
>>>>> GDB's coding standard, and the test is entirely mine.  This commit
>>>>> message is also entirely mine -- the original message was very terse
>>>>> and required the reader to understand how the various namespaces
>>>>> work and interact.  The above is my attempt to document what I now
>>>>> understand about the problem being fixed.
>>>>>
>>>>> I've left the original author in place as the core of the GDB change
>>>>> itself is largely as originally presented, but any inaccuracies in the
>>>>> commit message, or problems with the test, are all mine.
>>>>>
>>>>> Co-Authored-by: Andrew Burgess <aburgess@redhat.com>
>>>>
>>>> I've pushed this patch.
>>>>
>>>
>>> The new test-case fails on arm32 (Linaro CI reported this, and I was
>>> able to reproduce) due to insufficient permissions:
>>> ...
>>> (gdb) attach 184732
>>> Attaching to process 184732
>>> warning: process 184732 is a zombie - the process has already terminated
>>> ptrace: Operation not permitted.
>>> (gdb) FAIL: gdb.base/user-namespace-attach.exp: flags=--mount
>>> --map-root-user: attach to inferior
>>> ...
>>>
>>> In essence, the test-case assumes:
>>> ...
>>> $ unshare --mount --map-root-user /bin/true; echo $?
>>> 0
>>> ...
>>> but we get instead:
>>> ...
>>> $ unshare --mount --map-root-user /bin/true; echo $?
>>> unshare: unshare failed: Operation not permitted
>>> 1
>>> ...
>>>
>>> Filed here ( https://sourceware.org/bugzilla/show_bug.cgi?id=33108 ).
>> 
>> Hi!
>> 
>> Thanks for raising this issue.
>> 
>> What do you think to the patch below?
>> 
>> I've tested this by passing a bogus flag to `unshare`, e.g. "unshare
>> -blahblah", which has the same effect of causing the `unshare` process
>> to exit immediately with an exit code of 1.  I now see the test reported
>> as unsupported.
>> 
>
> Hi Andrew,
>
> thanks for picking this up.
>
> FWIW, the problem with this solution is that a timeout now looks like 
> unsupported.
>
> More concretely, by doing this (on x86_64-linux, where the test-case 
> passes for me):
> ...
> diff --git a/gdb/testsuite/gdb.base/user-namespace-attach.exp 
> b/gdb/testsuite/gdb.base/user
> -namespace-attach.exp
> index 01f3dae1693..c5ec5ef6369 100644
> --- a/gdb/testsuite/gdb.base/user-namespace-attach.exp
> +++ b/gdb/testsuite/gdb.base/user-namespace-attach.exp
> @@ -66,6 +66,7 @@ proc run_test { flags } {
>       }
>
>       set inferior_pid [spawn_id_get_pid $inferior_spawn_id]
> +    sleep 90
>
>       clean_restart
>
> ...
> I trigger the timeout of 60 seconds in the exec, and with your patch get:
> ...
> 		=== gdb Summary ===
>
> # of unsupported tests		3
> ...
> but without your patch I get:
> ...
> # of unexpected failures	6
> ...
>
> I don't think it's terribly important though.
>
> You could try the approach I proposed in the PR, or you could pursue 
> this one.
>
> In the latter case, please add a comment that a timeout may trigger the 
> same message.

You make a good point.  I think your suggestion is probably the best
approach then.  How about the patch below?

Thanks,
Andrew

---

4d0265f72da gdb/testsuite: handle failure to start process for later attach test [Andrew Burgess (2 minutes ago)]

diff --git a/gdb/testsuite/gdb.base/user-namespace-attach.exp b/gdb/testsuite/gdb.base/user-namespace-attach.exp
index 9936bb998eb..741093c2c14 100644
--- a/gdb/testsuite/gdb.base/user-namespace-attach.exp
+++ b/gdb/testsuite/gdb.base/user-namespace-attach.exp
@@ -56,10 +56,22 @@ proc run_test { flags } {
 	set prefix ""
     }
 
+    set unshare_cmd "unshare $flags"
 
+    # Run '/bin/true' using UNSHARE_CMD.  If the flags in UNSHARE_CMD
+    # aren't supported then this will fail, this means we shouldn't
+    # spawn the command with our test executable and try attaching.
+    #
+    # This will also fail if /bin/true isn't present, or doesn't work
+    # as we expect.  But this should be fine for many targets.
+    set res [remote_exec target "$unshare_cmd /bin/true"]
+    if { [lindex $res 0] != 0 } {
+	unsupported "unshare flags not supported"
+	return
+    }
 
     set inferior_spawn_id \
-	[spawn_wait_for_attach [list "unshare $flags $::binfile"]]
+	[spawn_wait_for_attach [list "$unshare_cmd $::binfile"]]
     if { $inferior_spawn_id == -1 } {
 	unsupported "failed to spawn for attach"
 	return