Skip to content

Commit

Permalink
locking/osq: Fix ordering of node initialisation in osq_lock
Browse files Browse the repository at this point in the history
The Cavium guys reported a soft lockup on their arm64 machine, caused by
commit c55a6ff ("locking/osq: Relax atomic semantics"):

    mutex_optimistic_spin+0x9c/0x1d0
    __mutex_lock_slowpath+0x44/0x158
    mutex_lock+0x54/0x58
    kernfs_iop_permission+0x38/0x70
    __inode_permission+0x88/0xd8
    inode_permission+0x30/0x6c
    link_path_walk+0x68/0x4d4
    path_openat+0xb4/0x2bc
    do_filp_open+0x74/0xd0
    do_sys_open+0x14c/0x228
    SyS_openat+0x3c/0x48
    el0_svc_naked+0x24/0x28

This is because in osq_lock we initialise the node for the current CPU:

    node->locked = 0;
    node->next = NULL;
    node->cpu = curr;

and then publish the current CPU in the lock tail:

    old = atomic_xchg_acquire(&lock->tail, curr);

Once the update to lock->tail is visible to another CPU, the node is
then live and can be both read and updated by concurrent lockers.

Unfortunately, the ACQUIRE semantics of the xchg operation mean that
there is no guarantee the contents of the node will be visible before
lock tail is updated.  This can lead to lock corruption when, for
example, a concurrent locker races to set the next field.

Fixes: c55a6ff ("locking/osq: Relax atomic semantics"):
Reported-by: David Daney <[email protected]>
Reported-by: Andrew Pinski <[email protected]>
Tested-by: Andrew Pinski <[email protected]>
Acked-by: Davidlohr Bueso <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Linus Torvalds <[email protected]>
  • Loading branch information
wildea01 authored and torvalds committed Dec 17, 2015
1 parent d7637d0 commit b4b29f9
Showing 1 changed file with 5 additions and 3 deletions.
8 changes: 5 additions & 3 deletions kernel/locking/osq_lock.c
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,12 @@ bool osq_lock(struct optimistic_spin_queue *lock)
node->cpu = curr;

/*
* ACQUIRE semantics, pairs with corresponding RELEASE
* in unlock() uncontended, or fastpath.
* We need both ACQUIRE (pairs with corresponding RELEASE in
* unlock() uncontended, or fastpath) and RELEASE (to publish
* the node fields we just initialised) semantics when updating
* the lock tail.
*/
old = atomic_xchg_acquire(&lock->tail, curr);
old = atomic_xchg(&lock->tail, curr);
if (old == OSQ_UNLOCKED_VAL)
return true;

Expand Down

0 comments on commit b4b29f9

Please sign in to comment.