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

[Tom de Vries <tdevries@suse.de>]

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.

Thanks,
- Tom

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