rxrpc: Fix locking issue

[ Upstream commit ad25f5cb39872ca14bcbe00816ae65c22fe04b89 ]

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: 17926a79320a ("[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>
Signed-off-by: Sasha Levin <sashal@kernel.org>

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 4a2cda04d3e293b523549f72f535a0ddb9f5c631..b17ee4c4f618a4abb73d27672010c9fab5285bb9 100644 (file)
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -947,6 +947,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

diff --git a/include/linux/list.h b/include/linux/list.h
index a119dd1990d4ef1a3fb588af66779d2b06623073..d206ae93c06daa8bbf4e77ab4f97a023168254ab 100644 (file)
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -577,6 +577,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.

diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 5733890df64f57bfeb3d90bf1cf7f023bf1d3421..0b429111f85e4bae26ccfc2a1a6c8b61960cf433 100644 (file)
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -261,6 +261,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
  */

diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index dce056adb78cf44dcc57f561cad0d908f3b1ae66..f2d593e27b64f94bdcfc94ad0d2aa44448582543 100644 (file)
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -68,7 +68,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;

diff --git a/net/rxrpc/call_accept.c b/net/rxrpc/call_accept.c
index 1ae90fb979362b8a585b5a5f984de91ba871220e..8b24ffbc72efb3b8c606e2f90d309a8b1096e984 100644 (file)
--- a/net/rxrpc/call_accept.c
+++ b/net/rxrpc/call_accept.c
@@ -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));

diff --git a/net/rxrpc/call_object.c b/net/rxrpc/call_object.c
index 043508fd8d8a5d3a57d8a805093769b0be1f3b06..25c9a2cbf048c477e6a13bcb94ad19043bf5d9ea 100644 (file)
--- a/net/rxrpc/call_object.c
+++ b/net/rxrpc/call_object.c
@@ -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);
@@ -631,9 +631,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);
@@ -705,7 +705,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,
@@ -720,12 +720,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);

diff --git a/net/rxrpc/net_ns.c b/net/rxrpc/net_ns.c
index cc7e30733feb0d3f381269dfc4b9bcd5532b42ec..e4d6d432515bc510dae62ae62c89af2a9bfcef15 100644 (file)
--- a/net/rxrpc/net_ns.c
+++ b/net/rxrpc/net_ns.c
@@ -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);

diff --git a/net/rxrpc/proc.c b/net/rxrpc/proc.c
index e2f990754f8820216856820652988aec360e2868..5a67955cc00f658e040bf184152d1c5915db70c3 100644 (file)
--- a/net/rxrpc/proc.c
+++ b/net/rxrpc/proc.c
@@ -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();
 }