Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] kernel: locking: rtmutex: Fix a possible
 sleep-in-atomic-context bug in rt_mutex_handle_deadlock()
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     peterz@infradead.org, mingo@redhat.com, will.deacon@arm.com,
        linux-kernel@vger.kernel.org
References: <20180811023524.13845-1-baijiaju1990@gmail.com>
 <20180811024456.ykccnkbdrac4nbem@home.goodmis.org>
From:   Jia-Ju Bai <baijiaju1990@gmail.com>
Message-ID: <edcc6397-6cf9-a629-56bd-8f3bd779d1bd@gmail.com>
Date:   Sat, 11 Aug 2018 10:50:33 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.0
MIME-Version: 1.0
In-Reply-To: <20180811024456.ykccnkbdrac4nbem@home.goodmis.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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