Re: [PATCH] kernel: locking: rtmutex: Fix a possible sleep-in-atomic-context bug in rt_mutex_handle_deadlock()

[Jia-Ju Bai <baijiaju1990@gmail.com>]

On 2018/8/11 10:44, Steven Rostedt wrote:
> On Sat, Aug 11, 2018 at 10:35:24AM +0800, Jia-Ju Bai wrote:
>> The driver may sleep with holding a spinlock.
>>
>> The function call paths (from bottom to top) in Linux-4.16 are:
>>
>> [FUNC] schedule
>> kernel/locking/rtmutex.c, 1223:
>> 	schedule in rt_mutex_handle_deadlock
>> kernel/locking/rtmutex.c, 1273:
>> 	rt_mutex_handle_deadlock in rt_mutex_slowlock
>> kernel/locking/rtmutex.c, 1249:
>> 	_raw_spin_lock_irqsave in rt_mutex_slowlock
>>
>> To fix the bug, the spinlock is released before schedule() and then acquired again.
>> This is found by my static analysis tool (DSAC).
>>
>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
>> ---
>>   kernel/locking/rtmutex.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
>> index 2823d4163a37..af03e162f812 100644
>> --- a/kernel/locking/rtmutex.c
>> +++ b/kernel/locking/rtmutex.c
>> @@ -1205,7 +1205,7 @@ __rt_mutex_slowlock(struct rt_mutex *lock, int state,
>>   }
>>   
>>   static void rt_mutex_handle_deadlock(int res, int detect_deadlock,
>> -				     struct rt_mutex_waiter *w)
>> +				     struct rt_mutex_waiter *w, struct rt_mutex *lock)
>>   {
>>   	/*
>>   	 * If the result is not -EDEADLOCK or the caller requested
>> @@ -1219,8 +1219,10 @@ static void rt_mutex_handle_deadlock(int res, int detect_deadlock,
>>   	 */
>>   	rt_mutex_print_deadlock(w);
>>   	while (1) {
>> +		raw_spin_unlock_irq(&lock->wait_lock);
>>   		set_current_state(TASK_INTERRUPTIBLE);
>>   		schedule();
>> +		raw_spin_lock_irq(&lock->wait_lock);
>>   	}
> If you look at the code you will notice that it stops the task and never lets
> it continue. Ever.
>
> If we hit this path, it means we are in a deadlock scenario and will not make
> any forward progress.
>
> If anything, it should simply be:
>
> 	rt_mutex_print_deadlock(w);
> +	/* We're not going anywhere, release the wait_lock */
> +	raw_spin_unlock_irq(&lock->wait_lock);
> 	while (1) {
> 		set_current_state(TASK_INTERRUPTIBLE);
> 		schedule();
> 	}

Thanks for your reply :)

Okay, I will send a V2 patch.


Best wishes,
Jia-Ju Bai