Skip to content

Commit

Permalink
Ensure our mutexes handle recursive locking properly
Browse files Browse the repository at this point in the history
We use shared-memory pthread mutexes to handle mutual exclusion
in Libpod. It turns out that these have configurable options for
how to handle a recursive lock (IE, a thread trying to lock a
lock that the same thread had previously locked). The mutex can
either deadlock, or allow the duplicate lock without deadlocking.
Default behavior is, helpfully, unspecified, so if not explicitly
set there is no clear indication of which of these behaviors will
be seen. Unfortunately, today is the first I learned of this, so
our initial implementation did *not* explicitly set our preferred
behavior.

This turns out to be a major problem with a language like Golang,
where multiple goroutines can (and often do) use the same OS
thread. So we can have two goroutines trying to stop the same
container, and if the no-deadlock mutex behavior is in use, both
threads will successfully acquire the lock because the C library,
not knowing about Go's lightweight threads, sees the same PID
trying to lock a mutex twice, and allows it without question.

It appears that, at least on Fedora/RHEL/Debian libc, the default
(unspecified) behavior of the locks is the non-deadlocking
version - so, effectively, our locks have been of questionable
utility within the same Podman process for the last four years.
This is somewhat concerning.

What's even more concerning is that the Golang-native sync.Mutex
that was also in use did nothing to prevent the duplicate locking
(I don't know if I like the implications of this).

Anyways, this resolves the major issue of our locks not working
correctly by explicitly setting the correct pthread mutex
behavior.

Signed-off-by: Matthew Heon <matthew.heon@pm.me>
  • Loading branch information
mheon committed Jun 6, 2023
1 parent 0a5e8f0 commit 7f43cf5
Showing 1 changed file with 8 additions and 0 deletions.
8 changes: 8 additions & 0 deletions libpod/lock/shm/shm_lock.c
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,14 @@ shm_struct_t *setup_lock_shm(char *path, uint32_t num_locks, int *error_code) {
goto CLEANUP_UNMAP;
}

// Ensure that recursive locking of a mutex by the same OS thread (which may
// refer to numerous goroutines) blocks.
ret_code = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_NORMAL);
if (ret_code != 0) {
*error_code = -1 * ret_code;
goto CLEANUP_FREEATTR;
}

// Set mutexes to pshared - multiprocess-safe
ret_code = pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
if (ret_code != 0) {
Expand Down

0 comments on commit 7f43cf5

Please sign in to comment.