| From 66dcb668bb12a0c489dc552e3d68e57c8cddc894 Mon Sep 17 00:00:00 2001 |
| From: David Howells <dhowells@redhat.com> |
| Date: Fri, 20 Dec 2019 16:20:56 +0000 |
| Subject: [PATCH] rxrpc: Don't take call->user_mutex in |
| rxrpc_new_incoming_call() |
| |
| commit 13b7955a0252e15265386b229b814152f109b234 upstream. |
| |
| 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 a0855d24fc22d49cdc25664fb224caee16998683 ("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: 540b1c48c37a ("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> |
| Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> |
| |
| diff --git a/net/rxrpc/call_accept.c b/net/rxrpc/call_accept.c |
| index 0a8dab33d3fc..ec11c03e173d 100644 |
| --- a/net/rxrpc/call_accept.c |
| +++ b/net/rxrpc/call_accept.c |
| @@ -380,18 +380,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; |
| @@ -432,6 +420,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); |
| @@ -443,11 +434,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; |
| |
| -- |
| 2.7.4 |
| |