Skip to content

Commit

Permalink
rxrpc: Fix locking issue
Browse files Browse the repository at this point in the history
There's a locking issue with the per-netns list of calls in rxrpc.  The
pieces of code that add and remove a call from the list use write_lock()
and the calls procfile uses read_lock() to access it.  However, the timer
callback function may trigger a removal by trying to queue a call for
processing and finding that it's already queued - at which point it has a
spare refcount that it has to do something with.  Unfortunately, if it puts
the call and this reduces the refcount to 0, the call will be removed from
the list.  Unfortunately, since the _bh variants of the locking functions
aren't used, this can deadlock.

================================
WARNING: inconsistent lock state
5.18.0-rc3-build4+ #10 Not tainted
--------------------------------
inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
ksoftirqd/2/25 [HC0[0]:SC1[1]:HE1:SE0] takes:
ffff888107ac4038 (&rxnet->call_lock){+.?.}-{2:2}, at: rxrpc_put_call+0x103/0x14b
{SOFTIRQ-ON-W} state was registered at:
...
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&rxnet->call_lock);
  <Interrupt>
    lock(&rxnet->call_lock);

 *** DEADLOCK ***

1 lock held by ksoftirqd/2/25:
 #0: ffff8881008ffdb0 ((&call->timer)){+.-.}-{0:0}, at: call_timer_fn+0x5/0x23d

Changes
=======
ver #2)
 - Changed to using list_next_rcu() rather than rcu_dereference() directly.

Fixes: 17926a7 ("[AF_RXRPC]: Provide secure RxRPC sockets for use by userspace and kernel both")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
dhowells authored and davem330 committed May 22, 2022
1 parent a057542 commit ad25f5c
Show file tree
Hide file tree
Showing 8 changed files with 62 additions and 22 deletions.
32 changes: 32 additions & 0 deletions fs/seq_file.c
Original file line number Diff line number Diff line change
Expand Up @@ -931,6 +931,38 @@ struct list_head *seq_list_next(void *v, struct list_head *head, loff_t *ppos)
}
EXPORT_SYMBOL(seq_list_next);

struct list_head *seq_list_start_rcu(struct list_head *head, loff_t pos)
{
struct list_head *lh;

list_for_each_rcu(lh, head)
if (pos-- == 0)
return lh;

return NULL;
}
EXPORT_SYMBOL(seq_list_start_rcu);

struct list_head *seq_list_start_head_rcu(struct list_head *head, loff_t pos)
{
if (!pos)
return head;

return seq_list_start_rcu(head, pos - 1);
}
EXPORT_SYMBOL(seq_list_start_head_rcu);

struct list_head *seq_list_next_rcu(void *v, struct list_head *head,
loff_t *ppos)
{
struct list_head *lh;

lh = list_next_rcu((struct list_head *)v);
++*ppos;
return lh == head ? NULL : lh;
}
EXPORT_SYMBOL(seq_list_next_rcu);

/**
* seq_hlist_start - start an iteration of a hlist
* @head: the head of the hlist
Expand Down
10 changes: 10 additions & 0 deletions include/linux/list.h
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,16 @@ static inline void list_splice_tail_init(struct list_head *list,
#define list_for_each(pos, head) \
for (pos = (head)->next; !list_is_head(pos, (head)); pos = pos->next)

/**
* list_for_each_rcu - Iterate over a list in an RCU-safe fashion
* @pos: the &struct list_head to use as a loop cursor.
* @head: the head for your list.
*/
#define list_for_each_rcu(pos, head) \
for (pos = rcu_dereference((head)->next); \
!list_is_head(pos, (head)); \
pos = rcu_dereference(pos->next))

/**
* list_for_each_continue - continue iteration over a list
* @pos: the &struct list_head to use as a loop cursor.
Expand Down
4 changes: 4 additions & 0 deletions include/linux/seq_file.h
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,10 @@ extern struct list_head *seq_list_start_head(struct list_head *head,
extern struct list_head *seq_list_next(void *v, struct list_head *head,
loff_t *ppos);

extern struct list_head *seq_list_start_rcu(struct list_head *head, loff_t pos);
extern struct list_head *seq_list_start_head_rcu(struct list_head *head, loff_t pos);
extern struct list_head *seq_list_next_rcu(void *v, struct list_head *head, loff_t *ppos);

/*
* Helpers for iteration over hlist_head-s in seq_files
*/
Expand Down
2 changes: 1 addition & 1 deletion net/rxrpc/ar-internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ struct rxrpc_net {
struct proc_dir_entry *proc_net; /* Subdir in /proc/net */
u32 epoch; /* Local epoch for detecting local-end reset */
struct list_head calls; /* List of calls active in this namespace */
rwlock_t call_lock; /* Lock for ->calls */
spinlock_t call_lock; /* Lock for ->calls */
atomic_t nr_calls; /* Count of allocated calls */

atomic_t nr_conns;
Expand Down
6 changes: 3 additions & 3 deletions net/rxrpc/call_accept.c
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,9 @@ static int rxrpc_service_prealloc_one(struct rxrpc_sock *rx,
write_unlock(&rx->call_lock);

rxnet = call->rxnet;
write_lock(&rxnet->call_lock);
list_add_tail(&call->link, &rxnet->calls);
write_unlock(&rxnet->call_lock);
spin_lock_bh(&rxnet->call_lock);
list_add_tail_rcu(&call->link, &rxnet->calls);
spin_unlock_bh(&rxnet->call_lock);

b->call_backlog[call_head] = call;
smp_store_release(&b->call_backlog_head, (call_head + 1) & (size - 1));
Expand Down
18 changes: 9 additions & 9 deletions net/rxrpc/call_object.c
Original file line number Diff line number Diff line change
Expand Up @@ -337,9 +337,9 @@ struct rxrpc_call *rxrpc_new_client_call(struct rxrpc_sock *rx,
write_unlock(&rx->call_lock);

rxnet = call->rxnet;
write_lock(&rxnet->call_lock);
list_add_tail(&call->link, &rxnet->calls);
write_unlock(&rxnet->call_lock);
spin_lock_bh(&rxnet->call_lock);
list_add_tail_rcu(&call->link, &rxnet->calls);
spin_unlock_bh(&rxnet->call_lock);

/* From this point on, the call is protected by its own lock. */
release_sock(&rx->sk);
Expand Down Expand Up @@ -633,9 +633,9 @@ void rxrpc_put_call(struct rxrpc_call *call, enum rxrpc_call_trace op)
ASSERTCMP(call->state, ==, RXRPC_CALL_COMPLETE);

if (!list_empty(&call->link)) {
write_lock(&rxnet->call_lock);
spin_lock_bh(&rxnet->call_lock);
list_del_init(&call->link);
write_unlock(&rxnet->call_lock);
spin_unlock_bh(&rxnet->call_lock);
}

rxrpc_cleanup_call(call);
Expand Down Expand Up @@ -707,7 +707,7 @@ void rxrpc_destroy_all_calls(struct rxrpc_net *rxnet)
_enter("");

if (!list_empty(&rxnet->calls)) {
write_lock(&rxnet->call_lock);
spin_lock_bh(&rxnet->call_lock);

while (!list_empty(&rxnet->calls)) {
call = list_entry(rxnet->calls.next,
Expand All @@ -722,12 +722,12 @@ void rxrpc_destroy_all_calls(struct rxrpc_net *rxnet)
rxrpc_call_states[call->state],
call->flags, call->events);

write_unlock(&rxnet->call_lock);
spin_unlock_bh(&rxnet->call_lock);
cond_resched();
write_lock(&rxnet->call_lock);
spin_lock_bh(&rxnet->call_lock);
}

write_unlock(&rxnet->call_lock);
spin_unlock_bh(&rxnet->call_lock);
}

atomic_dec(&rxnet->nr_calls);
Expand Down
2 changes: 1 addition & 1 deletion net/rxrpc/net_ns.c
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ static __net_init int rxrpc_init_net(struct net *net)
rxnet->epoch |= RXRPC_RANDOM_EPOCH;

INIT_LIST_HEAD(&rxnet->calls);
rwlock_init(&rxnet->call_lock);
spin_lock_init(&rxnet->call_lock);
atomic_set(&rxnet->nr_calls, 1);

atomic_set(&rxnet->nr_conns, 1);
Expand Down
10 changes: 2 additions & 8 deletions net/rxrpc/proc.c
Original file line number Diff line number Diff line change
Expand Up @@ -26,29 +26,23 @@ static const char *const rxrpc_conn_states[RXRPC_CONN__NR_STATES] = {
*/
static void *rxrpc_call_seq_start(struct seq_file *seq, loff_t *_pos)
__acquires(rcu)
__acquires(rxnet->call_lock)
{
struct rxrpc_net *rxnet = rxrpc_net(seq_file_net(seq));

rcu_read_lock();
read_lock(&rxnet->call_lock);
return seq_list_start_head(&rxnet->calls, *_pos);
return seq_list_start_head_rcu(&rxnet->calls, *_pos);
}

static void *rxrpc_call_seq_next(struct seq_file *seq, void *v, loff_t *pos)
{
struct rxrpc_net *rxnet = rxrpc_net(seq_file_net(seq));

return seq_list_next(v, &rxnet->calls, pos);
return seq_list_next_rcu(v, &rxnet->calls, pos);
}

static void rxrpc_call_seq_stop(struct seq_file *seq, void *v)
__releases(rxnet->call_lock)
__releases(rcu)
{
struct rxrpc_net *rxnet = rxrpc_net(seq_file_net(seq));

read_unlock(&rxnet->call_lock);
rcu_read_unlock();
}

Expand Down

0 comments on commit ad25f5c

Please sign in to comment.