rxrpc: Fix incoming call setup race

An incoming call can race with rxrpc socket destruction, leading to a
leaked call.  This may result in an oops when the call timer eventually
expires:

   BUG: kernel NULL pointer dereference, address: 0000000000000874
   RIP: 0010:_raw_spin_lock_irqsave+0x2a/0x50
   Call Trace:
    <IRQ>
    try_to_wake_up+0x59/0x550
    ? __local_bh_enable_ip+0x37/0x80
    ? rxrpc_poke_call+0x52/0x110 [rxrpc]
    ? rxrpc_poke_call+0x110/0x110 [rxrpc]
    ? rxrpc_poke_call+0x110/0x110 [rxrpc]
    call_timer_fn+0x24/0x120

with a warning in the kernel log looking something like:

   rxrpc: Call 00000000ba5e571a still in use (1,SvAwtACK,1061d,0)!

incurred during rmmod of rxrpc.  The 1061d is the call flags:

   RECVMSG_READ_ALL, RX_HEARD, BEGAN_RX_TIMER, RX_LAST, EXPOSED,
   IS_SERVICE, RELEASED

but no DISCONNECTED flag (0x800), so it's an incoming (service) call and
it's still connected.

The race appears to be that:

 (1) rxrpc_new_incoming_call() consults the service struct, checks sk_state
     and allocates a call - then pauses, possibly for an interrupt.

 (2) rxrpc_release_sock() sets RXRPC_CLOSE, nulls the service pointer,
     discards the prealloc and releases all calls attached to the socket.

 (3) rxrpc_new_incoming_call() resumes, launching the new call, including
     its timer and attaching it to the socket.

Fix this by read-locking local->services_lock to access the AF_RXRPC socket
providing the service rather than RCU in rxrpc_new_incoming_call().
There's no real need to use RCU here as local->services_lock is only
write-locked by the socket side in two places: when binding and when
shutting down.

Fixes: 5e6ef4f1017c ("rxrpc: Make the I/O thread take over the call and local processor work")
Reported-by: Marc Dionne <marc.dionne@auristor.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: linux-afs@lists.infradead.org

diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c
index cf200e4e0eae1879a5d11fff0341c590757acd1b..ebbd4a1c3f86e411fa740749019dfc8b6567253a 100644 (file)
--- a/net/rxrpc/af_rxrpc.c
+++ b/net/rxrpc/af_rxrpc.c
@@ -155,10 +155,10 @@ static int rxrpc_bind(struct socket *sock, struct sockaddr *saddr, int len)
 
                if (service_id) {
                        write_lock(&local->services_lock);
-                       if (rcu_access_pointer(local->service))
+                       if (local->service)
                                goto service_in_use;
                        rx->local = local;
-                       rcu_assign_pointer(local->service, rx);
+                       local->service = rx;
                        write_unlock(&local->services_lock);
 
                        rx->sk.sk_state = RXRPC_SERVER_BOUND;
@@ -875,9 +875,9 @@ static int rxrpc_release_sock(struct sock *sk)
 
        sk->sk_state = RXRPC_CLOSE;
 
-       if (rx->local && rcu_access_pointer(rx->local->service) == rx) {
+       if (rx->local && rx->local->service == rx) {
                write_lock(&rx->local->services_lock);
-               rcu_assign_pointer(rx->local->service, NULL);
+               rx->local->service = NULL;
                write_unlock(&rx->local->services_lock);
        }
 

diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 007258538beebb3d20b42730c7ed05d55360de6a..433060cade0381352d387da24df723b4b4565d27 100644 (file)
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -283,7 +283,7 @@ struct rxrpc_local {
        struct socket           *socket;        /* my UDP socket */
        struct task_struct      *io_thread;
        struct completion       io_thread_ready; /* Indication that the I/O thread started */
-       struct rxrpc_sock __rcu *service;       /* Service(s) listening on this endpoint */
+       struct rxrpc_sock       *service;       /* Service(s) listening on this endpoint */
        struct rw_semaphore     defrag_sem;     /* control re-enablement of IP DF bit */
        struct sk_buff_head     rx_queue;       /* Received packets */
        struct list_head        conn_attend_q;  /* Conns requiring immediate attention */

diff --git a/net/rxrpc/call_accept.c b/net/rxrpc/call_accept.c
index 3fbf2fcaaf9e22d7bf8823cd147cb004123b44fe..3e8689fdc437146fee49fc611d4fdec2530b18b6 100644 (file)
--- a/net/rxrpc/call_accept.c
+++ b/net/rxrpc/call_accept.c
@@ -343,13 +343,13 @@ bool rxrpc_new_incoming_call(struct rxrpc_local *local,
        if (sp->hdr.type != RXRPC_PACKET_TYPE_DATA)
                return rxrpc_protocol_error(skb, rxrpc_eproto_no_service_call);
 
-       rcu_read_lock();
+       read_lock(&local->services_lock);
 
        /* Weed out packets to services we're not offering.  Packets that would
         * begin a call are explicitly rejected and the rest are just
         * discarded.
         */
-       rx = rcu_dereference(local->service);
+       rx = local->service;
        if (!rx || (sp->hdr.serviceId != rx->srx.srx_service &&
                    sp->hdr.serviceId != rx->second_service)
            ) {
@@ -399,7 +399,7 @@ bool rxrpc_new_incoming_call(struct rxrpc_local *local,
        spin_unlock(&conn->state_lock);
 
        spin_unlock(&rx->incoming_lock);
-       rcu_read_unlock();
+       read_unlock(&local->services_lock);
 
        if (hlist_unhashed(&call->error_link)) {
                spin_lock(&call->peer->lock);
@@ -413,20 +413,20 @@ bool rxrpc_new_incoming_call(struct rxrpc_local *local,
        return true;
 
 unsupported_service:
-       rcu_read_unlock();
+       read_unlock(&local->services_lock);
        return rxrpc_direct_abort(skb, rxrpc_abort_service_not_offered,
                                  RX_INVALID_OPERATION, -EOPNOTSUPP);
 unsupported_security:
-       rcu_read_unlock();
+       read_unlock(&local->services_lock);
        return rxrpc_direct_abort(skb, rxrpc_abort_service_not_offered,
                                  RX_INVALID_OPERATION, -EKEYREJECTED);
 no_call:
        spin_unlock(&rx->incoming_lock);
-       rcu_read_unlock();
+       read_unlock(&local->services_lock);
        _leave(" = f [%u]", skb->mark);
        return false;
 discard:
-       rcu_read_unlock();
+       read_unlock(&local->services_lock);
        return true;
 }
 

diff --git a/net/rxrpc/security.c b/net/rxrpc/security.c
index cd66634dffe694fac055ccafd64118e4002ec984..cb8dd1d3b1d49ed9dc26bdfa301bc263174cd876 100644 (file)
--- a/net/rxrpc/security.c
+++ b/net/rxrpc/security.c
@@ -178,9 +178,9 @@ struct key *rxrpc_look_up_server_security(struct rxrpc_connection *conn,
                sprintf(kdesc, "%u:%u",
                        sp->hdr.serviceId, sp->hdr.securityIndex);
 
-       rcu_read_lock();
+       read_lock(&conn->local->services_lock);
 
-       rx = rcu_dereference(conn->local->service);
+       rx = conn->local->service;
        if (!rx)
                goto out;
 
@@ -202,6 +202,6 @@ struct key *rxrpc_look_up_server_security(struct rxrpc_connection *conn,
        }
 
 out:
-       rcu_read_unlock();
+       read_unlock(&conn->local->services_lock);
        return key;
 }