linux-kernel - Re: [PATCH v3] lock/semaphore: Avoid an unnecessary deadlock within up()

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20160309020037.GA2865@X58A-UD3R>
Date:	Wed, 9 Mar 2016 11:00:37 +0900
From:	Byungchul Park <byungchul.park@....com>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	willy@...ux.intel.com, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org, akinobu.mita@...il.com, jack@...e.cz,
	sergey.senozhatsky.work@...il.com, peter@...leysoftware.com
Subject: Re: [PATCH v3] lock/semaphore: Avoid an unnecessary deadlock within
 up()

On Wed, Feb 17, 2016 at 10:28:29AM +0100, Ingo Molnar wrote:
> 
> * Byungchul Park <byungchul.park@....com> wrote:
> 
> > diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c
> > index b8120ab..6634b68 100644
> > --- a/kernel/locking/semaphore.c
> > +++ b/kernel/locking/semaphore.c
> > @@ -130,13 +130,14 @@ EXPORT_SYMBOL(down_killable);
> >  int down_trylock(struct semaphore *sem)
> >  {
> >  	unsigned long flags;
> > -	int count;
> > +	int count = -1;
> >  
> > -	raw_spin_lock_irqsave(&sem->lock, flags);
> > -	count = sem->count - 1;
> > -	if (likely(count >= 0))
> > -		sem->count = count;
> > -	raw_spin_unlock_irqrestore(&sem->lock, flags);
> > +	if (raw_spin_trylock_irqsave(&sem->lock, flags)) {
> > +		count = sem->count - 1;
> > +		if (likely(count >= 0))
> > +			sem->count = count;
> > +		raw_spin_unlock_irqrestore(&sem->lock, flags);
> > +	}
> 
> I still don't really like it: two parallel trylocks will cause one of them to fail 
> - while with the previous code they would both succeed.
> 
> None of these changes are necessary with all the printk robustification 
> changes/enhancements we talked about, right?

Not only printk() but any code using a semaphore, mutex and so on, can also
cause a deadlock if wake_up_process() eventually tries to acquire the lock.
There are several ways to solve this problem.

1. ensure wake_up_process() does not try to acquire the locks.
2. ensure wake_up_process() isn't protected by a spinlock of the locks.
3. ensure any kind of trylock stuff never cause waiting and deadlock.
4. and so on..

I am not sure which one is the best. But I think 3rd one is the one since
it can be done by a generic way, even though it might decrease the success
ratio as Ingo said, but IMHO it's not a big problem since a trylock user 
only uses the trylock when it doesn't need to be cared whether it succeed
or fail.

Which one among those do you think the best approach? Please let me know,
then I will try to solve this problem by the appoach.

> 
> Thanks,
> 
> 	Ingo