8286030: Avoid JVM crash when containers share the same /tmp dir #9406

iklam · 2022-07-07T06:01:58Z

Some Kubernetes setups share the /tmp directory across multiple containers. On rare occasions, the JVM may crash when it tries to write to /tmp/hsperfdata_<user>/<pid> when a process in a separate container decides to do the same thing (because they happen to have the same namespaced pid).

This patch avoids the crash by using flock() to allow only one of these processes to write to the file. All other competing processes that fail to grab the lock will give up the file and run with PerfMemory disabled. We will try to enable PerfMemory for the failed processes in a follow-up RFE: JDK-8289883

Thanks to Vitaly Davidovich and Nico Williams for coming up with the idea of using flock().

I kept the use of kill() for stale file detection to be compatible with older JVMs.

I also took the opportunity to clean up the comments and remove dead code. The old code was using "shared memory resources" which sounds unclear and odd. I changed the terminology to say "shared memory file" instead.

(Note: this is a less ambitious revision of an earlier, withdrawn PR, #9226)

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8286030: Avoid JVM crash when containers share the same /tmp dir

Reviewers

Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to ecac352b
Severin Gehwolf (@jerboaa - Reviewer) ⚠️ Review applies to ecac352b

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9406/head:pull/9406
$ git checkout pull/9406

Update a local copy of the PR:
$ git checkout pull/9406
$ git pull https://git.openjdk.org/jdk pull/9406/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9406

View PR using the GUI difftool:
$ git pr show -t 9406

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9406.diff

…ing to delete

…he shared mem file

…are-tmp-dir

bridgekeeper · 2022-07-07T06:02:31Z

👋 Welcome back iklam! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-07-07T06:05:07Z

@iklam The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

iklam · 2022-07-07T06:36:09Z

/label add serviceability

openjdk · 2022-07-07T06:37:29Z

@iklam
The serviceability label was successfully added.

mlbridge · 2022-07-07T06:44:54Z

Webrevs

tstuefe

Hi Ioi,

not a full review, just a time-limited glance. Will take a closer look later.

Cheers, Thomas

src/hotspot/os/posix/perfMemory_posix.cpp

tstuefe

Just nits and questions. I like the test (did not know we have Docker support for tests in jtreg).

tstuefe · 2022-07-11T04:53:19Z

src/hotspot/os/posix/perfMemory_posix.cpp

-
-    pid_t pid = filename_to_pid(entry->d_name);
+    const char* filename = entry->d_name;
+    pid_t pid = filename_to_pid(filename);


Pre-existing. An error value of -1 would be somewhat cleaner since strictly speaking pid 0 is a valid PID. (Feel free to ignore this comment)

I think I'll leave it for now to minimize the behavior changes of this PR. I read somewhere that pid 0 is the scheduler

https://unix.stackexchange.com/questions/83322/which-process-has-pid-0

So no actual JVM process will create a stale file with name "0". If such a file exists for some other reason we would remove it, which be consistent with the new comment "any other files found in this directory may be removed".

tstuefe · 2022-07-11T04:55:01Z

src/hotspot/os/posix/perfMemory_posix.cpp

+    if (fd == OS_ERR) {
+      // Something wrong happened. Ignore the error and don't try to remove the
+      // file.
+      errno = 0;


Can we have a debug or trace log line here?

tstuefe · 2022-07-11T05:03:16Z

src/hotspot/os/posix/perfMemory_posix.cpp

+      unlink(filename);
+    }
+
+ #if defined(LINUX)


Indentation

tstuefe · 2022-07-11T05:07:05Z

src/hotspot/os/posix/perfMemory_posix.cpp

+ #if defined(LINUX)
+    // Hold the lock until here to prevent other JVMs from using this file
+    // while we are in the middle of deleting it.
+    ::close(fd);


I don't understand this comment, it seems to contradict what you are doing. We are closing the only fd referring to this lock file, right? So the lock should get unlocked here too? If we want to keep the lock open, shouldn't we avoid closing the fd?

I want to prevent the following race condition. Let's assume this process has PID 10 and there's another process (in a different pid namespace) with PID 20. Both process see a file named "20".

No one holds a lock on this file.

Process 20 successfully locks the file in cleanup_sharedmem_files().

Process 20 gives up the lock.

Process 20 decides it can delete the file (PID 20 matches its own PID).

This process successfully locks the file in cleanup_sharedmem_files().

This process gives up the lock

This process decides it can delete the file (PID 20 does not exist in my pid namespace)

Process 20 deletes the file. Creates a new version of this file. Successfully locks the new file.

This process deletes the new version of this file (by name).

By holding the lock between steps 4 and 8, we can guaranteed that if a process can successfully lock the file in create_sharedmem_file(), this file will never be unintentionally deleted.

I changed the comment slightly:

// Hold the lock until here to prevent other JVMs from using this file - // while we are in the middle of deleting it. + // while we were in the middle of deleting it.

Makes sense, thanks for explaining, and the past tense in the comment helps.

tstuefe · 2022-07-11T05:10:18Z

src/hotspot/os/posix/perfMemory_posix.cpp

+    log_warning(perf, memops)("Cannot use file %s/%s because %s", dirname, filename,
+                              (errno == EWOULDBLOCK) ?
+                              "it is locked by another process" :
+                              "flock() failed");


strerror would be helpful here, or at least errno

I added just the errno. Example:

[0.003s][warning][perf,memops] Cannot use file /tmp/hsperfdata_root/1 because it is locked by another process (errno = 11)

If we print the os::strerror() it would look like this:

... because it is locked by another process (errno = 11, Operation would block)

which seems too verbose and could be confusing.

tstuefe

Looks good!

tstuefe · 2022-07-12T18:55:59Z

src/hotspot/os/posix/perfMemory_posix.cpp

+ #if defined(LINUX)
+    // Hold the lock until here to prevent other JVMs from using this file
+    // while we are in the middle of deleting it.
+    ::close(fd);


Makes sense, thanks for explaining, and the past tense in the comment helps.

openjdk · 2022-07-12T19:11:49Z

@iklam 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:

8286030: Avoid JVM crash when containers share the same /tmp dir

Reviewed-by: stuefe, sgehwolf

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

iklam · 2022-07-14T21:40:11Z

@jerboaa could you take a look? Thanks.

jerboaa · 2022-07-15T12:08:02Z

test/hotspot/jtreg/containers/docker/ShareTmpDir.java

+        if (file1.equals(file2)) {
+            // This should be the common case -- the first started process in a container should
+            // have pid==1.
+            // One of the two contains must fail to create the hsperf file.


s/contains/containers/

jerboaa

LGTM. My manual tests of this work as expected as well.

$ podman run --rm -ti --userns=keep-id -u $(id -u) -v $(pwd)/shared-tmp:/tmp:z -v /disk/openjdk/upstream-sources/git/jdk-jdk/build/linux-x86_64-server-release/images/jdk:/opt/jdk:z -v $(pwd)/test:/opt/test:z fedora:36 /opt/jdk/bin/java -Xlog:perf+memops=debug -cp /opt/test HelloWait
[0.001s][debug][perf,memops] PerfDataMemorySize = 32768, os::vm_allocation_granularity = 4096, adjusted size = 32768
[0.001s][info ][perf,memops] Trying to open /tmp/hsperfdata_sgehwolf/1
[0.001s][info ][perf,memops] Successfully opened
[0.001s][debug][perf,memops] PerfMemory created: address = 0x00007fac290dd000, size = 32768
Hello!
$ podman run --rm -ti --userns=keep-id -u $(id -u) -v $(pwd)/shared-tmp:/tmp:z -v /disk/openjdk/upstream-sources/git/jdk-jdk/build/linux-x86_64-server-release/images/jdk:/opt/jdk:z -v $(pwd)/test:/opt/test:z fedora:36 /opt/jdk/bin/java -Xlog:perf+memops=debug -cp /opt/test HelloWait
[0.001s][debug][perf,memops] PerfDataMemorySize = 32768, os::vm_allocation_granularity = 4096, adjusted size = 32768
[0.001s][debug][perf,memops] flock for stale file check failed for /tmp/hsperfdata_sgehwolf/1
[0.001s][info ][perf,memops] Trying to open /tmp/hsperfdata_sgehwolf/1
[0.001s][warning][perf,memops] Cannot use file /tmp/hsperfdata_sgehwolf/1 because it is locked by another process (errno = 11)
[0.001s][debug  ][perf,memops] PerfMemory created: address = 0x00007fc60bc79000, size = 32768
Hello!

…avoid-jvm-crash-when-containers-share-tmp-dir

iklam · 2022-07-18T04:09:09Z

Thanks to @tstuefe and @jerboaa for the review.
/integrate

openjdk · 2022-07-18T04:10:26Z

Going to push as commit 84f2314.

openjdk · 2022-07-18T04:10:42Z

@iklam Pushed as commit 84f2314.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

ioilam added 7 commits June 29, 2022 11:27

temp-not-working

c2aaa88

working

13e47b6

fix timing hole -- another JVM process can create file while I am try…

6a3683c

…ing to delete

Added test case

fd22cd4

fixed comments

a73efb1

Cleaned up comments and avoid using the word "resource" to describe t…

f89b2e8

…he shared mem file

Merge branch 'master' into 8286030-avoid-jvm-crash-when-containers-sh…

fef8058

…are-tmp-dir

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jul 7, 2022

iklam mentioned this pull request Jul 7, 2022

8286030: Support sharing of /tmp across Linux containers #9226

Closed

3 tasks

openjdk bot added the serviceability serviceability-dev@openjdk.org label Jul 7, 2022

iklam marked this pull request as ready for review July 7, 2022 06:39

openjdk bot added the rfr Pull request is ready for review label Jul 7, 2022

tstuefe reviewed Jul 7, 2022

View reviewed changes

@tstuefe comments

43f67c4

tstuefe reviewed Jul 8, 2022

View reviewed changes

iklam added 3 commits July 8, 2022 23:27

Simplified code and improved comments

f1efc47

added more comments

7324293

fixed typo

7825491

tstuefe reviewed Jul 11, 2022

View reviewed changes

iklam added 2 commits July 12, 2022 11:18

added debug log and tweaked comment

a4d7716

add errno to log

ecac352

tstuefe approved these changes Jul 12, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 12, 2022

jerboaa reviewed Jul 15, 2022

View reviewed changes

jerboaa approved these changes Jul 15, 2022

View reviewed changes

iklam added 2 commits July 17, 2022 20:18

Fixed typo in comments

f79904e

Merge branch 'master' of https://github.com/openjdk/jdk into 8286030-…

efc00f6

…avoid-jvm-crash-when-containers-share-tmp-dir

openjdk bot added the integrated Pull request has been integrated label Jul 18, 2022

openjdk bot closed this Jul 18, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8286030: Avoid JVM crash when containers share the same /tmp dir #9406

8286030: Avoid JVM crash when containers share the same /tmp dir #9406

iklam commented Jul 7, 2022 •

edited

Loading

bridgekeeper bot commented Jul 7, 2022

openjdk bot commented Jul 7, 2022

iklam commented Jul 7, 2022

openjdk bot commented Jul 7, 2022

mlbridge bot commented Jul 7, 2022 •

edited

Loading

tstuefe left a comment

tstuefe left a comment

tstuefe Jul 11, 2022

iklam Jul 12, 2022

tstuefe Jul 11, 2022

iklam Jul 12, 2022

tstuefe Jul 11, 2022

iklam Jul 12, 2022

tstuefe Jul 11, 2022

iklam Jul 12, 2022

tstuefe Jul 12, 2022

tstuefe Jul 11, 2022

iklam Jul 12, 2022

tstuefe left a comment

tstuefe Jul 12, 2022

openjdk bot commented Jul 12, 2022 •

edited

Loading

iklam commented Jul 14, 2022

jerboaa Jul 15, 2022

jerboaa left a comment

iklam commented Jul 18, 2022

openjdk bot commented Jul 18, 2022

openjdk bot commented Jul 18, 2022

8286030: Avoid JVM crash when containers share the same /tmp dir #9406

8286030: Avoid JVM crash when containers share the same /tmp dir #9406

Conversation

iklam commented Jul 7, 2022 • edited Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jul 7, 2022

openjdk bot commented Jul 7, 2022

iklam commented Jul 7, 2022

openjdk bot commented Jul 7, 2022

mlbridge bot commented Jul 7, 2022 • edited Loading

Webrevs

tstuefe left a comment

Choose a reason for hiding this comment

tstuefe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstuefe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Jul 12, 2022 • edited Loading

iklam commented Jul 14, 2022

Choose a reason for hiding this comment

jerboaa left a comment

Choose a reason for hiding this comment

iklam commented Jul 18, 2022

openjdk bot commented Jul 18, 2022

openjdk bot commented Jul 18, 2022

iklam commented Jul 7, 2022 •

edited

Loading

mlbridge bot commented Jul 7, 2022 •

edited

Loading

openjdk bot commented Jul 12, 2022 •

edited

Loading