Messages in this thread | | | Date | Wed, 10 Nov 2021 22:38:54 +0100 | From | Peter Zijlstra <> | Subject | Re: [BUG]locking/rwsem: only clean RWSEM_FLAG_HANDOFF when already set |
| |
On Sun, Nov 07, 2021 at 02:52:36PM -0500, Waiman Long wrote: > > > > I did have a tentative patch to address this issue which is somewhat > > similar to your approach. However, I would like to further investigate > > the exact mechanics of the race condition to make sure that I won't miss > > a latent bug somewhere else in the rwsem code. > > I still couldn't figure how this race condition can happen. However, I do > discover that it is possible to leave rwsem with no waiter but handoff bit > set if we kill or interrupt all the waiters in the wait queue. I have just > sent out a patch to address that concern, but it should be able to handle > this race condition as well if it really happens.
The comment above RWSEM_WRITER_LOCKED seems wrong/out-dated in that there's a 4th place that modifies the HANDOFF bit namely rwsem_down_read_slowpath() in the out_nolock: case.
Now the thing I'm most worried about is that rwsem_down_write_slowpath() modifies the HANDOFF bit depending on wstate, and wstate itself it not determined under the same ->wait_lock section, so there could be a race there.
Another thing is that once wstate==HANDOFF, we rely on spin_on_owner() to return OWNER_NULL such that it goes to trylock_again, however if it returns anything else then we're at signal_pending_state() and the observed race can happen.
Now, spin_on_owner() *can* in fact return something else, consider need_resched() being set for instance.
Combined I think the observed race is valid.
Now before we go make things more complicated, I think we should see if we can make things simpler. Also I think perhaps the HANDOFF name here is a misnomer.
I agree that using _andnot() will fix this issue; I also agree with folding it with the existing _andnot() already there. But let me stare a little more at this code, something isn't making sense...
| |