Re: [PATCH] locking/mutex: Add debug code to help catching violation of mutex lifetime rule

[Waiman Long <llong@redhat.com>]

On 7/11/25 8:42 PM, Waiman Long wrote:
>
> On 7/11/25 7:28 PM, Boqun Feng wrote:
>> On Fri, Jul 11, 2025 at 03:30:05PM -0700, Linus Torvalds wrote:
>>> On Fri, 11 Jul 2025 at 15:20, Boqun Feng <boqun.feng@gmail.com> wrote:
>>>> Meta question: are we able to construct a case that shows this can 
>>>> help
>>>> detect the issue?
>>> Well, the thing that triggered this was hopefully fixed by
>>> 8c2e52ebbe88 ("eventpoll: don't decrement ep refcount while still
>>> holding the ep mutex"), but I think Jann figured that one out by code
>>> inspection.
>>>
>>> I doubt it can be triggered in real life without something like
>>> Waiman's patch, but *with* Waiman's patch, and commit 8c2e52ebbe88
>>> reverted (and obviously with CONFIG_KASAN and CONFIG_DEBUG_MUTEXES
>>> enabled), doing lots of concurrent epoll closes would hopefully then
>>> trigger the warning.
>>>
>>> Of course, to then find *other* potential bugs would be the whole
>>> point, and some of these kinds of bugs are definitely of the kind
>>> where the race condition doesn't actually trigger in any real load,
>>> because it's unlikely that real loads end up doing that kind of
>>> "release all these objects concurrently".
>>>
>>> But it might be interesting to try that "can you even recreate the bug
>>> fixed by 8c2e52ebbe88" with this. Because if that one *known* bug
>>> can't be found by this, then it's obviously unlikely to help find
>>> others.
>>>
>> Yeah, I guess I asked the question because there is no clear link from
>> the bug scenario to an extra context switch, that is, even if the
>> context switch didn't happen, the bug would trigger if
>> __mutex_unlock_slowpath() took too long after giving the ownership to
>> someone else. So my instinct was: would cond_resched() be slow enough
>> ;-)
>>
>> But I agree it's a trivel thing to do, and I think another thing we can
>> do is adding a kasan_check_byte(lock) at the end of
>> __mutex_unlock_slowpath(), because conceptually the mutex should be
>> valid throughout the whole __mutex_unlock_slowpath() function, i.e.
>>
>>     void __mutex_unlock_slowpath(...)
>>     {
>>         ...
>>         raw_spin_unlock_irqrestore_wake(&lock->wait_lock, flags, 
>> &wake_q);
>>         // <- conceptually "lock" should still be valid here.
>>         // so if anyone free the memory of the mutex, it's going
>>         // to be a problem.
>>         kasan_check_byte(lock);
>>     }
>>
>> I think this may also give us a good chance of finding more bugs, one of
>> the reasons is that raw_spin_unlock_irqrestore_wake() has a
>> preempt_enable() at last, which may trigger a context switch.
>>
>> Regards,
>> Boqun
>
> I think this is a good idea. We should extend that to add the check in 
> rwsem as well. Will a post a patch to do that.

Digging into it some more, I think adding kasan_check_byte() may not be 
necessary. If KASAN is enabled, it will instrument the locking code 
including __mutex_unlock_slowpath(). I checked the generated assembly 
code, it has 2 __kasan_check_read() and 4 __kasan_check_write() calls. 
Adding an extra kasan_check_byte() can be redundant.

Cheers,
Longman