Skip to content

Commit

Permalink
rxrpc: Don't take call->user_mutex in rxrpc_new_incoming_call()
Browse files Browse the repository at this point in the history
Standard kernel mutexes cannot be used in any way from interrupt or softirq
context, so the user_mutex which manages access to a call cannot be a mutex
since on a new call the mutex must start off locked and be unlocked within
the softirq handler to prevent userspace interfering with a call we're
setting up.

Commit a0855d2 ("locking/mutex: Complain
upon mutex API misuse in IRQ contexts") causes big warnings to be splashed
in dmesg for each a new call that comes in from the server.  Whilst it
*seems* like it should be okay, since the accept path uses trylock, there
are issues with PI boosting and marking the wrong task as the owner.

Fix this by not taking the mutex in the softirq path at all.  It's not
obvious that there should be any need for it as the state is set before the
first notification is generated for the new call.

There's also no particular reason why the link-assessing ping should be
triggered inside the mutex.  It's not actually transmitted there anyway,
but rather it has to be deferred to a workqueue.

Further, I don't think that there's any particular reason that the socket
notification needs to be done from within rx->incoming_lock, so the amount
of time that lock is held can be shortened too and the ping prepared before
the new call notification is sent.

Fixes: 540b1c4 ("rxrpc: Fix deadlock between call creation and sendmsg/recvmsg")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Peter Zijlstra (Intel) <peterz@infradead.org>
cc: Ingo Molnar <mingo@redhat.com>
cc: Will Deacon <will@kernel.org>
cc: Davidlohr Bueso <dave@stgolabs.net>
  • Loading branch information
David Howells committed Dec 20, 2019
1 parent f33121c commit 13b7955
Showing 1 changed file with 3 additions and 17 deletions.
20 changes: 3 additions & 17 deletions net/rxrpc/call_accept.c
Original file line number Diff line number Diff line change
Expand Up @@ -381,18 +381,6 @@ struct rxrpc_call *rxrpc_new_incoming_call(struct rxrpc_local *local,
trace_rxrpc_receive(call, rxrpc_receive_incoming,
sp->hdr.serial, sp->hdr.seq);

/* Lock the call to prevent rxrpc_kernel_send/recv_data() and
* sendmsg()/recvmsg() inconveniently stealing the mutex once the
* notification is generated.
*
* The BUG should never happen because the kernel should be well
* behaved enough not to access the call before the first notification
* event and userspace is prevented from doing so until the state is
* appropriate.
*/
if (!mutex_trylock(&call->user_mutex))
BUG();

/* Make the call live. */
rxrpc_incoming_call(rx, call, skb);
conn = call->conn;
Expand Down Expand Up @@ -433,6 +421,9 @@ struct rxrpc_call *rxrpc_new_incoming_call(struct rxrpc_local *local,
BUG();
}
spin_unlock(&conn->state_lock);
spin_unlock(&rx->incoming_lock);

rxrpc_send_ping(call, skb);

if (call->state == RXRPC_CALL_SERVER_ACCEPTING)
rxrpc_notify_socket(call);
Expand All @@ -444,11 +435,6 @@ struct rxrpc_call *rxrpc_new_incoming_call(struct rxrpc_local *local,
*/
rxrpc_put_call(call, rxrpc_call_put);

spin_unlock(&rx->incoming_lock);

rxrpc_send_ping(call, skb);
mutex_unlock(&call->user_mutex);

_leave(" = %p{%d}", call, call->debug_id);
return call;

Expand Down

0 comments on commit 13b7955

Please sign in to comment.