Skip to content

Commit

Permalink
rxrpc: Fix locking issues in rxrpc_put_peer_locked()
Browse files Browse the repository at this point in the history
Now that rxrpc_put_local() may call kthread_stop(), it can't be called
under spinlock as it might sleep.  This can cause a problem in the peer
keepalive code in rxrpc as it tries to avoid dropping the peer_hash_lock
from the point it needs to re-add peer->keepalive_link to going round the
loop again in rxrpc_peer_keepalive_dispatch().

Fix this by just dropping the lock when we don't need it and accepting that
we'll have to take it again.  This code is only called about every 20s for
each peer, so not very often.

This allows rxrpc_put_peer_unlocked() to be removed also.

If triggered, this bug produces an oops like the following, as reproduced
by a syzbot reproducer for a different oops[1]:

BUG: sleeping function called from invalid context at kernel/sched/completion.c:101
...
RCU nest depth: 0, expected: 0
3 locks held by kworker/u9:0/50:
 #0: ffff88810e74a138 ((wq_completion)krxrpcd){+.+.}-{0:0}, at: process_one_work+0x294/0x636
 #1: ffff8881013a7e20 ((work_completion)(&rxnet->peer_keepalive_work)){+.+.}-{0:0}, at: process_one_work+0x294/0x636
 #2: ffff88817d366390 (&rxnet->peer_hash_lock){+.+.}-{2:2}, at: rxrpc_peer_keepalive_dispatch+0x2bd/0x35f
...
Call Trace:
 <TASK>
 dump_stack_lvl+0x4c/0x5f
 __might_resched+0x2cf/0x2f2
 __wait_for_common+0x87/0x1e8
 kthread_stop+0x14d/0x255
 rxrpc_peer_keepalive_dispatch+0x333/0x35f
 rxrpc_peer_keepalive_worker+0x2e9/0x449
 process_one_work+0x3c1/0x636
 worker_thread+0x25f/0x359
 kthread+0x1a6/0x1b5
 ret_from_fork+0x1f/0x30

Fixes: a275da6 ("rxrpc: Create a per-local endpoint receive queue and I/O thread")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
Link: https://lore.kernel.org/r/0000000000002b4a9f05ef2b616f@google.com/ [1]
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
David Howells authored and David S. Miller committed Dec 19, 2022
1 parent 8fbcc83 commit 608aecd
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 23 deletions.
1 change: 0 additions & 1 deletion net/rxrpc/ar-internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -1073,7 +1073,6 @@ void rxrpc_destroy_all_peers(struct rxrpc_net *);
struct rxrpc_peer *rxrpc_get_peer(struct rxrpc_peer *, enum rxrpc_peer_trace);
struct rxrpc_peer *rxrpc_get_peer_maybe(struct rxrpc_peer *, enum rxrpc_peer_trace);
void rxrpc_put_peer(struct rxrpc_peer *, enum rxrpc_peer_trace);
void rxrpc_put_peer_locked(struct rxrpc_peer *, enum rxrpc_peer_trace);

/*
* proc.c
Expand Down
10 changes: 7 additions & 3 deletions net/rxrpc/peer_event.c
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ static void rxrpc_peer_keepalive_dispatch(struct rxrpc_net *rxnet,
struct rxrpc_peer *peer;
const u8 mask = ARRAY_SIZE(rxnet->peer_keepalive) - 1;
time64_t keepalive_at;
bool use;
int slot;

spin_lock(&rxnet->peer_hash_lock);
Expand All @@ -247,9 +248,10 @@ static void rxrpc_peer_keepalive_dispatch(struct rxrpc_net *rxnet,
if (!rxrpc_get_peer_maybe(peer, rxrpc_peer_get_keepalive))
continue;

if (__rxrpc_use_local(peer->local, rxrpc_local_use_peer_keepalive)) {
spin_unlock(&rxnet->peer_hash_lock);
use = __rxrpc_use_local(peer->local, rxrpc_local_use_peer_keepalive);
spin_unlock(&rxnet->peer_hash_lock);

if (use) {
keepalive_at = peer->last_tx_at + RXRPC_KEEPALIVE_TIME;
slot = keepalive_at - base;
_debug("%02x peer %u t=%d {%pISp}",
Expand All @@ -270,9 +272,11 @@ static void rxrpc_peer_keepalive_dispatch(struct rxrpc_net *rxnet,
spin_lock(&rxnet->peer_hash_lock);
list_add_tail(&peer->keepalive_link,
&rxnet->peer_keepalive[slot & mask]);
spin_unlock(&rxnet->peer_hash_lock);
rxrpc_unuse_local(peer->local, rxrpc_local_unuse_peer_keepalive);
}
rxrpc_put_peer_locked(peer, rxrpc_peer_put_keepalive);
rxrpc_put_peer(peer, rxrpc_peer_put_keepalive);
spin_lock(&rxnet->peer_hash_lock);
}

spin_unlock(&rxnet->peer_hash_lock);
Expand Down
19 changes: 0 additions & 19 deletions net/rxrpc/peer_object.c
Original file line number Diff line number Diff line change
Expand Up @@ -438,25 +438,6 @@ void rxrpc_put_peer(struct rxrpc_peer *peer, enum rxrpc_peer_trace why)
}
}

/*
* Drop a ref on a peer record where the caller already holds the
* peer_hash_lock.
*/
void rxrpc_put_peer_locked(struct rxrpc_peer *peer, enum rxrpc_peer_trace why)
{
unsigned int debug_id = peer->debug_id;
bool dead;
int r;

dead = __refcount_dec_and_test(&peer->ref, &r);
trace_rxrpc_peer(debug_id, r - 1, why);
if (dead) {
hash_del_rcu(&peer->hash_link);
list_del_init(&peer->keepalive_link);
rxrpc_free_peer(peer);
}
}

/*
* Make sure all peer records have been discarded.
*/
Expand Down

0 comments on commit 608aecd

Please sign in to comment.