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

[Thomas Stuefe]

On Thu, 7 Jul 2022 06:01:58 GMT, Ioi Lam <iklam at openjdk.org> wrote:

> 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](https://bugs.openjdk.org/browse/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.

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 line 708:

> 706:   fd = ::open(filename, O_RDONLY);
> 707:   if (fd >= 0) {
> 708:     is_locked = (flock(fd, LOCK_EX|LOCK_NB) != 0);

Out of the n error conditions for flock(), only EWOULDBLOCK indicates a occupied lock. I would handle the rest differently.

src/hotspot/os/posix/perfMemory_posix.cpp line 720:

> 718: 
> 719:   return is_locked;
> 720: }

The interface of this function seems weirdly unbalanced and threw me off at first. We hand in fd=-1, and it opens the fd, maybe closes the fd, maybe returns a valid fd (via reference, one has to look closely), maybe returns an invalid but valid-looking fd...

I would disentangle this a bit either bei moving the open and the close out to the caller and making this a simple 


static bool is_locked_by_another_process(int fd);


or, alternatively, just inlining the whole section into the one place where we use it.

src/hotspot/os/posix/perfMemory_posix.cpp line 770:

> 768:     // create_sharedmem_file() and is_locked_by_another_process().
> 769:     // If it's already locked by another process, then obviously it's
> 770:     // not stale

Period missing.

src/hotspot/os/posix/perfMemory_posix.cpp line 781:

> 779:     // signal the process, then the file is assumed to
> 780:     // be stale and is removed because the files for such a
> 781:     // process should be in a different user specific directory.

I am not sure this is good. If two conflicting hotspots share the same PID in tmp, from two different users, this will probably be a setup error. Is the best way really to provoke SIGBUS in the other VM? Seems a bit harsh. 

Also terminology would be wrong. Its not stale then, since the target process probably exists, is a VM, and uses that file.

-------------

PR: https://git.openjdk.org/jdk/pull/9406