[PATCH net-next 0/2] net: qrtr: Few qrtr fixes

[PATCH net-next 0/2] net: qrtr: Few qrtr fixes

[Sricharan Ramabadhran]

Patch #1 fixes a race condition between qrtr driver and ns opening and
sending data to a control port.

Patch #2 address the issue with legacy targets sending the SSR
notifications using DEL_PROC control message.

Sricharan Ramabadhran (1):
  net: qrtr: Add support for processing DEL_PROC type control message

Vignesh Viswanathan (1):
  net: qrtr: Prevent stale ports from sending

 include/uapi/linux/qrtr.h |  1 +
 net/qrtr/af_qrtr.c        | 75 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)

-- 
2.7.4

[PATCH net-next 1/2] net: qrtr: Prevent stale ports from sending

[Sricharan Ramabadhran]

From: Vignesh Viswanathan <quic_viswanat@quicinc.com>

If qrtr and some other process try to bind to the QMI Control port at
the same time, NEW_SERVER might come before ENETRESET is given to the
socket. This might cause a socket down/up when ENETRESET is received as
per the protocol and this triggers a DEL_SERVER and a second NEW_SERVER.

In order to prevent such messages from stale sockets being sent, check
if ENETRESET has been set on the socket and drop the packet.

Signed-off-by: Chris Lew <quic_clew@quicinc.com>
Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
---
 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 41ece61..26197a0 100644
--- a/net/qrtr/af_qrtr.c
+++ b/net/qrtr/af_qrtr.c
@@ -851,6 +851,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 */
@@ -860,6 +861,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;
-- 
2.7.4

Re: [PATCH net-next 1/2] net: qrtr: Prevent stale ports from sending

[Bjorn Andersson]

On Fri, Sep 01, 2023 at 03:50:20PM +0530, Sricharan Ramabadhran wrote:
> From: Vignesh Viswanathan <quic_viswanat@quicinc.com>
> 
> If qrtr and some other process try to bind to the QMI Control port at

It's unclear to me which "qrtr" is being referred here, could it be
"qrtr-ns", if so could we express that as "the name server".

We only allow one bind on the qrtr control port, so could it be that
"QMI Control port" refer to the control socket in the userspace QC[CS]I
libraries, if so that's just any random socket sending out a control
message.

Can we please rephrase this problem description to make the chain of
events clear?

> the same time, NEW_SERVER might come before ENETRESET is given to the
> socket. This might cause a socket down/up when ENETRESET is received as
> per the protocol and this triggers a DEL_SERVER and a second NEW_SERVER.
> 
> In order to prevent such messages from stale sockets being sent, check
> if ENETRESET has been set on the socket and drop the packet.
> 
> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
> Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>

The first person to certify the patch's origin, must be the author, and
when you pick the patch to send it you need to add your s-o-b.

So please fix the author, and add your s-o-b.


Let's add Chris to the recipients list as well.

> ---
>  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 41ece61..26197a0 100644
> --- a/net/qrtr/af_qrtr.c
> +++ b/net/qrtr/af_qrtr.c
> @@ -851,6 +851,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 */
> @@ -860,6 +861,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;

Isn't this line unnecessary?

Regards,
Bjorn

> +		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.7.4
> 

Re: [PATCH net-next 1/2] net: qrtr: Prevent stale ports from sending

[Sricharan Ramabadhran]

Hi Bjorn,

On 9/1/2023 7:41 PM, Bjorn Andersson wrote:
> On Fri, Sep 01, 2023 at 03:50:20PM +0530, Sricharan Ramabadhran wrote:
>> From: Vignesh Viswanathan <quic_viswanat@quicinc.com>
>>
>> If qrtr and some other process try to bind to the QMI Control port at
> 
> It's unclear to me which "qrtr" is being referred here, could it be
> "qrtr-ns", if so could we express that as "the name server".
> 

  yes, its name-space server. Will put it explicitly.

> We only allow one bind on the qrtr control port, so could it be that
> "QMI Control port" refer to the control socket in the userspace QC[CS]I
> libraries, if so that's just any random socket sending out a control
> message.
> 
> Can we please rephrase this problem description to make the chain of
> events clear?
> 

   In this case we are talking about a client connecting/sending to QRTR
   socket and the 'NS' doing a qrtr_bind during its init. There is
   possibility that a client tries to send to the 'NS' before  processing
   the ENETRESET. In the case of a NEW_SERVER 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.

   Also about the patch #2, i guess QRTR_BYE/DEL_PROC should also be
   broadcasted, right now we are only listening on DEL_PROC sent by
   legacy kernels like SDX modems. Without that modem SSR feature is
   broken on IPQ + SDX targets.

>> the same time, NEW_SERVER might come before ENETRESET is given to the
>> socket. This might cause a socket down/up when ENETRESET is received as
>> per the protocol and this triggers a DEL_SERVER and a second NEW_SERVER.
>>
>> In order to prevent such messages from stale sockets being sent, check
>> if ENETRESET has been set on the socket and drop the packet.
>>
>> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
>> Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
> 
> The first person to certify the patch's origin, must be the author, and
> when you pick the patch to send it you need to add your s-o-b.
> 
> So please fix the author, and add your s-o-b.
> 

  ok sure, will fix.

> 
> Let's add Chris to the recipients list as well.
> 

  ok.

>> ---
>>   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 41ece61..26197a0 100644
>> --- a/net/qrtr/af_qrtr.c
>> +++ b/net/qrtr/af_qrtr.c
>> @@ -851,6 +851,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 */
>> @@ -860,6 +861,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;
> 
> Isn't this line unnecessary?
> 

  yup, will be removed in V2.

Regards,
  Sricharan

[PATCH net-next 2/2] net: qrtr: Add support for processing DEL_PROC type control message

[Sricharan Ramabadhran]

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 <quic_srichara@quicinc.com>
---
Right now DEL_PROC is sent only by some legacy targets, latest uses
only _BYE signalling for local observers only. So later that needs to
be changed to broadcast and do the same DEL_PROC processing.

 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 f7e2fb3..1c92015 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 26197a0..426cea0 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 qrtr_ctrl_pkt *pkt;
+	void __rcu **slot;
+	struct radix_tree_iter iter;
+	struct qrtr_tx_flow *flow;
+	struct sk_buff *skb;
+	struct qrtr_sock *ipc;
+
+	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 = *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.7.4

Re: [PATCH net-next 0/2] net: qrtr: Few qrtr fixes

[Simon Horman]

On Fri, Sep 01, 2023 at 03:50:19PM +0530, Sricharan Ramabadhran wrote:
> Patch #1 fixes a race condition between qrtr driver and ns opening and
> sending data to a control port.
> 
> Patch #2 address the issue with legacy targets sending the SSR
> notifications using DEL_PROC control message.

Hi Sricharan,

if these are fixes then they should be targeted at 'net' rather than
'net-next', and consideration should be given to supplying Fixes tags.

If these are not fixes, then please don't describe them as such.
In this case targeting net-next is correct, but it is currently closed,
as per the form letter below.

In either case please consider:

* Arranging local variables for new Networking code in
  reverse xmas tree order - longest line to shortest

* Avoiding introducing new Sparse warnings


## Form letter - net-next-closed

The merge window for v6.6 has begun and therefore net-next is closed
for new drivers, features, code refactoring and optimizations.
We are currently accepting bug fixes only.

Please repost when net-next reopens after Sept 11th.

RFC patches sent for review only are obviously welcome at any time.

See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle
--
pw-bot: defer

Re: [PATCH net-next 0/2] net: qrtr: Few qrtr fixes

[Sricharan Ramabadhran]

Hi Simon,

On 9/1/2023 7:49 PM, Simon Horman wrote:
> On Fri, Sep 01, 2023 at 03:50:19PM +0530, Sricharan Ramabadhran wrote:
>> Patch #1 fixes a race condition between qrtr driver and ns opening and
>> sending data to a control port.
>>
>> Patch #2 address the issue with legacy targets sending the SSR
>> notifications using DEL_PROC control message.
> 
> Hi Sricharan,
> 
> if these are fixes then they should be targeted at 'net' rather than
> 'net-next', and consideration should be given to supplying Fixes tags.
> 
  There is as such no existing feature broken without these 2 patches
  today. Then they might qualify as preparatory patches for adding
  some features.

> If these are not fixes, then please don't describe them as such.
> In this case targeting net-next is correct, but it is currently closed,
> as per the form letter below.
> 
   Sure, then in that case looks like it belongs to 'net-next' and will
   post V2 once the net-next is opened again.

> In either case please consider:
> 
> * Arranging local variables for new Networking code in
>    reverse xmas tree order - longest line to shortest
> 
> * Avoiding introducing new Sparse warnings
> 
>   ok.

> ## Form letter - net-next-closed
> 
> The merge window for v6.6 has begun and therefore net-next is closed
> for new drivers, features, code refactoring and optimizations.
> We are currently accepting bug fixes only.
> 
> Please repost when net-next reopens after Sept 11th.
> 

  ok, sure.

Regards,
  Sricharan