* [PATCH net 1/3] rxrpc: Unlock new call in rxrpc_new_incoming_call() rather than the caller
2019-12-20 23:05 [PATCH net 0/3] rxrpc: Fixes David Howells
@ 2019-12-20 23:05 ` David Howells
2019-12-20 23:05 ` [PATCH net 2/3] rxrpc: Don't take call->user_mutex in rxrpc_new_incoming_call() David Howells
2019-12-20 23:05 ` [PATCH net 3/3] rxrpc: Fix missing security check on incoming calls David Howells
2 siblings, 0 replies; 4+ messages in thread
From: David Howells @ 2019-12-20 23:05 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
Move the unlock and the ping transmission for a new incoming call into
rxrpc_new_incoming_call() rather than doing it in the caller. This makes
it clearer to see what's going on.
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
cc: Ingo Molnar <mingo@redhat.com>
cc: Will Deacon <will@kernel.org>
cc: Davidlohr Bueso <dave@stgolabs.net>
---
net/rxrpc/call_accept.c | 36 ++++++++++++++++++++++++++++--------
net/rxrpc/input.c | 18 ------------------
2 files changed, 28 insertions(+), 26 deletions(-)
diff --git a/net/rxrpc/call_accept.c b/net/rxrpc/call_accept.c
index 135bf5cd8dd5..3685b1732f65 100644
--- a/net/rxrpc/call_accept.c
+++ b/net/rxrpc/call_accept.c
@@ -239,6 +239,22 @@ void rxrpc_discard_prealloc(struct rxrpc_sock *rx)
kfree(b);
}
+/*
+ * Ping the other end to fill our RTT cache and to retrieve the rwind
+ * and MTU parameters.
+ */
+static void rxrpc_send_ping(struct rxrpc_call *call, struct sk_buff *skb)
+{
+ struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
+ ktime_t now = skb->tstamp;
+
+ if (call->peer->rtt_usage < 3 ||
+ ktime_before(ktime_add_ms(call->peer->rtt_last_req, 1000), now))
+ rxrpc_propose_ACK(call, RXRPC_ACK_PING, sp->hdr.serial,
+ true, true,
+ rxrpc_propose_ack_ping_for_params);
+}
+
/*
* Allocate a new incoming call from the prealloc pool, along with a connection
* and a peer as necessary.
@@ -346,9 +362,7 @@ struct rxrpc_call *rxrpc_new_incoming_call(struct rxrpc_local *local,
sp->hdr.seq, RX_INVALID_OPERATION, ESHUTDOWN);
skb->mark = RXRPC_SKB_MARK_REJECT_ABORT;
skb->priority = RX_INVALID_OPERATION;
- _leave(" = NULL [close]");
- call = NULL;
- goto out;
+ goto no_call;
}
/* The peer, connection and call may all have sprung into existence due
@@ -361,9 +375,7 @@ struct rxrpc_call *rxrpc_new_incoming_call(struct rxrpc_local *local,
call = rxrpc_alloc_incoming_call(rx, local, peer, conn, skb);
if (!call) {
skb->mark = RXRPC_SKB_MARK_REJECT_BUSY;
- _leave(" = NULL [busy]");
- call = NULL;
- goto out;
+ goto no_call;
}
trace_rxrpc_receive(call, rxrpc_receive_incoming,
@@ -432,10 +444,18 @@ struct rxrpc_call *rxrpc_new_incoming_call(struct rxrpc_local *local,
*/
rxrpc_put_call(call, rxrpc_call_put);
- _leave(" = %p{%d}", call, call->debug_id);
-out:
spin_unlock(&rx->incoming_lock);
+
+ rxrpc_send_ping(call, skb);
+ mutex_unlock(&call->user_mutex);
+
+ _leave(" = %p{%d}", call, call->debug_id);
return call;
+
+no_call:
+ spin_unlock(&rx->incoming_lock);
+ _leave(" = NULL [%u]", skb->mark);
+ return NULL;
}
/*
diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index 157be1ff8697..86bd133b4fa0 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -192,22 +192,6 @@ static void rxrpc_congestion_management(struct rxrpc_call *call,
goto out_no_clear_ca;
}
-/*
- * Ping the other end to fill our RTT cache and to retrieve the rwind
- * and MTU parameters.
- */
-static void rxrpc_send_ping(struct rxrpc_call *call, struct sk_buff *skb)
-{
- struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
- ktime_t now = skb->tstamp;
-
- if (call->peer->rtt_usage < 3 ||
- ktime_before(ktime_add_ms(call->peer->rtt_last_req, 1000), now))
- rxrpc_propose_ACK(call, RXRPC_ACK_PING, sp->hdr.serial,
- true, true,
- rxrpc_propose_ack_ping_for_params);
-}
-
/*
* Apply a hard ACK by advancing the Tx window.
*/
@@ -1396,8 +1380,6 @@ int rxrpc_input_packet(struct sock *udp_sk, struct sk_buff *skb)
call = rxrpc_new_incoming_call(local, rx, skb);
if (!call)
goto reject_packet;
- rxrpc_send_ping(call, skb);
- mutex_unlock(&call->user_mutex);
}
/* Process a call packet; this either discards or passes on the ref
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH net 2/3] rxrpc: Don't take call->user_mutex in rxrpc_new_incoming_call()
2019-12-20 23:05 [PATCH net 0/3] rxrpc: Fixes David Howells
2019-12-20 23:05 ` [PATCH net 1/3] rxrpc: Unlock new call in rxrpc_new_incoming_call() rather than the caller David Howells
@ 2019-12-20 23:05 ` David Howells
2019-12-20 23:05 ` [PATCH net 3/3] rxrpc: Fix missing security check on incoming calls David Howells
2 siblings, 0 replies; 4+ messages in thread
From: David Howells @ 2019-12-20 23:05 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
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>
---
net/rxrpc/call_accept.c | 20 +++-----------------
1 file changed, 3 insertions(+), 17 deletions(-)
diff --git a/net/rxrpc/call_accept.c b/net/rxrpc/call_accept.c
index 3685b1732f65..44fa22b020ef 100644
--- a/net/rxrpc/call_accept.c
+++ b/net/rxrpc/call_accept.c
@@ -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;
@@ -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);
@@ -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;
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH net 3/3] rxrpc: Fix missing security check on incoming calls
2019-12-20 23:05 [PATCH net 0/3] rxrpc: Fixes David Howells
2019-12-20 23:05 ` [PATCH net 1/3] rxrpc: Unlock new call in rxrpc_new_incoming_call() rather than the caller David Howells
2019-12-20 23:05 ` [PATCH net 2/3] rxrpc: Don't take call->user_mutex in rxrpc_new_incoming_call() David Howells
@ 2019-12-20 23:05 ` David Howells
2 siblings, 0 replies; 4+ messages in thread
From: David Howells @ 2019-12-20 23:05 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
Fix rxrpc_new_incoming_call() to check that we have a suitable service key
available for the combination of service ID and security class of a new
incoming call - and to reject calls for which we don't.
This causes an assertion like the following to appear:
rxrpc: Assertion failed - 6(0x6) == 12(0xc) is false
kernel BUG at net/rxrpc/call_object.c:456!
Where call->state is RXRPC_CALL_SERVER_SECURING (6) rather than
RXRPC_CALL_COMPLETE (12).
Fixes: 248f219cb8bc ("rxrpc: Rewrite the data and ack handling code")
Reported-by: Marc Dionne <marc.dionne@auristor.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/ar-internal.h | 10 +++++--
net/rxrpc/call_accept.c | 14 +++++++--
net/rxrpc/conn_event.c | 16 +----------
net/rxrpc/conn_service.c | 4 +++
net/rxrpc/rxkad.c | 5 ++-
net/rxrpc/security.c | 70 ++++++++++++++++++++++------------------------
6 files changed, 59 insertions(+), 60 deletions(-)
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 7c7d10f2e0c1..5e99df80e80a 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -209,6 +209,7 @@ struct rxrpc_skb_priv {
struct rxrpc_security {
const char *name; /* name of this service */
u8 security_index; /* security type provided */
+ u32 no_key_abort; /* Abort code indicating no key */
/* Initialise a security service */
int (*init)(void);
@@ -977,8 +978,9 @@ static inline void rxrpc_reduce_conn_timer(struct rxrpc_connection *conn,
struct rxrpc_connection *rxrpc_find_service_conn_rcu(struct rxrpc_peer *,
struct sk_buff *);
struct rxrpc_connection *rxrpc_prealloc_service_connection(struct rxrpc_net *, gfp_t);
-void rxrpc_new_incoming_connection(struct rxrpc_sock *,
- struct rxrpc_connection *, struct sk_buff *);
+void rxrpc_new_incoming_connection(struct rxrpc_sock *, struct rxrpc_connection *,
+ const struct rxrpc_security *, struct key *,
+ struct sk_buff *);
void rxrpc_unpublish_service_conn(struct rxrpc_connection *);
/*
@@ -1103,7 +1105,9 @@ extern const struct rxrpc_security rxkad;
int __init rxrpc_init_security(void);
void rxrpc_exit_security(void);
int rxrpc_init_client_conn_security(struct rxrpc_connection *);
-int rxrpc_init_server_conn_security(struct rxrpc_connection *);
+bool rxrpc_look_up_server_security(struct rxrpc_local *, struct rxrpc_sock *,
+ const struct rxrpc_security **, struct key **,
+ struct sk_buff *);
/*
* sendmsg.c
diff --git a/net/rxrpc/call_accept.c b/net/rxrpc/call_accept.c
index 44fa22b020ef..70e44abf106c 100644
--- a/net/rxrpc/call_accept.c
+++ b/net/rxrpc/call_accept.c
@@ -263,6 +263,8 @@ static struct rxrpc_call *rxrpc_alloc_incoming_call(struct rxrpc_sock *rx,
struct rxrpc_local *local,
struct rxrpc_peer *peer,
struct rxrpc_connection *conn,
+ const struct rxrpc_security *sec,
+ struct key *key,
struct sk_buff *skb)
{
struct rxrpc_backlog *b = rx->backlog;
@@ -310,7 +312,7 @@ static struct rxrpc_call *rxrpc_alloc_incoming_call(struct rxrpc_sock *rx,
conn->params.local = rxrpc_get_local(local);
conn->params.peer = peer;
rxrpc_see_connection(conn);
- rxrpc_new_incoming_connection(rx, conn, skb);
+ rxrpc_new_incoming_connection(rx, conn, sec, key, skb);
} else {
rxrpc_get_connection(conn);
}
@@ -349,9 +351,11 @@ struct rxrpc_call *rxrpc_new_incoming_call(struct rxrpc_local *local,
struct sk_buff *skb)
{
struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
+ const struct rxrpc_security *sec = NULL;
struct rxrpc_connection *conn;
struct rxrpc_peer *peer = NULL;
- struct rxrpc_call *call;
+ struct rxrpc_call *call = NULL;
+ struct key *key = NULL;
_enter("");
@@ -372,7 +376,11 @@ struct rxrpc_call *rxrpc_new_incoming_call(struct rxrpc_local *local,
*/
conn = rxrpc_find_connection_rcu(local, skb, &peer);
- call = rxrpc_alloc_incoming_call(rx, local, peer, conn, skb);
+ if (!conn && !rxrpc_look_up_server_security(local, rx, &sec, &key, skb))
+ goto no_call;
+
+ call = rxrpc_alloc_incoming_call(rx, local, peer, conn, sec, key, skb);
+ key_put(key);
if (!call) {
skb->mark = RXRPC_SKB_MARK_REJECT_BUSY;
goto no_call;
diff --git a/net/rxrpc/conn_event.c b/net/rxrpc/conn_event.c
index a1ceef4f5cd0..808a4723f868 100644
--- a/net/rxrpc/conn_event.c
+++ b/net/rxrpc/conn_event.c
@@ -376,21 +376,7 @@ static void rxrpc_secure_connection(struct rxrpc_connection *conn)
_enter("{%d}", conn->debug_id);
ASSERT(conn->security_ix != 0);
-
- if (!conn->params.key) {
- _debug("set up security");
- ret = rxrpc_init_server_conn_security(conn);
- switch (ret) {
- case 0:
- break;
- case -ENOENT:
- abort_code = RX_CALL_DEAD;
- goto abort;
- default:
- abort_code = RXKADNOAUTH;
- goto abort;
- }
- }
+ ASSERT(conn->server_key);
if (conn->security->issue_challenge(conn) < 0) {
abort_code = RX_CALL_DEAD;
diff --git a/net/rxrpc/conn_service.c b/net/rxrpc/conn_service.c
index 123d6ceab15c..21da48e3d2e5 100644
--- a/net/rxrpc/conn_service.c
+++ b/net/rxrpc/conn_service.c
@@ -148,6 +148,8 @@ struct rxrpc_connection *rxrpc_prealloc_service_connection(struct rxrpc_net *rxn
*/
void rxrpc_new_incoming_connection(struct rxrpc_sock *rx,
struct rxrpc_connection *conn,
+ const struct rxrpc_security *sec,
+ struct key *key,
struct sk_buff *skb)
{
struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
@@ -160,6 +162,8 @@ void rxrpc_new_incoming_connection(struct rxrpc_sock *rx,
conn->service_id = sp->hdr.serviceId;
conn->security_ix = sp->hdr.securityIndex;
conn->out_clientflag = 0;
+ conn->security = sec;
+ conn->server_key = key_get(key);
if (conn->security_ix)
conn->state = RXRPC_CONN_SERVICE_UNSECURED;
else
diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 8d8aa3c230b5..098f1f9ec53b 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -648,9 +648,9 @@ static int rxkad_issue_challenge(struct rxrpc_connection *conn)
u32 serial;
int ret;
- _enter("{%d,%x}", conn->debug_id, key_serial(conn->params.key));
+ _enter("{%d,%x}", conn->debug_id, key_serial(conn->server_key));
- ret = key_validate(conn->params.key);
+ ret = key_validate(conn->server_key);
if (ret < 0)
return ret;
@@ -1293,6 +1293,7 @@ static void rxkad_exit(void)
const struct rxrpc_security rxkad = {
.name = "rxkad",
.security_index = RXRPC_SECURITY_RXKAD,
+ .no_key_abort = RXKADUNKNOWNKEY,
.init = rxkad_init,
.exit = rxkad_exit,
.init_connection_security = rxkad_init_connection_security,
diff --git a/net/rxrpc/security.c b/net/rxrpc/security.c
index a4c47d2b7054..9b1fb9ed0717 100644
--- a/net/rxrpc/security.c
+++ b/net/rxrpc/security.c
@@ -101,62 +101,58 @@ int rxrpc_init_client_conn_security(struct rxrpc_connection *conn)
}
/*
- * initialise the security on a server connection
+ * Find the security key for a server connection.
*/
-int rxrpc_init_server_conn_security(struct rxrpc_connection *conn)
+bool rxrpc_look_up_server_security(struct rxrpc_local *local, struct rxrpc_sock *rx,
+ const struct rxrpc_security **_sec,
+ struct key **_key,
+ struct sk_buff *skb)
{
const struct rxrpc_security *sec;
- struct rxrpc_local *local = conn->params.local;
- struct rxrpc_sock *rx;
- struct key *key;
- key_ref_t kref;
+ struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
+ key_ref_t kref = NULL;
char kdesc[5 + 1 + 3 + 1];
_enter("");
- sprintf(kdesc, "%u:%u", conn->service_id, conn->security_ix);
+ sprintf(kdesc, "%u:%u", sp->hdr.serviceId, sp->hdr.securityIndex);
- sec = rxrpc_security_lookup(conn->security_ix);
+ sec = rxrpc_security_lookup(sp->hdr.securityIndex);
if (!sec) {
- _leave(" = -ENOKEY [lookup]");
- return -ENOKEY;
+ trace_rxrpc_abort(0, "SVS",
+ sp->hdr.cid, sp->hdr.callNumber, sp->hdr.seq,
+ RX_INVALID_OPERATION, EKEYREJECTED);
+ skb->mark = RXRPC_SKB_MARK_REJECT_ABORT;
+ skb->priority = RX_INVALID_OPERATION;
+ return false;
}
- /* find the service */
- read_lock(&local->services_lock);
- rx = rcu_dereference_protected(local->service,
- lockdep_is_held(&local->services_lock));
- if (rx && (rx->srx.srx_service == conn->service_id ||
- rx->second_service == conn->service_id))
- goto found_service;
+ if (sp->hdr.securityIndex == RXRPC_SECURITY_NONE)
+ goto out;
- /* the service appears to have died */
- read_unlock(&local->services_lock);
- _leave(" = -ENOENT");
- return -ENOENT;
-
-found_service:
if (!rx->securities) {
- read_unlock(&local->services_lock);
- _leave(" = -ENOKEY");
- return -ENOKEY;
+ trace_rxrpc_abort(0, "SVR",
+ sp->hdr.cid, sp->hdr.callNumber, sp->hdr.seq,
+ RX_INVALID_OPERATION, EKEYREJECTED);
+ skb->mark = RXRPC_SKB_MARK_REJECT_ABORT;
+ skb->priority = RX_INVALID_OPERATION;
+ return false;
}
/* look through the service's keyring */
kref = keyring_search(make_key_ref(rx->securities, 1UL),
&key_type_rxrpc_s, kdesc, true);
if (IS_ERR(kref)) {
- read_unlock(&local->services_lock);
- _leave(" = %ld [search]", PTR_ERR(kref));
- return PTR_ERR(kref);
+ trace_rxrpc_abort(0, "SVK",
+ sp->hdr.cid, sp->hdr.callNumber, sp->hdr.seq,
+ sec->no_key_abort, EKEYREJECTED);
+ skb->mark = RXRPC_SKB_MARK_REJECT_ABORT;
+ skb->priority = sec->no_key_abort;
+ return false;
}
- key = key_ref_to_ptr(kref);
- read_unlock(&local->services_lock);
-
- conn->server_key = key;
- conn->security = sec;
-
- _leave(" = 0");
- return 0;
+out:
+ *_sec = sec;
+ *_key = key_ref_to_ptr(kref);
+ return true;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread