nptl: futex_lock_pi deadlock detection provides valuable information but it is turned into a rather cryptic assertion failure
Checks
| Context |
Check |
Description |
| redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
| linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Test passed
|
Commit Message
Thanks for your detailed review. Those additional test support helpers
I haven't been aware of do help simplify the code a great amount indeed.
>> +#ifndef TST_DEADLK_MUTEX_PI
>> +# define TST_DEADLK_MUTEX_PI 0
>> +#endif
>
> Just use the TST_DEADLK_MUTEX_PI=1 and move this test to
> tst-deadlk-pi.c.
Ah, sorry, I forgot to explain that point in my previous message. I had
originally added this configurability based on the assumption that we
might want to use it to also test that non-PI mutexes actually do
deadlock by making these XFAIL tests, but didn't went as far as actually
doing that. However, based on your other feedback ...
>> The added test case currently only checks for EDEADLK to be returned,
>> but doesn't cover the cases where we still expect the deadlock to
>> happen. I could add these as well, but it would introduce an
>> (unreasonably?) long delay, waiting for the alarm clock to go off
>> eventually. Would you rather take this delay over the lack of test
>> coverage? Please also let me know whether having one test executable
>> that loops over the various types (current patch) or one executable
>> per type would be preferred.
>
> I would prefer the later (one less binary to build and run). For the
> deadlock to happen, it would be better to spawn a new process with
> support_capture_subprocess, trigger the deadlock, and wait it with
> short delayed_exit value.
... I went ahead and merged everything into a single tst-deadlk.c file
which now checks PI and non-PI mutexes alike. Since the number of
combinations became quite large, I refactored the test code to remove
the explicit list in lieu of some nested loops and a simple
should_detect_deadlock predicate function.
I've picked 3 s for a delayed_exit value, hoping that this wold be a
sensible choice, given that nptl/tst-eintr1.c uses the same (and seems
to be the only other NPTL test case currently using delayed_exit).
Alas, this makes the test register approx. 30 s on the clock. I've
raised the overall timeout (which should never be hit) to 60 s
accordingly. Are you okay which such a slow test or do we need to
improve on this? We could, for example, do all the waiting for all
the subprocesses in parallel, but I'm afraid that it would add quite
some complexity. A lower-hanging fruit might be to reduce the delay
from 3 s to 1 s, which has worked well for my computer even with high
concurrent CPU load, but I'm not sure how universally acceptable it
might be.
I found that still using alarm(3) in the subprocess and then expecting
-SIGALRM in the parent would result in slightly simpler code compared
to the delayed_exit but since you recommended it and I've also seen
that other test cases have been refactored in the past specifically to
replace alarm with delayed_exit, I assume that this is preferred way
to go.
Looking forward to your feedback on the updated patch.
From de343146051b75e96e04f4167c2b6148990abca0 Mon Sep 17 00:00:00 2001
From: Moritz Klammler <moritz.klammler.ext@siemens.com>
Date: Tue, 7 Apr 2026 21:36:12 +0200
Subject: [PATCH 1/1] nptl: Propagate EDEADLK from FUTEX_LOCK_PI for
errror-checking and recursive mutexes
This patch changes the behavior of pthread_mutex_lock for error-checking and
recursive PI mutexes in case of non-trivial deadlock. The user-space code
doesn't detect the case where two or more threads would mutually deadlock each
other, but the Linux kernel can. NPTL's previous behavior, if the syscall
returns EDEADLK, was to run into an assertion. With this patch, the error code
will be propagated to the caller who might then, at its own discretion and with
knowledge about the application-level logic, use it to attempt resolving the
situation gracefully or terminate the process after all.
The behavior for other (normal) mutex types is not changed, they will
continue to actually deadlock the calling thread. A new test is added
to assert the expected behavior of all mutex types.
Since POSIX doesn't seem to mandate any particular behavior for this situation,
and no existing code should have a dependency of running into an assertion,
changing this behavior to what is presumably the most useful one seems to be
justified.
The previous (design) discussion can be seen here:
https://sourceware.org/pipermail/libc-alpha/2025-December/173431.html
Signed-off-by: Moritz Klammler <moritz.klammler.ext@siemens.com>
---
nptl/Makefile | 1 +
nptl/pthread_mutex_lock.c | 15 +++-
nptl/tst-deadlk.c | 146 ++++++++++++++++++++++++++++++++++++++
3 files changed, 159 insertions(+), 3 deletions(-)
create mode 100644 nptl/tst-deadlk.c
@@ -283,6 +283,7 @@ tests = \
tst-cleanup5 \
tst-cond26 \
tst-context1 \
+ tst-deadlk \
tst-default-attr \
tst-dlsym1 \
tst-exec4 \
@@ -418,9 +418,18 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
NULL, private);
if (e == ESRCH || e == EDEADLK)
{
- assert (e != EDEADLK
- || (kind != PTHREAD_MUTEX_ERRORCHECK_NP
- && kind != PTHREAD_MUTEX_RECURSIVE_NP));
+ if (e == EDEADLK
+ && (kind == PTHREAD_MUTEX_ERRORCHECK_NP
+ || kind == PTHREAD_MUTEX_RECURSIVE_NP))
+ {
+ /* FUTEX_LOCK_PI may return EDEADLK due to cross‑thread
+ deadlock detection, beyond the same‑thread recursive
+ check above. Pass this error through for these two
+ mutex types; otherwise, intentionally deadlock for
+ normal mutexes. */
+ return e;
+ }
+
/* ESRCH can happen only for non-robust PI mutexes where
the owner of the lock died. */
assert (e != ESRCH || !robust);
new file mode 100644
@@ -0,0 +1,146 @@
+/* Test for pthread_mutex_lock deadlock behavior.
+ Copyright (C) 2026 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <array_length.h>
+#include <errno.h>
+#include <stdint.h>
+#include <stdio.h>
+
+#include <support/capture_subprocess.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/test-driver.h>
+#include <support/xthread.h>
+
+#define ASSUME_DEADLOCK_AFTER_SECONDS 3
+
+struct which_mutex
+{
+ int type;
+ bool prio_inherit;
+ bool robust;
+};
+
+struct task_context
+{
+ pthread_mutex_t *first, *second;
+ pthread_barrier_t *barrier;
+};
+
+static bool
+should_detect_deadlock (const struct which_mutex *const that)
+{
+ return (that->prio_inherit)
+ && (that->type == PTHREAD_MUTEX_ERRORCHECK
+ || that->type == PTHREAD_MUTEX_RECURSIVE);
+}
+
+static void *
+thread_function (void *const arg)
+{
+ const struct task_context *ctx = arg;
+ intptr_t ret = 0;
+ xpthread_mutex_lock (ctx->first);
+ xpthread_barrier_wait (ctx->barrier);
+ ret = pthread_mutex_lock (ctx->second);
+ xpthread_mutex_unlock (ctx->first);
+ if (ret == 0)
+ xpthread_mutex_unlock (ctx->second);
+ return (void *) ret;
+}
+
+static void
+prepare_mutex (pthread_mutex_t *const mutex,
+ const struct which_mutex *const that)
+{
+ pthread_mutexattr_t attr;
+ xpthread_mutexattr_init (&attr);
+ xpthread_mutexattr_settype (&attr, that->type);
+ if (that->robust)
+ xpthread_mutexattr_setrobust (&attr, PTHREAD_MUTEX_ROBUST);
+ if (that->prio_inherit)
+ xpthread_mutexattr_setprotocol (&attr, PTHREAD_PRIO_INHERIT);
+ xpthread_mutex_init (mutex, &attr);
+ xpthread_mutexattr_destroy (&attr);
+}
+
+static void
+do_test_single (void *const ctx)
+{
+ pthread_mutex_t m1, m2;
+ pthread_barrier_t barrier;
+ const struct which_mutex *const that = ctx;
+ const bool graceful = should_detect_deadlock (that);
+ struct task_context ctx1
+ = { .first = &m1, .second = &m2, .barrier = &barrier };
+ struct task_context ctx2
+ = { .first = &m2, .second = &m1, .barrier = &barrier };
+ xpthread_barrier_init (&barrier, NULL, 2);
+ prepare_mutex (&m1, that);
+ prepare_mutex (&m2, that);
+ const pthread_t t1 = xpthread_create (NULL, thread_function, &ctx1);
+ const pthread_t t2 = xpthread_create (NULL, thread_function, &ctx2);
+ if (!graceful)
+ delayed_exit (ASSUME_DEADLOCK_AFTER_SECONDS);
+ const int ret1 = (intptr_t) xpthread_join (t1);
+ const int ret2 = (intptr_t) xpthread_join (t2);
+ xpthread_mutex_destroy (&m1);
+ xpthread_mutex_destroy (&m2);
+ xpthread_barrier_destroy (&barrier);
+ TEST_VERIFY (graceful);
+ TEST_VERIFY (ret1 == 0 || ret1 == EDEADLK);
+ TEST_VERIFY (ret2 == 0 || ret2 == EDEADLK);
+ TEST_VERIFY (ret1 == EDEADLK || ret2 == EDEADLK);
+}
+
+static int
+do_test (void)
+{
+ const int mutex_types[] = {
+ PTHREAD_MUTEX_TIMED_NP,
+ PTHREAD_MUTEX_RECURSIVE_NP,
+ PTHREAD_MUTEX_ERRORCHECK_NP,
+ PTHREAD_MUTEX_ADAPTIVE_NP,
+ };
+ for (size_t i = 0; i < array_length (mutex_types); ++i)
+ {
+ for (int pi = 0; pi < 2; ++pi)
+ {
+ for (int rb = 0; rb < 2; ++rb)
+ {
+ struct which_mutex that = {
+ .type = mutex_types[i],
+ .prio_inherit = pi,
+ .robust = rb,
+ };
+ const char *const description
+ = xasprintf ("type = %d, prio_inherit = %d, robust = %d",
+ that.type, that.robust, that.prio_inherit);
+ struct support_capture_subprocess capture
+ = support_capture_subprocess (do_test_single, &that);
+ support_capture_subprocess_check (&capture, description, 0,
+ sc_allow_none);
+ support_capture_subprocess_free (&capture);
+ }
+ }
+ }
+ return 0;
+}
+
+#define TIMEOUT 60
+#include <support/test-driver.c>