From 20cca071518af2c57e097ee497819328900fcc46 Mon Sep 17 00:00:00 2001 From: Chris Lew Date: Mon, 3 Oct 2022 09:57:33 -0700 Subject: [PATCH 1/5] net: qrtr: Change node mutex to a rw_semaphore There is a chance for a remote processor to become unresponsive while QRTR is trying to broadcast a control message. This results in the node lock being held for an extended period of time and prevents node lookup which is needed to send messages. Change the mutex to a rw_semaphore to allow concurrency during node lookups and broadcasts. Change-Id: I2e3fdde22edaac64f164cf08900e9d09b16c380d Signed-off-by: Chris Lew --- net/qrtr/af_qrtr.c | 41 ++++++++++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c index b814fc296f6e..bf641890ecd1 100644 --- a/net/qrtr/af_qrtr.c +++ b/net/qrtr/af_qrtr.c @@ -10,6 +10,7 @@ #include /* For TIOCINQ/OUTQ */ #include #include +#include #include @@ -112,7 +113,7 @@ static DEFINE_SPINLOCK(qrtr_nodes_lock); /* broadcast list */ static LIST_HEAD(qrtr_all_nodes); /* lock for qrtr_all_nodes and node reference */ -static DEFINE_MUTEX(qrtr_node_lock); +static DECLARE_RWSEM(qrtr_node_lock); /* local port allocation management */ static DEFINE_XARRAY_ALLOC(qrtr_ports); @@ -167,6 +168,32 @@ static int qrtr_bcast_enqueue(struct qrtr_node *node, struct sk_buff *skb, static struct qrtr_sock *qrtr_port_lookup(int port); static void qrtr_port_put(struct qrtr_sock *ipc); +static bool refcount_dec_and_rwsem_lock(refcount_t *r, + struct rw_semaphore *sem) +{ + if (refcount_dec_not_one(r)) + return false; + + down_write(sem); + if (!refcount_dec_and_test(r)) { + up_write(sem); + return false; + } + + return true; +} + +static inline int kref_put_rwsem_lock(struct kref *kref, + void (*release)(struct kref *kref), + struct rw_semaphore *sem) +{ + if (refcount_dec_and_rwsem_lock(&kref->refcount, sem)) { + release(kref); + return 1; + } + return 0; +} + /* Release node resources and free the node. * * Do not call directly, use qrtr_node_release. To be used with @@ -191,7 +218,7 @@ static void __qrtr_node_release(struct kref *kref) spin_unlock_irqrestore(&qrtr_nodes_lock, flags); list_del(&node->item); - mutex_unlock(&qrtr_node_lock); + up_write(&qrtr_node_lock); skb_queue_purge(&node->rx_queue); @@ -217,7 +244,7 @@ static void qrtr_node_release(struct qrtr_node *node) { if (!node) return; - kref_put_mutex(&node->ref, __qrtr_node_release, &qrtr_node_lock); + kref_put_rwsem_lock(&node->ref, __qrtr_node_release, &qrtr_node_lock); } /** @@ -612,9 +639,9 @@ int qrtr_endpoint_register(struct qrtr_endpoint *ep, unsigned int nid) qrtr_node_assign(node, nid); - mutex_lock(&qrtr_node_lock); + down_write(&qrtr_node_lock); list_add(&node->item, &qrtr_all_nodes); - mutex_unlock(&qrtr_node_lock); + up_write(&qrtr_node_lock); ep->node = node; return 0; @@ -921,7 +948,7 @@ static int qrtr_bcast_enqueue(struct qrtr_node *node, struct sk_buff *skb, { struct sk_buff *skbn; - mutex_lock(&qrtr_node_lock); + down_read(&qrtr_node_lock); list_for_each_entry(node, &qrtr_all_nodes, item) { if (node->nid == QRTR_EP_NID_AUTO) continue; @@ -931,7 +958,7 @@ static int qrtr_bcast_enqueue(struct qrtr_node *node, struct sk_buff *skb, skb_set_owner_w(skbn, skb->sk); qrtr_node_enqueue(node, skbn, type, from, to); } - mutex_unlock(&qrtr_node_lock); + up_read(&qrtr_node_lock); qrtr_local_enqueue(NULL, skb, type, from, to); From 9edfe6657fee645d47871c3fd31ac3fd135058a0 Mon Sep 17 00:00:00 2001 From: Chris Lew Date: Mon, 3 Oct 2022 09:57:42 -0700 Subject: [PATCH 2/5] net: qrtr: Change port allocation to use cyclic xa There is a race for clients that open sockets before the control port is bound. If a client gets an idr that was allocated before the control port is bound, there is a chance the previous address owner sent lookup packets to the control port. The new address owner will get residual responses to these lookup packets. Change the xa_alloc to xa_alloc_cyclic so new ids are allocated instead of trying to reuse the freed ids. Change-Id: Ie1bda7a818309503f80542e739bac646327296f7 Signed-off-by: Chris Lew --- net/qrtr/af_qrtr.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c index bf641890ecd1..05030c63df77 100644 --- a/net/qrtr/af_qrtr.c +++ b/net/qrtr/af_qrtr.c @@ -117,6 +117,7 @@ static DECLARE_RWSEM(qrtr_node_lock); /* local port allocation management */ static DEFINE_XARRAY_ALLOC(qrtr_ports); +u32 qrtr_ports_next = QRTR_MIN_EPH_SOCKET; /** * struct qrtr_node - endpoint node @@ -802,8 +803,9 @@ static int qrtr_port_assign(struct qrtr_sock *ipc, int *port) int rc; if (!*port) { - rc = xa_alloc(&qrtr_ports, port, ipc, QRTR_EPH_PORT_RANGE, - GFP_KERNEL); + rc = xa_alloc_cyclic(&qrtr_ports, port, ipc, + QRTR_EPH_PORT_RANGE, &qrtr_ports_next, + GFP_KERNEL); } else if (*port < QRTR_MIN_EPH_SOCKET && !capable(CAP_NET_ADMIN)) { rc = -EACCES; } else if (*port == QRTR_PORT_CTRL) { From 2ec1e4ae67db54ff0ceabbe2a0e08f01c1781828 Mon Sep 17 00:00:00 2001 From: Chris Lew Date: Mon, 3 Oct 2022 09:57:50 -0700 Subject: [PATCH 3/5] net: qrtr: Prevent stale ports from sending In order to prevent messages from stale sockets being sent, check if ENETRESET has been set on the socket and drop the packet. Change-Id: Ie9abc6c51139e82d3c9ebd4d546d1acd7269875e Signed-off-by: Chris Lew --- net/qrtr/af_qrtr.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c index 05030c63df77..8bc4e635e10b 100644 --- a/net/qrtr/af_qrtr.c +++ b/net/qrtr/af_qrtr.c @@ -919,6 +919,7 @@ static int qrtr_local_enqueue(struct qrtr_node *node, struct sk_buff *skb, { struct qrtr_sock *ipc; struct qrtr_cb *cb; + struct sock *sk = skb->sk; ipc = qrtr_port_lookup(to->sq_port); if (!ipc || &ipc->sk == skb->sk) { /* do not send to self */ @@ -928,6 +929,15 @@ static int qrtr_local_enqueue(struct qrtr_node *node, struct sk_buff *skb, return -ENODEV; } + /* Keep resetting NETRESET until socket is closed */ + if (sk && sk->sk_err == ENETRESET) { + sk->sk_err = ENETRESET; + sk_error_report(sk); + qrtr_port_put(ipc); + kfree_skb(skb); + return 0; + } + cb = (struct qrtr_cb *)skb->cb; cb->src_node = from->sq_node; cb->src_port = from->sq_port; From 8fa3203fdca717cb1260891a4295864249fe2d28 Mon Sep 17 00:00:00 2001 From: Chris Lew Date: Mon, 3 Oct 2022 09:57:58 -0700 Subject: [PATCH 4/5] net: qrtr: use alloc_skb_with_frags() in rx path net_dev_alloc() fails for high order allocations in system where memory is fragmented and cause communication stall due to packet drops. Use alloc_skb_with_frags() to avoid packet drops in memory fragmented case. Change-Id: I46b7e0da0ed04c9222c975ee930f0f8d9b41453b Signed-off-by: Chris Lew --- net/qrtr/af_qrtr.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c index 8bc4e635e10b..af77ee732753 100644 --- a/net/qrtr/af_qrtr.c +++ b/net/qrtr/af_qrtr.c @@ -255,12 +255,15 @@ static void qrtr_node_release(struct qrtr_node *node) */ static void qrtr_tx_resume(struct qrtr_node *node, struct sk_buff *skb) { - struct qrtr_ctrl_pkt *pkt = (struct qrtr_ctrl_pkt *)skb->data; - u64 remote_node = le32_to_cpu(pkt->client.node); - u32 remote_port = le32_to_cpu(pkt->client.port); + struct qrtr_ctrl_pkt pkt = {0,}; struct qrtr_tx_flow *flow; unsigned long key; + u64 remote_node; + u32 remote_port; + skb_copy_bits(skb, 0, &pkt, sizeof(pkt)); + remote_node = le32_to_cpu(pkt.client.node); + remote_port = le32_to_cpu(pkt.client.port); key = remote_node << 32 | remote_port; rcu_read_lock(); @@ -483,14 +486,16 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len) size_t size; unsigned int ver; size_t hdrlen; + int errcode; if (len == 0 || len & 3) return -EINVAL; - skb = __netdev_alloc_skb(NULL, len, GFP_ATOMIC | __GFP_NOWARN); + skb = alloc_skb_with_frags(sizeof(*v1), len, 0, &errcode, GFP_ATOMIC); if (!skb) return -ENOMEM; + skb_reserve(skb, sizeof(*v1)); cb = (struct qrtr_cb *)skb->cb; /* Version field in v1 is little endian, so this works for both cases */ @@ -544,7 +549,9 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len) cb->type != QRTR_TYPE_RESUME_TX) goto err; - skb_put_data(skb, data + hdrlen, size); + skb->data_len = size; + skb->len = size; + skb_store_bits(skb, 0, data + hdrlen, size); qrtr_node_assign(node, cb->src_node); From e00d15abf59110811900645dd8319dee24c81247 Mon Sep 17 00:00:00 2001 From: Chris Lew Date: Mon, 3 Oct 2022 09:58:06 -0700 Subject: [PATCH 5/5] net: qrtr: Improve qrtr_node_assign First check if the node is already assigned the requested nid, this allows an early exit for the common case. Do not allow later nodes to overwrite a node id. The first to claim a node id should get priority. Change-Id: Ie3da0c3b709de8c68f9918cd485492f9eaa41247 Signed-off-by: Chris Lew --- net/qrtr/af_qrtr.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c index af77ee732753..28b6fc6d5ccb 100644 --- a/net/qrtr/af_qrtr.c +++ b/net/qrtr/af_qrtr.c @@ -457,11 +457,13 @@ static void qrtr_node_assign(struct qrtr_node *node, unsigned int nid) { unsigned long flags; - if (nid == QRTR_EP_NID_AUTO) + if (nid == node->nid || nid == QRTR_EP_NID_AUTO) return; spin_lock_irqsave(&qrtr_nodes_lock, flags); - radix_tree_insert(&qrtr_nodes, nid, node); + if (!radix_tree_lookup(&qrtr_nodes, nid)) + radix_tree_insert(&qrtr_nodes, nid, node); + if (node->nid == QRTR_EP_NID_AUTO) node->nid = nid; spin_unlock_irqrestore(&qrtr_nodes_lock, flags);