linux-kernel - Re: [PATCH] semaphore: Add might_sleep() to down

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1a5b0f50-b071-2d1c-5277-b6d7f652c257@huawei.com>
Date:   Mon, 9 Aug 2021 11:51:09 +0800
From:   Xiaoming Ni <nixiaoming@...wei.com>
To:     Waiman Long <llong@...hat.com>, <linux-kernel@...r.kernel.org>,
        <peterz@...radead.org>, <mingo@...hat.com>, <will@...nel.org>,
        <boqun.feng@...il.com>
CC:     <wangle6@...wei.com>, <xiaoqian9@...wei.com>, <shaolexi@...wei.com>
Subject: Re: [PATCH] semaphore: Add might_sleep() to down_*() family

On 2021/8/9 11:01, Waiman Long wrote:
> On 8/8/21 10:12 PM, Xiaoming Ni wrote:
>> Semaphore is sleeping lock. Add might_sleep() to down*() family
>> (with exception of down_trylock()) to detect atomic context sleep.
>>
>> Previously discussed with Peter Zijlstra, see link:
>>   
>> https://lore.kernel.org/lkml/20210806082320.GD22037@worktop.programming.kicks-ass.net 
>>
>>
>> Signed-off-by: Xiaoming Ni <nixiaoming@...wei.com>
>> ---
>>   kernel/locking/semaphore.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c
>> index 9aa855a96c4a..9ee381e4d2a4 100644
>> --- a/kernel/locking/semaphore.c
>> +++ b/kernel/locking/semaphore.c
>> @@ -54,6 +54,7 @@ void down(struct semaphore *sem)
>>   {
>>       unsigned long flags;
>> +    might_sleep();
>>       raw_spin_lock_irqsave(&sem->lock, flags);
>>       if (likely(sem->count > 0))
>>           sem->count--;
>> @@ -77,6 +78,7 @@ int down_interruptible(struct semaphore *sem)
>>       unsigned long flags;
>>       int result = 0;
>> +    might_sleep();
>>       raw_spin_lock_irqsave(&sem->lock, flags);
>>       if (likely(sem->count > 0))
>>           sem->count--;
>> @@ -103,6 +105,7 @@ int down_killable(struct semaphore *sem)
>>       unsigned long flags;
>>       int result = 0;
>> +    might_sleep();
>>       raw_spin_lock_irqsave(&sem->lock, flags);
>>       if (likely(sem->count > 0))
>>           sem->count--;
>> @@ -157,6 +160,7 @@ int down_timeout(struct semaphore *sem, long timeout)
>>       unsigned long flags;
>>       int result = 0;
>> +    might_sleep();
>>       raw_spin_lock_irqsave(&sem->lock, flags);
>>       if (likely(sem->count > 0))
>>           sem->count--;
> 
> I think it is simpler to just put a "might_sleep()" in __down_common() 
> which is the function where sleep can actually happen.
> 

If the actual atomic context hibernation occurs, the corresponding alarm 
log is generated in __schedule_bug().
	__schedule()
		--> schedule_debug()
			--> __schedule_bug()

However, "might_sleep()" indicates the possibility of sleep, so that 
code writers can identify and fix the problem as soon as possible, but 
does not trigger atomic context sleep.

Is it better to put "might_sleep()" in each down API entry than 
__down_common() to help identify potential code problems?

Thanks
Xiaoming Ni