8226919: attach in linux hangs due to permission denied accessing /proc/pid/root #17628
Conversation
|
👋 Welcome back slovdahl! A progress list of the required criteria for merging this PR into |
|
/issue add JDK-8226919 |
|
@slovdahl |
|
I have poked around in the JDK sources but not found any tests related to this. Is there some prior art to look at? Anyway, this is how I reproduced it locally, and verified that the fix works. Basic environment information: Reproducer systemd unit that can bind to port 81 as non- Fails with vanilla OpenJDK 17: Works when trying to attach as A JDK built with the fix in this PR: Attaching works as non- Attaching to a JVM inside a Docker container works as before with vanilla OpenJDK 17 and my locally built one (always requires |
slovdahl
changed the title
|
ping @jerboaa |
| // A process may not exist in the same mount namespace as the caller. | ||
| // Instead, attach relative to the target root filesystem as exposed by | ||
| // procfs regardless of namespaces. | ||
| String root = "/proc/" + pid + "/root/" + tmpdir; |
There was a problem hiding this comment.
Helping myself and other future readers understand this: the problem with the previous implementation is that the code assumed that the tmpdir could be accessed this way (/proc/<pid>/root/<tmpdir>). In other words:
- The code for creating the socket would correctly check if
pid != ns_pidand then act accordingly (/proc/<pid>/root/<tmpdir>or just plain<tmpdir>) - The code for reading the socket would not have the check the above. It would resort to always use
/proc/<pid>/root/<tmpdir>. - For certain scenarios (
CAP_NET_BIND_SERVICE-processes, as described in 8226919: attach in linux hangs due to permission denied accessing /proc/pid/root #17628 (comment)), we would get aPermission deniedwhen trying to access the temporary directory like this.
What this PR does is to ensure that the same pid != ns_pid check is used both when creating and reading the socket, and fall back to <tmpdir> when no namespacing is being used. This seems to work better for these processes with elevated permissions.
/cc @slovdahl, feel free to add/correct the above if needed.
There was a problem hiding this comment.
should it not be comparing pid namespace ids and not pids?
There was a problem hiding this comment.
Do you mean that it should compare the input PID against the outermost (leftmost) PID in the NSpid list from /proc/<pid>/status and not innermost (rightmost) as is done right now? What would be the benefit of that? Or did you mean something else?
I'm working on a fix for https://bugs.openjdk.org/browse/JDK-8327114 right now, and it occurred to me that there is a tiny risk of pid != ns_pid not evaluating to true even though the processes are in different PID namespaces (because two different PID namespaces can have the same PIDs). I think it could be mitigated by always trying /proc/<pid>/root/tmp first, and if it cannot be read, fall back to /tmp.
There was a problem hiding this comment.
c.f: /proc//ns/pid
every (Linux) namespace has a unique id, if 2 (or more) processes occupy the same pid namespace (or any other for that matter) then their ../ns/pid namespace ids will be the same.
There was a problem hiding this comment.
Files.readSymbolicLink(Path.of("/proc/self/ns/pid"))
There was a problem hiding this comment.
h'mmm ignore my ramblings for now, I need to spend some more time looking into this before wading into the fray with random opinions etc!
Please run container tests, which do some jcmd testing across containers (host system runs |
|
|
|
Mailing list message from Bernd Eckenfels on serviceability-dev: Is that actually safe to allow low priveledged user context to attach and control to a higher prived? It can at least overwrite files, but probably also inject code? On the native level a ptrace(2) would probably not be allowed. Gru? |
|
Hi @jerboaa, thanks a lot for the hints! The container tests were new to me at least. Just out of curiosity, are the container tests run as part of the tier1 tests (
If I understand it correctly, this is the way to run them. Please correct me if I'm wrong.
|
It's a good question. For context, this has worked fine in JDK 8, and AFAIK it was never intentionally broken for security reasons. In some cases the opposite can also be true - that one needs root access to attach to a process is not acceptable or even possible. |
Thanks! Please make sure that the tests actually ran. If, for example, |
Ah, good point. Running the tests did take some amount of time, so it felt like they did something. And by spamming |
Note that for the dynamic attach mechanism the file ownership of the files the JVM creates on both sides need to match. In this case it's user |
|
/reviewers 2 |
jerboaa
left a comment
jerboaa
left a comment
There was a problem hiding this comment.
This looks good to me, but would like for somebody from the serviceability group to look at this as well. @plummercj perhaps?
|
Hi, Does CAP_NET_BIND_SERVICE cause any issues for createAttachFile(int pid, int ns_pid) where it creates the .attach file in the current directory - it starts by trying "/proc/" + pid + "/cwd/" + ".attach_pid" + ns_pid, regardless of ns_pid. I'm curious if that always fails in the situation that causes the issue in this bug. Thanks! |
Hi @kevinjwalls, and thank you for taking a look! To make sure we're on the same page, is what you are asking if something like this would make sense (on top of the current state of the PR)? diff --git src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
index 81d4fd259ed..c06c972b39a 100644
--- src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
+++ src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
@@ -221,16 +221,19 @@ private File findSocketFile(int pid, int ns_pid) throws IOException {
// checks for the file.
private File createAttachFile(int pid, int ns_pid) throws IOException {
String fn = ".attach_pid" + ns_pid;
- String path = "/proc/" + pid + "/cwd/" + fn;
- File f = new File(path);
- try {
- // Do not canonicalize the file path, or we will fail to attach to a VM in a container.
- f.createNewFile();
- } catch (IOException x) {
+
+ File f;
+ if (pid != ns_pid) {
+ String path = "/proc/" + pid + "/cwd/" + fn;
+ f = new File(path);
+ } else {
String root = findTargetProcessTmpDirectory(pid, ns_pid);
f = new File(root, fn);
- f.createNewFile();
}
+
+ // Do not canonicalize the file path, or we will fail to attach to a VM in a container.
+ f.createNewFile();
+
return f;
}
That is, if we know that That's a good question. I tried to minimize the changes because I'm so unfamiliar with JDK internals and also don't have a good understanding of all the different use-cases that need to work. I tried out the diff above locally using the reproducer steps from #17628 (comment). It seems to work equally fine in the case of a systemd unit using The That said, I'm still more confident in the current state of the PR, as it more closely follows what has existed before. But if you believe that this is a better way of handling it, I'm fine with that too. |
|
Thanks, yes that's what I was thinking about. I tested setting I see the failure to attach, and I see it fixed by this change. We catch this and retry in /tmp. But exactly as in your response, we can predict that and if the target is not in a namespace, go straight to /tmp. I tested what you have there and it works well. Also tested that a new jcmd attaching to an older JDK, that still works. One other thing - JDK-8226919 looks like the original bug for this, logged a few years back, so if this fixes both, the record should show that it fixes that one, and JDK-8307977 should be closed as a duplicate. I/somebody can take care of that JBS admin. But if this PR could be associated with only JDK-8226919 that would be simple. Thanks! 8-) |
|
Will this result in files being left in /tmp that are not cleaned up during test runs? |
It shouldn't... We do cleanup, VirtualMachineImpl creates the attach file and deletes it in a finally block. |
|
Cool. Thanks for the confirmation. |
|
Hi, looking at it again: When getting the target's current directory, /proc/PID/cwd will fail for the evelated processes, but will work for others (where permissions allow). So that was all very interesting, but let me just approve what we have here already. No need to proceed with the additional code you had in the comment response above. 8-) |
|
@slovdahl This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 139 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jerboaa, @kevinjwalls) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
Alright, sounds good to me. :) Thanks again for taking a look!
I'll still fix this. So, I should change the PR title to match JDK-8226919, and issue an Once that is done, I would kindly ask for someone sponsoring this change as well. |
Yes exactly, thanks. |
slovdahl
changed the title
|
/issue remove JDK-8307977 |
|
@slovdahl This PR does not contain any additional solved issues that can be removed. To remove the primary solved issue, simply edit the title of this PR. |
|
/integrate |
|
/sponsor |
|
Going to push as commit ac4607e.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@kevinjwalls @slovdahl Pushed as commit ac4607e. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
slovdahl
deleted the
8307977-fix-dynamic-attach-for-target-process-with-elevated-capability
branch
|
Thank you @kevinjwalls and @jerboaa for reviewing and guiding me through this process, this was a great as a first-time JDK contributor! One more question, can I do anything to help getting this backported to e.g. 21 and 17? |
First, I suggest to wait a few weeks in order to see if there are any follow-up bugs which show up in testing in mainline. Then start backporting it to 22u, then 21u, then 17u (in that order). A few references: https://openjdk.org/guide/#backporting |
|
@slovdahl - Apologies for adding a comment to a closed Pull Request, but I happened on https://bugs.openjdk.org/browse/JDK-8307977 via the earlier https://bugs.openjdk.org/browse/JDK-8179498 after researching "AttachNotSupportedException: Unable to open socket file" and troubleshooting our own OpenJDK 17 jcmd setup on top of containers and Kubernetes. Reading the code changes and discussion here, I'm concerned that this change, which I understand is not yet in OpenJDK 17, might cause a regression with our setup. We're running jcmd (OpenJDK build 17.0.10+7-LTS) and the target JVM in two separate containers in a Kubernetes pod. The target JVM container is already running, and then we use I believe the attach file and socket file paths then work like this in OpenJDK 17:
I think this scenario with a Kubernetes debug container may be a little different from other Docker container scenarios because these are two different containers with different root filesystems but the same Linux process namespace. So jcmd using If I understand the code change for this PR, I think it will change the behavior in this scenario, because We are lucky currently that the only place the current OpenJDK 17 code checks Could the Thanks for your time! |
That is certainly worth capturing in a new JBS bug for investigating a further change. If you can't log one, I'll use the information here to do that, thanks! |
|
Logged https://bugs.openjdk.org/browse/JDK-8327114 for investigation. Thanks @jdoylei ! |
|
@kevinjwalls - Perfect, thank you for opening the JBS bug! |
|
Thanks for the detailed write-up, @jdoylei! I'm sorry to have introduced a regression here. Good that the backporting was held off a bit at least :) Let's continue the discussion at https://mail.openjdk.org/pipermail/serviceability-dev/2024-April/055317.html. |
8 participants
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17628/head:pull/17628$ git checkout pull/17628Update a local copy of the PR:
$ git checkout pull/17628$ git pull https://git.openjdk.org/jdk.git pull/17628/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 17628View PR using the GUI difftool:
$ git pr show -t 17628Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17628.diff
Webrev
Link to Webrev Comment