RFR: 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container)

[Laurence Cable]

I'll ponder ... have a good weekend!

regardless I think the added check for mnt ns comparison "adds value" by 
expressing the constraints explicitly vs comparing pid & ns pid

Rgds

- Larry


On 5/3/24 9:45 AM, Sebastian Lövdahl wrote:
> On Thu, 2 May 2024 10:13:51 GMT, Sebastian Lövdahl <duke at openjdk.org> wrote:
>
>> 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container)
> Thanks for the patch @larry-cable, much appreciated! I really like this idea.
>
> I tried it out a bit locally. These cases seem to work:
> - attaching to a process running on the same host (PID and mount namespace the same)
> - attaching as root from the host to a JVM inside a container
> - attaching from a sidecar container to a JVM in another container
>
> Unfortunately, attaching to a JVM process running as the same user in the same PID and mount namespace but one that has elevated privileges no longer works (the case that JDK-8226919 fixed). That case ends up failing like this with the patch:
>
>
> slovdahl at ubuntu2204:~/reproducer$ /home/slovdahl/dev/external/jdk/build/linux-x86_64-server-release/images/jdk/bin/jcmd 1751545 VM.version
> 1751545:
> java.io.IOException: Unable to access root directory /proc/1751545/root of target process 1751545
> 	at jdk.attach/sun.tools.attach.VirtualMachineImpl.findTargetProcessTmpDirectory(VirtualMachineImpl.java:284)
> 	at jdk.attach/sun.tools.attach.VirtualMachineImpl.findSocketFile(VirtualMachineImpl.java:229)
> 	at jdk.attach/sun.tools.attach.VirtualMachineImpl.<init>(VirtualMachineImpl.java:86)
> 	at jdk.attach/sun.tools.attach.AttachProviderImpl.attachVirtualMachine(AttachProviderImpl.java:58)
> 	at jdk.attach/com.sun.tools.attach.VirtualMachine.attach(VirtualMachine.java:207)
> 	at jdk.jcmd/sun.tools.jcmd.JCmd.executeCommandForPid(JCmd.java:113)
> 	at jdk.jcmd/sun.tools.jcmd.JCmd.main(JCmd.java:97)
>
>
> I _think_ it boils down to the same reason as why the fix for JDK-8226919 was needed in the first place - a non-root user cannot read the symlinks in `/proc/<pid>/ns` for a process running with more privileges even though it's run by the same non-root user.
>
>
> slovdahl at ubuntu2204:/proc/1751545/ns$ ls -lh
> ls: cannot read symbolic link 'net': Permission denied
> ls: cannot read symbolic link 'uts': Permission denied
> ls: cannot read symbolic link 'ipc': Permission denied
> ls: cannot read symbolic link 'pid': Permission denied
> ls: cannot read symbolic link 'pid_for_children': Permission denied
> ls: cannot read symbolic link 'user': Permission denied
> ls: cannot read symbolic link 'mnt': Permission denied
> ls: cannot read symbolic link 'cgroup': Permission denied
> ls: cannot read symbolic link 'time': Permission denied
> ls: cannot read symbolic link 'time_for_children': Permission denied
> total 0
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 cgroup
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 ipc
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:27 mnt
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 net
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 pid
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 pid_for_children
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 time
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 time_for_children
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 user
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 uts
> slovdahl at ubuntu2204:/proc/1751545/ns$ sudo ls -lh
> total 0
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 cgroup -> 'cgroup:[4026531835]'
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 ipc -> 'ipc:[4026531839]'
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:27 mnt -> 'mnt:[4026533862]'
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 net -> 'net:[4026531840]'
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 pid -> 'pid:[4026531836]'
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 pid_for_children -> 'pid:[4026531836]'
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 time -> 'time:[4026531834]'
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 time_for_children -> 'time:[4026531834]'
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 user -> 'user:[4026531837]'
> lrwxrwxrwx 1 slovdahl slovdahl 0 maj  3 19:28 uts -> 'uts:[4026531838]'
>
>
> FWIW, my IntelliJ also highlighted the fact that the suggested patch contains unreachable code. The `else throw new IOException(String.format("target process:%d and this do not share common mount namespace for: %s attach failed", pid, tmpdir));` path can never be taken, because either the `if` statement evaluates to `true`, or the `else if`.
>
> I'm not sure if we can do any better than always falling back to `/tmp`? But if anyone has suggestions, I'm certainly happy to try it out.
>
> -------------
>
> PR Comment: https://git.openjdk.org/jdk/pull/19055#issuecomment-2093378574