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 16:15, Andrew Burgess wrote:
>> 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?
>> 
>
> LGTM.
>
> I've also tested it on x86_64-linux, where the test-case still passes, 
> and on arm-linux, where the test-case now results in 3 times unsupported.
>

I pushed this patch.

Thanks,
Andrew