Stable Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN
@ 2020-01-14 19:03 Waiman Long
  2020-01-15  6:50 ` Christoph Hellwig
  0 siblings, 1 reply; 8+ messages in thread
From: Waiman Long @ 2020-01-14 19:03 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Will Deacon
  Cc: linux-kernel, Christoph Hellwig, stable, Waiman Long

The commit 91d2a812dfb9 ("locking/rwsem: Make handoff writer
optimistically spin on owner") will allow a recently woken up waiting
writer to spin on the owner. Unfortunately, if the owner happens to be
RWSEM_OWNER_UNKNOWN, the code will incorrectly spin on it leading to a
kernel crash. This is fixed by passing the proper non-spinnable bits
to rwsem_spin_on_owner() so that RWSEM_OWNER_UNKNOWN will be treated
as a non-spinnable target.

Fixes: 91d2a812dfb9 ("locking/rwsem: Make handoff writer optimistically spin on owner")

Reported-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Waiman Long <longman@redhat.com>
---
 kernel/locking/rwsem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 44e68761f432..1dd3d53f43c3 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -1227,7 +1227,7 @@ rwsem_down_write_slowpath(struct rw_semaphore *sem, int state)
 		 * without sleeping.
 		 */
 		if ((wstate == WRITER_HANDOFF) &&
-		    (rwsem_spin_on_owner(sem, 0) == OWNER_NULL))
+		    rwsem_spin_on_owner(sem, RWSEM_NONSPINNABLE) == OWNER_NULL)
 			goto trylock_again;
 
 		/* Block until there are no active lockers. */
-- 
2.18.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN
  2020-01-14 19:03 [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN Waiman Long
@ 2020-01-15  6:50 ` Christoph Hellwig
  2020-01-15 14:27   ` Waiman Long
                     ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Christoph Hellwig @ 2020-01-15  6:50 UTC (permalink / raw)
  To: Waiman Long
  Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, linux-kernel,
	Christoph Hellwig, stable

On Tue, Jan 14, 2020 at 02:03:03PM -0500, Waiman Long wrote:
> The commit 91d2a812dfb9 ("locking/rwsem: Make handoff writer
> optimistically spin on owner") will allow a recently woken up waiting
> writer to spin on the owner. Unfortunately, if the owner happens to be
> RWSEM_OWNER_UNKNOWN, the code will incorrectly spin on it leading to a
> kernel crash. This is fixed by passing the proper non-spinnable bits
> to rwsem_spin_on_owner() so that RWSEM_OWNER_UNKNOWN will be treated
> as a non-spinnable target.
> 
> Fixes: 91d2a812dfb9 ("locking/rwsem: Make handoff writer optimistically spin on owner")
> 
> Reported-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Waiman Long <longman@redhat.com>

This survives all the tests that showed the problems with the original
code:

Tested-by: Christoph Hellwig <hch@lst.de>

>  		if ((wstate == WRITER_HANDOFF) &&
> -		    (rwsem_spin_on_owner(sem, 0) == OWNER_NULL))
> +		    rwsem_spin_on_owner(sem, RWSEM_NONSPINNABLE) == OWNER_NULL)

Nit: the inner braces in the first half of the conditional aren't required
either.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN
  2020-01-15  6:50 ` Christoph Hellwig
@ 2020-01-15 14:27   ` Waiman Long
  2020-01-15 15:16     ` David Laight
  2020-01-15 15:28   ` Waiman Long
  2020-01-15 15:37   ` Peter Zijlstra
  2 siblings, 1 reply; 8+ messages in thread
From: Waiman Long @ 2020-01-15 14:27 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, linux-kernel, stable

On 1/15/20 1:50 AM, Christoph Hellwig wrote:
> On Tue, Jan 14, 2020 at 02:03:03PM -0500, Waiman Long wrote:
>> The commit 91d2a812dfb9 ("locking/rwsem: Make handoff writer
>> optimistically spin on owner") will allow a recently woken up waiting
>> writer to spin on the owner. Unfortunately, if the owner happens to be
>> RWSEM_OWNER_UNKNOWN, the code will incorrectly spin on it leading to a
>> kernel crash. This is fixed by passing the proper non-spinnable bits
>> to rwsem_spin_on_owner() so that RWSEM_OWNER_UNKNOWN will be treated
>> as a non-spinnable target.
>>
>> Fixes: 91d2a812dfb9 ("locking/rwsem: Make handoff writer optimistically spin on owner")
>>
>> Reported-by: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Waiman Long <longman@redhat.com>
> This survives all the tests that showed the problems with the original
> code:
>
> Tested-by: Christoph Hellwig <hch@lst.de>
>
>>  		if ((wstate == WRITER_HANDOFF) &&
>> -		    (rwsem_spin_on_owner(sem, 0) == OWNER_NULL))
>> +		    rwsem_spin_on_owner(sem, RWSEM_NONSPINNABLE) == OWNER_NULL)
> Nit: the inner braces in the first half of the conditional aren't required
> either.

I typically over-parenthesize the code to make it easier to read as we
don't need to think too much about operator precedence to see if it is
doing the right thing. I remove the 2nd parentheses to avoid breaking
the 80-colnum limit.

Cheers,
Longman


^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN
  2020-01-15 14:27   ` Waiman Long
@ 2020-01-15 15:16     ` David Laight
  2020-01-15 15:47       ` Waiman Long
  0 siblings, 1 reply; 8+ messages in thread
From: David Laight @ 2020-01-15 15:16 UTC (permalink / raw)
  To: Waiman Long, Christoph Hellwig
  Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, linux-kernel, stable

From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf Of Waiman Long
> Sent: 15 January 2020 14:27
...
> >>  		if ((wstate == WRITER_HANDOFF) &&
> >> -		    (rwsem_spin_on_owner(sem, 0) == OWNER_NULL))
> >> +		    rwsem_spin_on_owner(sem, RWSEM_NONSPINNABLE) == OWNER_NULL)
> > Nit: the inner braces in the first half of the conditional aren't required
> > either.
> 
> I typically over-parenthesize the code to make it easier to read as we
> don't need to think too much about operator precedence to see if it is
> doing the right thing.

The problem is it actually makes it harder to read.
It is difficult for the 'mark 1 eyeball' to follow lots of sets of brackets.
Since == (etc) are the lowest priority operators (apart from ?:) they
never need ().

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN
  2020-01-15  6:50 ` Christoph Hellwig
  2020-01-15 14:27   ` Waiman Long
@ 2020-01-15 15:28   ` Waiman Long
  2020-01-15 15:37   ` Peter Zijlstra
  2 siblings, 0 replies; 8+ messages in thread
From: Waiman Long @ 2020-01-15 15:28 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, linux-kernel, stable

On 1/15/20 1:50 AM, Christoph Hellwig wrote:
> On Tue, Jan 14, 2020 at 02:03:03PM -0500, Waiman Long wrote:
>> The commit 91d2a812dfb9 ("locking/rwsem: Make handoff writer
>> optimistically spin on owner") will allow a recently woken up waiting
>> writer to spin on the owner. Unfortunately, if the owner happens to be
>> RWSEM_OWNER_UNKNOWN, the code will incorrectly spin on it leading to a
>> kernel crash. This is fixed by passing the proper non-spinnable bits
>> to rwsem_spin_on_owner() so that RWSEM_OWNER_UNKNOWN will be treated
>> as a non-spinnable target.
>>
>> Fixes: 91d2a812dfb9 ("locking/rwsem: Make handoff writer optimistically spin on owner")
>>
>> Reported-by: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Waiman Long <longman@redhat.com>
> This survives all the tests that showed the problems with the original
> code:
>
> Tested-by: Christoph Hellwig <hch@lst.de>
>
Thanks for the testing.
>>  		if ((wstate == WRITER_HANDOFF) &&
>> -		    (rwsem_spin_on_owner(sem, 0) == OWNER_NULL))
>> +		    rwsem_spin_on_owner(sem, RWSEM_NONSPINNABLE) == OWNER_NULL)
> Nit: the inner braces in the first half of the conditional aren't required
> either.

Yes, it is inconsistent and so is not good. I will post a v2 patch to
fix that.

Cheers,
Longman


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN
  2020-01-15  6:50 ` Christoph Hellwig
  2020-01-15 14:27   ` Waiman Long
  2020-01-15 15:28   ` Waiman Long
@ 2020-01-15 15:37   ` Peter Zijlstra
  2 siblings, 0 replies; 8+ messages in thread
From: Peter Zijlstra @ 2020-01-15 15:37 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Waiman Long, Ingo Molnar, Will Deacon, linux-kernel, stable

On Wed, Jan 15, 2020 at 07:50:55AM +0100, Christoph Hellwig wrote:
> On Tue, Jan 14, 2020 at 02:03:03PM -0500, Waiman Long wrote:
> > The commit 91d2a812dfb9 ("locking/rwsem: Make handoff writer
> > optimistically spin on owner") will allow a recently woken up waiting
> > writer to spin on the owner. Unfortunately, if the owner happens to be
> > RWSEM_OWNER_UNKNOWN, the code will incorrectly spin on it leading to a
> > kernel crash. This is fixed by passing the proper non-spinnable bits
> > to rwsem_spin_on_owner() so that RWSEM_OWNER_UNKNOWN will be treated
> > as a non-spinnable target.
> > 
> > Fixes: 91d2a812dfb9 ("locking/rwsem: Make handoff writer optimistically spin on owner")
> > 
> > Reported-by: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Waiman Long <longman@redhat.com>
> 
> This survives all the tests that showed the problems with the original
> code:
> 
> Tested-by: Christoph Hellwig <hch@lst.de>

Thanks!

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN
  2020-01-15 15:16     ` David Laight
@ 2020-01-15 15:47       ` Waiman Long
  2020-01-15 16:01         ` David Laight
  0 siblings, 1 reply; 8+ messages in thread
From: Waiman Long @ 2020-01-15 15:47 UTC (permalink / raw)
  To: David Laight, Christoph Hellwig
  Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, linux-kernel, stable

On 1/15/20 10:16 AM, David Laight wrote:
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf Of Waiman Long
>> Sent: 15 January 2020 14:27
> ...
>>>>  		if ((wstate == WRITER_HANDOFF) &&
>>>> -		    (rwsem_spin_on_owner(sem, 0) == OWNER_NULL))
>>>> +		    rwsem_spin_on_owner(sem, RWSEM_NONSPINNABLE) == OWNER_NULL)
>>> Nit: the inner braces in the first half of the conditional aren't required
>>> either.
>> I typically over-parenthesize the code to make it easier to read as we
>> don't need to think too much about operator precedence to see if it is
>> doing the right thing.
> The problem is it actually makes it harder to read.
> It is difficult for the 'mark 1 eyeball' to follow lots of sets of brackets.
> Since == (etc) are the lowest priority operators (apart from ?:) they
> never need ().
>
> 	David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
It depends. I find it hard to read an expression with "&" and "&&"
without parentheses. Anyway, I will admit that the above code is
inconsistent in term of how parentheses are used. So I will change that.

Cheers,
Longman


^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN
  2020-01-15 15:47       ` Waiman Long
@ 2020-01-15 16:01         ` David Laight
  0 siblings, 0 replies; 8+ messages in thread
From: David Laight @ 2020-01-15 16:01 UTC (permalink / raw)
  To: Waiman Long, Christoph Hellwig
  Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, linux-kernel, stable

From: Waiman Long
> Sent: 15 January 2020 15:48
...
> It depends. I find it hard to read an expression with "&" and "&&"
> without parentheses. Anyway, I will admit that the above code is
> inconsistent in term of how parentheses are used. So I will change that.

Conditionals containing fragments like (a == b && c == d && ...)
are much easier to read without any extra ().

The only problem with && is that when K&R added it to C they didn't
change the priority of & to be higher than == (where it should be).
At that time they could have changed all the existing code...
Modern compilers do warn about (a == b & c).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-14 19:03 [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN Waiman Long
2020-01-15  6:50 ` Christoph Hellwig
2020-01-15 14:27   ` Waiman Long
2020-01-15 15:16     ` David Laight
2020-01-15 15:47       ` Waiman Long
2020-01-15 16:01         ` David Laight
2020-01-15 15:28   ` Waiman Long
2020-01-15 15:37   ` Peter Zijlstra

Stable Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ https://lore.kernel.org/stable \
		stable@vger.kernel.org
	public-inbox-index stable

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.stable


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git