nptl: futex_lock_pi deadlock detection provides valuable information but it is turned into a rather cryptic assertion failure

  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
  

On 08/04/26 17:07, Moritz KLAMMLER (FERCHAU) wrote:
> 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.
Carlos, you brought to my attention that glibc current Error Handling
guideline [1] states:

  "If it's user code invoking undefined behavior, then it should fail 
   early and catastrophically so that developers don't get the false 
   impression that their code is OK when it happens not to break the 
   use cases they test adequately."

However, this patch changes the ERRORCHECK and RECURSIVE pthread types 
and, although POSIX only states a *shall fail* for self-deadlock, it does 
permit an error return for multi-mutex deadlock in the *may fail*.

This change still has the potential to break code that does not handle
EDEADLK in such cases; however this currently aborts the process anyway.
So different than I has said (sorry if I forgot all the details on Monday's
call), this is *not* UB and I think that issuing a fatal error like
futex_fatal_error is not the course of action here. 

The assert really has not place since kernel does allow to have fast deadlock
check and POSIX allows it.

[1] https://sourceware.org/glibc/wiki/Style_and_Conventions#Error_Handling
[2] https://pubs.opengroup.org/onlinepubs/9799919799/functions/pthread_mutex_lock.html
  

On 4/14/26 4:14 PM, Adhemerval Zanella Netto wrote:
> 
> 
> On 08/04/26 17:07, Moritz KLAMMLER (FERCHAU) wrote:
>> 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.
> Carlos, you brought to my attention that glibc current Error Handling
> guideline [1] states:
> 
>    "If it's user code invoking undefined behavior, then it should fail
>     early and catastrophically so that developers don't get the false
>     impression that their code is OK when it happens not to break the
>     use cases they test adequately."
> 
> However, this patch changes the ERRORCHECK and RECURSIVE pthread types
> and, although POSIX only states a *shall fail* for self-deadlock, it does
> permit an error return for multi-mutex deadlock in the *may fail*.
> 
> This change still has the potential to break code that does not handle
> EDEADLK in such cases; however this currently aborts the process anyway.
> So different than I has said (sorry if I forgot all the details on Monday's
> call), this is *not* UB and I think that issuing a fatal error like
> futex_fatal_error is not the course of action here.
> 
> The assert really has not place since kernel does allow to have fast deadlock
> check and POSIX allows it.

If we aren't triggering undefined behaviour then I think it's perfectly
acceptable to return a "may fail" error to the caller.
  

* Carlos O'Donell:

> If we aren't triggering undefined behaviour then I think it's perfectly
> acceptable to return a "may fail" error to the caller.

Doing that triggers undefined behavior in this case if the caller
ignores the EDEADLK right, no?

Thanks,
Florian
  

On 15/04/26 04:24, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> If we aren't triggering undefined behaviour then I think it's perfectly
>> acceptable to return a "may fail" error to the caller.
> 
> Doing that triggers undefined behavior in this case if the caller
> ignores the EDEADLK right, no?

The 'may fail' for multi-mutex deadlock is defined even for POSIX 2001 [1],
so I am not sure if we can consider this UB.

[1] https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_mutex_lock.html
  

* Adhemerval Zanella Netto:

> On 15/04/26 04:24, Florian Weimer wrote:
>> * Carlos O'Donell:
>> 
>>> If we aren't triggering undefined behaviour then I think it's perfectly
>>> acceptable to return a "may fail" error to the caller.
>> 
>> Doing that triggers undefined behavior in this case if the caller
>> ignores the EDEADLK right, no?
>
> The 'may fail' for multi-mutex deadlock is defined even for POSIX 2001 [1],
> so I am not sure if we can consider this UB.
>
> [1] https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_mutex_lock.html

It's likely to end up in UB because the lock is not taken, the caller
doesn't realize this, and enters the code for the critical section
without any protection against data races.

Thanks,
Florian
  

On 15/04/26 11:02, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 15/04/26 04:24, Florian Weimer wrote:
>>> * Carlos O'Donell:
>>>
>>>> If we aren't triggering undefined behaviour then I think it's perfectly
>>>> acceptable to return a "may fail" error to the caller.
>>>
>>> Doing that triggers undefined behavior in this case if the caller
>>> ignores the EDEADLK right, no?
>>
>> The 'may fail' for multi-mutex deadlock is defined even for POSIX 2001 [1],
>> so I am not sure if we can consider this UB.
>>
>> [1] https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_mutex_lock.html
> 
> It's likely to end up in UB because the lock is not taken, the caller
> doesn't realize this, and enters the code for the critical section
> without any protection against data races.

But currently any program that issues this scenario will abort anyway,
and it also depends whether glibc is built with assert enabled.

It also follow the error-checking semantic that it *should* return
an error, so I think it is unlikely to cause a regression because
I would expect error checking is a standard when this mutex type is
used.

The recursive check *might* trigger some issues, but even then for 
non-pi mutexes this is a deadlock anyway.  Maybe just change for
error-checking and let robust fall the path of __futex_abstimed_wait64.
  

> But currently any program that issues this scenario will abort anyway,
> and it also depends whether glibc is built with assert enabled.
>
> It also follow the error-checking semantic that it *should* return
> an error, so I think it is unlikely to cause a regression because
> I would expect error checking is a standard when this mutex type is
> used.
>
> The recursive check *might* trigger some issues, but even then for
> non-pi mutexes this is a deadlock anyway.  Maybe just change for
> error-checking and let robust fall the path of __futex_abstimed_wait64.

Should I update the patch accordingly to have only error-checking PI
mutex propagate the EDEADLK and deliberately deadlock in all other
cases?  Running into the assertion seems to be the least desirable
option to me.
  

On 20/04/26 11:25, Moritz KLAMMLER (FERCHAU) wrote:
>> But currently any program that issues this scenario will abort anyway,
>> and it also depends whether glibc is built with assert enabled.
>>
>> It also follow the error-checking semantic that it *should* return
>> an error, so I think it is unlikely to cause a regression because
>> I would expect error checking is a standard when this mutex type is
>> used.
>>
>> The recursive check *might* trigger some issues, but even then for
>> non-pi mutexes this is a deadlock anyway.  Maybe just change for
>> error-checking and let robust fall the path of __futex_abstimed_wait64.
> 
> Should I update the patch accordingly to have only error-checking PI
> mutex propagate the EDEADLK and deliberately deadlock in all other
> cases?  Running into the assertion seems to be the least desirable
> option to me.

I think this should a reasonable approach, I am not sure if Florian still
think this might not be acceptable.