Messages in this thread | | | From | Waiman Long <> | Subject | Re: [PATCH v2] locking/qrwlock: Fix ordering in queued_write_lock_slowpath | Date | Thu, 15 Apr 2021 13:45:54 -0400 |
| |
On 4/15/21 1:27 PM, Ali Saidi wrote: > While this code is executed with the wait_lock held, a reader can > acquire the lock without holding wait_lock. The writer side loops > checking the value with the atomic_cond_read_acquire(), but only truly > acquires the lock when the compare-and-exchange is completed > successfully which isn’t ordered. This exposes the window between the > acquire and the cmpxchg to an A-B-A problem which allows reads following > the lock acquisition to observe values speculatively before the write > lock is truly acquired. > > We've seen a problem in epoll where the reader does a xchg while > holding the read lock, but the writer can see a value change out from under it. > > Writer | Reader 2 > -------------------------------------------------------------------------------- > ep_scan_ready_list() | > |- write_lock_irq() | > |- queued_write_lock_slowpath() | > |- atomic_cond_read_acquire() | > | read_lock_irqsave(&ep->lock, flags); > --> (observes value before unlock)| chain_epi_lockless() > | | epi->next = xchg(&ep->ovflist, epi); > | | read_unlock_irqrestore(&ep->lock, flags); > | | > | atomic_cmpxchg_relaxed() | > |-- READ_ONCE(ep->ovflist); | > > A core can order the read of the ovflist ahead of the > atomic_cmpxchg_relaxed(). Switching the cmpxchg to use acquire semantics > addresses this issue at which point the atomic_cond_read can be switched > to use relaxed semantics. > > Fixes: b519b56e378ee ("locking/qrwlock: Use atomic_cond_read_acquire() when spinning in qrwlock") > Signed-off-by: Ali Saidi <alisaidi@amazon.com> > Cc: stable@vger.kernel.org > Acked-by: Will Deacon <will@kernel.org> > Tested-by: Steve Capper <steve.capper@arm.com> > Reviewed-by: Steve Capper <steve.capper@arm.com> > > --- > kernel/locking/qrwlock.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c > index 4786dd271b45..10770f6ac4d9 100644 > --- a/kernel/locking/qrwlock.c > +++ b/kernel/locking/qrwlock.c > @@ -73,8 +73,8 @@ void queued_write_lock_slowpath(struct qrwlock *lock) > > /* When no more readers or writers, set the locked flag */ > do { > - atomic_cond_read_acquire(&lock->cnts, VAL == _QW_WAITING); > - } while (atomic_cmpxchg_relaxed(&lock->cnts, _QW_WAITING, > + atomic_cond_read_relaxed(&lock->cnts, VAL == _QW_WAITING); > + } while (atomic_cmpxchg_acquire(&lock->cnts, _QW_WAITING, > _QW_LOCKED) != _QW_WAITING); > unlock: > arch_spin_unlock(&lock->wait_lock);
Acked-by: Waiman Long <longman@redhat.com>
| |