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. */
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.
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
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)
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
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)
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
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!
linux-stable-mirror@lists.linaro.org
-
Christoph Hellwig -
David Laight -
Peter Zijlstra -
Waiman Long