Patch #1 Addresses a race condition between qrtr driver, ns opening
the control post and sending data to it.
Patch #2 Address the issue with legacy targets sending the SSR
notifications using DEL_PROC control message.
[v2] Patch #1,
Added more appropriate commit text,
Removed a redundant check and fixed local variables
in reverse-christmas tree order.
Patch #2,
Fixed a sparse warning.
Chris Lew (1):
net: qrtr: Prevent stale ports from sending
Sricharan Ramabadhran (1):
net: qrtr: Add support for processing DEL_PROC type control message
include/uapi/linux/qrtr.h | 1 +
net/qrtr/af_qrtr.c | 74 +++++++++++++++++++++++++++++++++++++++
2 files changed, 75 insertions(+)
--
2.34.1
From: Chris Lew <[email protected]>
If some client tries to initialize a QRTR socket during QRTR
init, the socket will become stale after the ns(namespace server)
binds to the QRTR control port. The client should close and reopen
the QRTR socket once ENETRESET is posted to the stale socket.
There is a possibility that a client tries to send to the NS before
processing the ENETRESET. In the case of a NEW_SERVER control message,
the control message will reach the NS and be forwarded to the firmware.
The client will then process the ENETRESET closing and re-opening the
socket which triggers a DEL_SERVER and then a second NEW_SERVER.
This scenario will give an unnecessary disconnect to the clients on the
firmware who were able to initialize on the first NEW_SERVER.
This was seen when qrtr-ns was a separate application, but there is
still a potential gap between AF_QIPCRTR socket register and when
qrtr_ns_init binds to the socket where this issue can still occur.
Signed-off-by: Chris Lew <[email protected]>
Signed-off-by: Vignesh Viswanathan <[email protected]>
Signed-off-by: Sricharan Ramabadhran <[email protected]>
---
[v2] Added more appropriate commit text,
Removed a redundant check and fixed local variables
in reverse-christmas tree order.
Added 'Chris Lew' Signed-off tag.
net/qrtr/af_qrtr.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
index 41ece61eb57a..e5cf4245c3dc 100644
--- a/net/qrtr/af_qrtr.c
+++ b/net/qrtr/af_qrtr.c
@@ -849,6 +849,7 @@ static int qrtr_local_enqueue(struct qrtr_node *node, struct sk_buff *skb,
int type, struct sockaddr_qrtr *from,
struct sockaddr_qrtr *to)
{
+ struct sock *sk = skb->sk;
struct qrtr_sock *ipc;
struct qrtr_cb *cb;
@@ -860,6 +861,14 @@ 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_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;
--
2.34.1
On Wed, Sep 20, 2023 at 11:03:16AM +0530, Sricharan Ramabadhran wrote:
> From: Chris Lew <[email protected]>
>
> If some client tries to initialize a QRTR socket during QRTR
> init, the socket will become stale after the ns(namespace server)
> binds to the QRTR control port. The client should close and reopen
> the QRTR socket once ENETRESET is posted to the stale socket.
>
> There is a possibility that a client tries to send to the NS before
> processing the ENETRESET. In the case of a NEW_SERVER control message,
> the control message will reach the NS and be forwarded to the firmware.
> The client will then process the ENETRESET closing and re-opening the
> socket which triggers a DEL_SERVER and then a second NEW_SERVER.
> This scenario will give an unnecessary disconnect to the clients on the
> firmware who were able to initialize on the first NEW_SERVER.
>
> This was seen when qrtr-ns was a separate application, but there is
> still a potential gap between AF_QIPCRTR socket register and when
> qrtr_ns_init binds to the socket where this issue can still occur.
>
> Signed-off-by: Chris Lew <[email protected]>
> Signed-off-by: Vignesh Viswanathan <[email protected]>
> Signed-off-by: Sricharan Ramabadhran <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Regards,
Bjorn
For certain rproc's like modem, when it goes down and endpoint gets
un-registered, DEL_PROC control message gets forwarded to other
remote nodes. So remote nodes should listen on the message,
wakeup all local waiters waiting for tx_resume notifications
(which will never come) and also forward the message to all
local qrtr sockets like QMI etc. Adding the support here.
Introduced a new rx worker here, because endpoint_post can get called in
atomic contexts, but processing of DEL_PROC needs to acquire node
qrtr_tx mutex.
Signed-off-by: Sricharan Ramabadhran <[email protected]>
---
[v2] Fixed a sparse warning.
include/uapi/linux/qrtr.h | 1 +
net/qrtr/af_qrtr.c | 65 +++++++++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+)
diff --git a/include/uapi/linux/qrtr.h b/include/uapi/linux/qrtr.h
index f7e2fb3d752b..1c920159d610 100644
--- a/include/uapi/linux/qrtr.h
+++ b/include/uapi/linux/qrtr.h
@@ -26,6 +26,7 @@ enum qrtr_pkt_type {
QRTR_TYPE_PING = 9,
QRTR_TYPE_NEW_LOOKUP = 10,
QRTR_TYPE_DEL_LOOKUP = 11,
+ QRTR_TYPE_DEL_PROC = 13,
};
struct qrtr_ctrl_pkt {
diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
index e5cf4245c3dc..016b946f308b 100644
--- a/net/qrtr/af_qrtr.c
+++ b/net/qrtr/af_qrtr.c
@@ -3,6 +3,7 @@
* Copyright (c) 2015, Sony Mobile Communications Inc.
* Copyright (c) 2013, The Linux Foundation. All rights reserved.
*/
+#include <linux/kthread.h>
#include <linux/module.h>
#include <linux/netlink.h>
#include <linux/qrtr.h>
@@ -122,6 +123,9 @@ static DEFINE_XARRAY_ALLOC(qrtr_ports);
* @qrtr_tx_lock: lock for qrtr_tx_flow inserts
* @rx_queue: receive queue
* @item: list item for broadcast list
+ * @kworker: worker thread for recv work
+ * @task: task to run the worker thread
+ * @read_data: scheduled work for recv work
*/
struct qrtr_node {
struct mutex ep_lock;
@@ -134,6 +138,9 @@ struct qrtr_node {
struct sk_buff_head rx_queue;
struct list_head item;
+ struct kthread_worker kworker;
+ struct task_struct *task;
+ struct kthread_work read_data;
};
/**
@@ -186,6 +193,9 @@ static void __qrtr_node_release(struct kref *kref)
list_del(&node->item);
mutex_unlock(&qrtr_node_lock);
+ kthread_flush_worker(&node->kworker);
+ kthread_stop(node->task);
+
skb_queue_purge(&node->rx_queue);
/* Free tx flow counters */
@@ -526,6 +536,9 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
if (cb->type == QRTR_TYPE_RESUME_TX) {
qrtr_tx_resume(node, skb);
+ } else if (cb->type == QRTR_TYPE_DEL_PROC) {
+ skb_queue_tail(&node->rx_queue, skb);
+ kthread_queue_work(&node->kworker, &node->read_data);
} else {
ipc = qrtr_port_lookup(cb->dst_port);
if (!ipc)
@@ -574,6 +587,50 @@ static struct sk_buff *qrtr_alloc_ctrl_packet(struct qrtr_ctrl_pkt **pkt,
return skb;
}
+/* Handle DEL_PROC control message */
+static void qrtr_node_rx_work(struct kthread_work *work)
+{
+ struct qrtr_node *node = container_of(work, struct qrtr_node,
+ read_data);
+ struct radix_tree_iter iter;
+ struct qrtr_tx_flow *flow;
+ struct qrtr_ctrl_pkt *pkt;
+ struct qrtr_sock *ipc;
+ struct sk_buff *skb;
+ void __rcu **slot;
+
+ while ((skb = skb_dequeue(&node->rx_queue)) != NULL) {
+ struct qrtr_cb *cb = (struct qrtr_cb *)skb->cb;
+
+ ipc = qrtr_port_lookup(cb->dst_port);
+ if (!ipc) {
+ kfree_skb(skb);
+ continue;
+ }
+
+ if (cb->type == QRTR_TYPE_DEL_PROC) {
+ /* Free tx flow counters */
+ mutex_lock(&node->qrtr_tx_lock);
+ radix_tree_for_each_slot(slot, &node->qrtr_tx_flow, &iter, 0) {
+ flow = rcu_dereference_raw(*slot);
+ wake_up_interruptible_all(&flow->resume_tx);
+ }
+ mutex_unlock(&node->qrtr_tx_lock);
+
+ /* Translate DEL_PROC to BYE for local enqueue */
+ cb->type = QRTR_TYPE_BYE;
+ pkt = (struct qrtr_ctrl_pkt *)skb->data;
+ memset(pkt, 0, sizeof(*pkt));
+ pkt->cmd = cpu_to_le32(QRTR_TYPE_BYE);
+
+ if (sock_queue_rcv_skb(&ipc->sk, skb))
+ kfree_skb(skb);
+
+ qrtr_port_put(ipc);
+ }
+ }
+}
+
/**
* qrtr_endpoint_register() - register a new endpoint
* @ep: endpoint to register
@@ -599,6 +656,14 @@ int qrtr_endpoint_register(struct qrtr_endpoint *ep, unsigned int nid)
node->nid = QRTR_EP_NID_AUTO;
node->ep = ep;
+ kthread_init_work(&node->read_data, qrtr_node_rx_work);
+ kthread_init_worker(&node->kworker);
+ node->task = kthread_run(kthread_worker_fn, &node->kworker, "qrtr_rx");
+ if (IS_ERR(node->task)) {
+ kfree(node);
+ return -ENOMEM;
+ }
+
INIT_RADIX_TREE(&node->qrtr_tx_flow, GFP_KERNEL);
mutex_init(&node->qrtr_tx_lock);
--
2.34.1
On 9/19/2023 10:33 PM, Sricharan Ramabadhran wrote:
> @@ -122,6 +123,9 @@ static DEFINE_XARRAY_ALLOC(qrtr_ports);
> * @qrtr_tx_lock: lock for qrtr_tx_flow inserts
> * @rx_queue: receive queue
> * @item: list item for broadcast list
> + * @kworker: worker thread for recv work
> + * @task: task to run the worker thread
> + * @read_data: scheduled work for recv work
I think I made these descriptions a bit ambiguous with "recv work".
Since we are only parsing DEL_PROC messages at the moment, the
descriptions should be more accurate on what they are for.
> */
> struct qrtr_node {
> struct mutex ep_lock;
> @@ -134,6 +138,9 @@ struct qrtr_node {
>
> struct sk_buff_head rx_queue;
> struct list_head item;
> + struct kthread_worker kworker;
> + struct task_struct *task;
> + struct kthread_work read_data;
I think our own kthread here might have been overkill. I forget why we
needed it instead of using a workqueue.
> + if (cb->type == QRTR_TYPE_DEL_PROC) {
> + /* Free tx flow counters */
> + mutex_lock(&node->qrtr_tx_lock);
> + radix_tree_for_each_slot(slot, &node->qrtr_tx_flow, &iter, 0) {
> + flow = rcu_dereference_raw(*slot);
> + wake_up_interruptible_all(&flow->resume_tx);
> + }
> + mutex_unlock(&node->qrtr_tx_lock);
> +
I don't see any other places where we use rcu_dereference_raw for the
flow. Does this need to be updated for the rest of the places we get the
flow?
The same loop is done in qrtr_endpoint_unregister() so maybe we should
look into adding a helper for this logic?
> + /* Translate DEL_PROC to BYE for local enqueue */
> + cb->type = QRTR_TYPE_BYE;
> + pkt = (struct qrtr_ctrl_pkt *)skb->data;
> + memset(pkt, 0, sizeof(*pkt));
> + pkt->cmd = cpu_to_le32(QRTR_TYPE_BYE);
> +
Are we relying on the remote to program the destination of this packet
to be the control port?
Thanks,
Chris
On 9/21/2023 5:56 AM, Chris Lew wrote:
>
> On 9/19/2023 10:33 PM, Sricharan Ramabadhran wrote:
>
>> @@ -122,6 +123,9 @@ static DEFINE_XARRAY_ALLOC(qrtr_ports);
>> * @qrtr_tx_lock: lock for qrtr_tx_flow inserts
>> * @rx_queue: receive queue
>> * @item: list item for broadcast list
>> + * @kworker: worker thread for recv work
>> + * @task: task to run the worker thread
>> + * @read_data: scheduled work for recv work
>
> I think I made these descriptions a bit ambiguous with "recv work".
> Since we are only parsing DEL_PROC messages at the moment, the
> descriptions should be more accurate on what they are for.
ok, btw, then would keep your authorship in first place.
In our downstream, there were multiple changes around here and
could not get a clear author here. I fixed this while testing
with Modem SSR recently for our tree, that said, will fix it
next version.
>
>> */
>> struct qrtr_node {
>> struct mutex ep_lock;
>> @@ -134,6 +138,9 @@ struct qrtr_node {
>> struct sk_buff_head rx_queue;
>> struct list_head item;
>> + struct kthread_worker kworker;
>> + struct task_struct *task;
>> + struct kthread_work read_data;
>
> I think our own kthread here might have been overkill. I forget why we
> needed it instead of using a workqueue.
I added a workqueue here because endpoint post is getting called from
atomic contexts and below DEL_PROC handling acquires qrtr_tx_lock.
>
>> + if (cb->type == QRTR_TYPE_DEL_PROC) {
>> + /* Free tx flow counters */
>> + mutex_lock(&node->qrtr_tx_lock);
>> + radix_tree_for_each_slot(slot, &node->qrtr_tx_flow,
>> &iter, 0) {
>> + flow = rcu_dereference_raw(*slot);
>> + wake_up_interruptible_all(&flow->resume_tx);
>> + }
>> + mutex_unlock(&node->qrtr_tx_lock);
>> +
>
> I don't see any other places where we use rcu_dereference_raw for the
> flow. Does this need to be updated for the rest of the places we get the
> flow?
>
Yes, without the rcu_dereference_raw there is a SPARSE warning.
For some reason, did not see the same in other places where flow is
de-referenced. That said, yeah, will pull this common code and
create a new helper.
> The same loop is done in qrtr_endpoint_unregister() so maybe we should
> look into adding a helper for this logic?
>
>> + /* Translate DEL_PROC to BYE for local enqueue */
>> + cb->type = QRTR_TYPE_BYE;
>> + pkt = (struct qrtr_ctrl_pkt *)skb->data;
>> + memset(pkt, 0, sizeof(*pkt));
>> + pkt->cmd = cpu_to_le32(QRTR_TYPE_BYE);
>> +
>
> Are we relying on the remote to program the destination of this packet
> to be the control port?
>
Yeah, targets like SDX modems, have a qrtr_fwd_del_proc in the
endpoint_unregister path.
Regards,
Sricharan
On 9/23/2023 6:49 PM, Sricharan Ramabadhran wrote:
>>> */
>>> struct qrtr_node {
>>> struct mutex ep_lock;
>>> @@ -134,6 +138,9 @@ struct qrtr_node {
>>> struct sk_buff_head rx_queue;
>>> struct list_head item;
>>> + struct kthread_worker kworker;
>>> + struct task_struct *task;
>>> + struct kthread_work read_data;
>>
>> I think our own kthread here might have been overkill. I forget why we
>> needed it instead of using a workqueue.
>
> I added a workqueue here because endpoint post is getting called from
> atomic contexts and below DEL_PROC handling acquires qrtr_tx_lock.
Got it, I think deferring the processing makes sense. I was more
focusing on the fact that we are creating our own kthread instead of
using the system workqueues.
Prior to commit e04df98adf7d ("net: qrtr: Remove receive worker"), this
was a work_struct. I think we should keep it as a work_struct until we
can motivate why a qrtr_node needs it's own kthread.