[PATCH net] rxrpc: Fix call RCU cleanup using non-bh-safe locks

[PATCH net] rxrpc: Fix call RCU cleanup using non-bh-safe locks

[David Howells]

rxrpc_rcu_destroy_call(), which is called as an RCU callback to clean up a
put call, calls rxrpc_put_connection() which, deep in its bowels, takes a
number of spinlocks in a non-BH-safe way, including rxrpc_conn_id_lock and
local->client_conns_lock.  RCU callbacks, however, are normally called from
softirq context, which can cause lockdep to notice the locking
inconsistency.

To get lockdep to detect this, it's necessary to have the connection
cleaned up on the put at the end of the last of its calls, though normally
the clean up is deferred.  This can be induced, however, by starting a call
on an AF_RXRPC socket and then closing the socket without reading the
reply.

Fix this by having rxrpc_rcu_destroy_call() punt the destruction to a
workqueue if in softirq-mode and defer the destruction to process context.

Note that another way to fix this could be to add a bunch of bh-disable
annotations to the spinlocks concerned - and there might be more than just
those two - but that means spending more time with BHs disabled.

Note also that some of these places were covered by bh-disable spinlocks
belonging to the rxrpc_transport object, but these got removed without the
_bh annotation being retained on the next lock in.

Fixes: 999b69f89241 ("rxrpc: Kill the client connection bundle concept")
Reported-by: syzbot+d82f3ac8d87e7ccbb2c9@syzkaller.appspotmail.com
Reported-by: syzbot+3f1fd6b8cbf8702d134e@syzkaller.appspotmail.com
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Hillf Danton <hdanton@sina.com>
---

 net/rxrpc/call_object.c |   22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/net/rxrpc/call_object.c b/net/rxrpc/call_object.c
index dbdbc4f18b5e..c9f34b0a11df 100644
--- a/net/rxrpc/call_object.c
+++ b/net/rxrpc/call_object.c
@@ -562,11 +562,11 @@ void rxrpc_put_call(struct rxrpc_call *call, enum rxrpc_call_trace op)
 }
 
 /*
- * Final call destruction under RCU.
+ * Final call destruction - but must be done in process context.
  */
-static void rxrpc_rcu_destroy_call(struct rcu_head *rcu)
+static void rxrpc_destroy_call(struct work_struct *work)
 {
-	struct rxrpc_call *call = container_of(rcu, struct rxrpc_call, rcu);
+	struct rxrpc_call *call = container_of(work, struct rxrpc_call, processor);
 	struct rxrpc_net *rxnet = call->rxnet;
 
 	rxrpc_put_connection(call->conn);
@@ -578,6 +578,22 @@ static void rxrpc_rcu_destroy_call(struct rcu_head *rcu)
 		wake_up_var(&rxnet->nr_calls);
 }
 
+/*
+ * Final call destruction under RCU.
+ */
+static void rxrpc_rcu_destroy_call(struct rcu_head *rcu)
+{
+	struct rxrpc_call *call = container_of(rcu, struct rxrpc_call, rcu);
+
+	if (in_softirq()) {
+		INIT_WORK(&call->processor, rxrpc_destroy_call);
+		if (!rxrpc_queue_work(&call->processor))
+			BUG();
+	} else {
+		rxrpc_destroy_call(&call->processor);
+	}
+}
+
 /*
  * clean up a call
  */

Re: [PATCH net] rxrpc: Fix call RCU cleanup using non-bh-safe locks

[David Miller]

From: David Howells <dhowells@redhat.com>
Date: Thu, 06 Feb 2020 13:57:40 +0000

> rxrpc_rcu_destroy_call(), which is called as an RCU callback to clean up a
> put call, calls rxrpc_put_connection() which, deep in its bowels, takes a
> number of spinlocks in a non-BH-safe way, including rxrpc_conn_id_lock and
> local->client_conns_lock.  RCU callbacks, however, are normally called from
> softirq context, which can cause lockdep to notice the locking
> inconsistency.
> 
> To get lockdep to detect this, it's necessary to have the connection
> cleaned up on the put at the end of the last of its calls, though normally
> the clean up is deferred.  This can be induced, however, by starting a call
> on an AF_RXRPC socket and then closing the socket without reading the
> reply.
> 
> Fix this by having rxrpc_rcu_destroy_call() punt the destruction to a
> workqueue if in softirq-mode and defer the destruction to process context.
> 
> Note that another way to fix this could be to add a bunch of bh-disable
> annotations to the spinlocks concerned - and there might be more than just
> those two - but that means spending more time with BHs disabled.
> 
> Note also that some of these places were covered by bh-disable spinlocks
> belonging to the rxrpc_transport object, but these got removed without the
> _bh annotation being retained on the next lock in.
> 
> Fixes: 999b69f89241 ("rxrpc: Kill the client connection bundle concept")
> Reported-by: syzbot+d82f3ac8d87e7ccbb2c9@syzkaller.appspotmail.com
> Reported-by: syzbot+3f1fd6b8cbf8702d134e@syzkaller.appspotmail.com
> Signed-off-by: David Howells <dhowells@redhat.com>

Applied and queued up for -stable.