[PATCH 0/2] net: qrtr: don't lose name server packets behind the HELLO gate

[PATCH 0/2] net: qrtr: don't lose name server packets behind the HELLO gate

[Igor Opaniuk]

Since commit 544d85de4dc2 ("net: qrtr: Send HELLO message on endpoint
register") the core refuses every non-HELLO packet to a node until the
HELLO to it has been sent, and retries a HELLO that cannot be sent yet
100 ms later. A freshly booted remoteproc has no glink rx intents, so
the first HELLO fails, and the name server's announcements in that
window (the replies to the remote's HELLO and the NEW_SERVER broadcasts
of local servers) are dropped for good. The name server then also skips
the lookup notifications for a server whose broadcast failed.

On QCM2290 (Arduino UNO Q) this stalls the modem in about one boot in
five: it waits for the RMTFS service, which rmtfs registers in the same
instant the modem starts, until its watchdog fires 40 s later; WiFi,
whose firmware runs on the modem, comes up a minute late.

Patch 1 makes the name server carry on after a failed send (announce
the remaining servers, notify the pending lookups). Patch 2 holds
control packets for a node behind its HELLO and sends them right after
it, instead of dropping them; data packets are still refused before the
HELLO, so flow control is unchanged.

Initially tested on the Arduino UNO Q (QRB2210) with v7.3-rc6 plus the board
support it needs: 10 fresh boots and 10 modem stop/start cycles, no
"failed to announce new service", no modem watchdog, WLAN firmware up
6.4-8.2 s after boot every time; unpatched, 2/10 boots and 1/18 restarts
stalled. Built with W=1 on net/main; applies to next-20261016.

I'm not deeply familiar with QRTR; an alternative would be to have the
core trigger announce_servers() for the node once its HELLO is sent.
Happy to rework it that way if preferred.

Cc: Chris Lew <christopher.lew@oss.qualcomm.com>

---
Igor Opaniuk (2):
      net: qrtr: ns: keep going when an announcement cannot be sent
      net: qrtr: hold control packets behind the HELLO gate instead of dropping them

 net/qrtr/af_qrtr.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++-------
 net/qrtr/ns.c      | 10 +++++----
 2 files changed, 58 insertions(+), 11 deletions(-)
---
base-commit: 07bd391eb9572e74f073c49acef327611234f9bb
change-id: 20261019-upstream-qrtr-hello-gate-fix_b4-c2e8c2f4f906

Best regards,
--  
Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>

[PATCH 1/2] net: qrtr: ns: keep going when an announcement cannot be sent

[Igor Opaniuk]

announce_servers() answers a HELLO by announcing every local server to
the new node, and stops at the first send error: the node then never
learns about the servers after that one. ctrl_cmd_new_server() returns
before lookup_notify() when the broadcast of a new local server fails,
so lookups already registered for that service, including the ones of
remote nodes, are never told the server exists.

A send failure to one node says nothing about the others. Log it and
carry on in both places.

Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
---
 net/qrtr/ns.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
index bcb090ee79d4..dfc2cdad688f 100644
--- a/net/qrtr/ns.c
+++ b/net/qrtr/ns.c
@@ -232,7 +232,7 @@ static int announce_servers(struct sockaddr_qrtr *sq)
 				continue;
 
 			pr_err("failed to announce new service\n");
-			return ret;
+			/* the remote needs the other servers no less */
 		}
 	}
 	return 0;
@@ -488,10 +488,12 @@ static int ctrl_cmd_new_server(struct sockaddr_qrtr *from,
 
 	if (srv->node == qrtr_ns.local_node) {
 		ret = service_announce_new(&qrtr_ns.bcast_sq, srv);
-		if (ret < 0) {
+		/*
+		 * A failed broadcast must not hide the server from the
+		 * lookups already registered for it.
+		 */
+		if (ret < 0)
 			pr_err("failed to announce new service\n");
-			return ret;
-		}
 	}
 
 	/* Notify any potential lookups about the new server */

-- 
2.53.0

[PATCH 2/2] net: qrtr: hold control packets behind the HELLO gate instead of dropping them

[Igor Opaniuk]

Since commit 544d85de4dc2 ("net: qrtr: Send HELLO message on endpoint
register") qrtr_node_enqueue() refuses every non-HELLO packet to a node
with -EAGAIN until the HELLO to that node has been sent, and a HELLO
that cannot be sent yet is retried 100 ms later. The name server does
not retry, so whatever it sends in that window is lost: the replies to
the remote's HELLO (announce_servers()) and the NEW_SERVER broadcasts of
local servers registering at the same time.

A freshly booted remoteproc hits this every time: its glink channel has
no rx intent yet, so our HELLO fails once. On the QCM2290 (Arduino UNO Q)
rmtfs and tqftpserv register in the same instant as the modem starts;
when the RMTFS announcement falls into the window the modem waits for a
service that exists until its watchdog fires 40 s later ("DOG detects
stalled initialization"), about one boot in five, and WiFi (whose
firmware runs on the modem) comes up a minute late.

Keep refusing data packets (they would count against the flow control
window), but queue control packets for a node whose HELLO is not out
yet, bounded to 64, and send them in order right after the HELLO
succeeds. Purge the queue when the endpoint goes away.

Fixes: 544d85de4dc2 ("net: qrtr: Send HELLO message on endpoint register")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
---
 net/qrtr/af_qrtr.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 52 insertions(+), 7 deletions(-)

diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
index e7b3647424b8..071fa980fae2 100644
--- a/net/qrtr/af_qrtr.c
+++ b/net/qrtr/af_qrtr.c
@@ -122,6 +122,7 @@ static DEFINE_XARRAY_ALLOC(qrtr_ports);
  * @qrtr_tx_flow: xarray of qrtr_tx_flow, keyed by node << 32 | port
  * @qrtr_tx_lock: lock for qrtr_tx_flow inserts
  * @hello_sent: hello packet send successful
+ * @tx_backlog: control packets waiting for the hello to be sent
  * @rx_queue: receive queue
  * @item: list item for broadcast list
  * @say_hello: delayed work for sending hello packet
@@ -136,6 +137,7 @@ struct qrtr_node {
 	struct mutex qrtr_tx_lock; /* for qrtr_tx_flow */
 
 	bool hello_sent;
+	struct sk_buff_head tx_backlog;
 
 	struct sk_buff_head rx_queue;
 	struct list_head item;
@@ -195,6 +197,7 @@ static void __qrtr_node_release(struct kref *kref)
 
 	cancel_delayed_work_sync(&node->say_hello);
 
+	skb_queue_purge(&node->tx_backlog);
 	skb_queue_purge(&node->rx_queue);
 
 	/* Free tx flow counters */
@@ -340,6 +343,26 @@ static void qrtr_tx_flow_failed(struct qrtr_node *node, int dest_node,
 	}
 }
 
+/*
+ * Control packets for a node whose HELLO has not gone out yet are held back
+ * and sent right after it; this bounds how many.
+ */
+#define QRTR_TX_BACKLOG_MAX	64
+
+/* Send the control packets held back until the HELLO, in order. */
+static void qrtr_node_flush_backlog(struct qrtr_node *node)
+{
+	struct sk_buff *skb;
+
+	lockdep_assert_held(&node->ep_lock);
+
+	while ((skb = skb_dequeue(&node->tx_backlog))) {
+		if (node->ep->xmit(node->ep, skb))
+			pr_warn_ratelimited("qrtr: held control packet to node %u lost\n",
+					    node->nid);
+	}
+}
+
 /* Pass an outgoing packet socket buffer to the endpoint driver. */
 static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 			     int type, struct sockaddr_qrtr *from,
@@ -349,8 +372,12 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 	size_t len = skb->len;
 	int rc, confirm_rx;
 
+	/*
+	 * Data before the HELLO is refused: it would count against the flow
+	 * control window. Control packets are held back further down.
+	 */
 	mutex_lock(&node->ep_lock);
-	if (!node->hello_sent && type != QRTR_TYPE_HELLO) {
+	if (!node->hello_sent && type == QRTR_TYPE_DATA) {
 		mutex_unlock(&node->ep_lock);
 		kfree_skb(skb);
 		return -EAGAIN;
@@ -383,13 +410,29 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 
 	if (!rc) {
 		mutex_lock(&node->ep_lock);
-		rc = -ENODEV;
-		if (node->ep)
-			rc = node->ep->xmit(node->ep, skb);
-		else
+		if (!node->ep) {
 			kfree_skb(skb);
-		if (!rc && type == QRTR_TYPE_HELLO)
-			node->hello_sent = true;
+			rc = -ENODEV;
+		} else if (!node->hello_sent && type != QRTR_TYPE_HELLO) {
+			/*
+			 * The remote may not take traffic yet (a freshly
+			 * booted remoteproc without rx intents). Dropping
+			 * here would lose name server announcements for
+			 * good, so hold them until the HELLO is out.
+			 */
+			if (skb_queue_len(&node->tx_backlog) < QRTR_TX_BACKLOG_MAX) {
+				skb_queue_tail(&node->tx_backlog, skb);
+			} else {
+				kfree_skb(skb);
+				rc = -EAGAIN;
+			}
+		} else {
+			rc = node->ep->xmit(node->ep, skb);
+			if (!rc && type == QRTR_TYPE_HELLO) {
+				node->hello_sent = true;
+				qrtr_node_flush_backlog(node);
+			}
+		}
 		mutex_unlock(&node->ep_lock);
 	}
 	/* Need to ensure that a subsequent message carries the otherwise lost
@@ -657,6 +700,7 @@ int qrtr_endpoint_register(struct qrtr_endpoint *ep, unsigned int nid)
 
 	kref_init(&node->ref);
 	mutex_init(&node->ep_lock);
+	skb_queue_head_init(&node->tx_backlog);
 	skb_queue_head_init(&node->rx_queue);
 	node->nid = QRTR_EP_NID_AUTO;
 	node->ep = ep;
@@ -700,6 +744,7 @@ void qrtr_endpoint_unregister(struct qrtr_endpoint *ep)
 
 	mutex_lock(&node->ep_lock);
 	node->ep = NULL;
+	skb_queue_purge(&node->tx_backlog);
 	mutex_unlock(&node->ep_lock);
 
 	/* Notify the local controller about the event */

-- 
2.53.0

Re: [PATCH 1/2] net: qrtr: ns: keep going when an announcement cannot be sent

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 (1) · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Several statements in the commit message do not match the
  code, and they overstate what the patch fixes and why.
- [Medium] announce_servers() (net/qrtr/ns.c:228-237) no longer stops at
  the first failed send.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the patch
  leaves it next to the code it changes.
- [Low] This problem wasn't introduced by this patch, but it limits what
  the ctrl_cmd_new_server() change can achieve.

--

net: qrtr: ns: keep going when an announcement cannot be sent

This patch changes announce_servers() so it logs a failed announcement and
moves on to the next local server instead of returning. It also changes
ctrl_cmd_new_server() so a failed broadcast of a new local server no
longer skips the lookup_notify() loop.

> ctrl_cmd_new_server() returns
> before lookup_notify() when the broadcast of a new local server fails,
> so lookups already registered for that service, including the ones of
> remote nodes, are never told the server exists.
>
> A send failure to one node says nothing about the others. Log it and
> carry on in both places.

[Severity: Medium]
Can a remote node have a lookup in qrtr_ns.lookups at all? Lookups are
only added in ctrl_cmd_new_lookup(), and that function rejects
non-local senders:

net/qrtr/ns.c:ctrl_cmd_new_lookup() {
    ...
	/* Accept only local observers */
	if (from->sq_node != qrtr_ns.local_node)
		return -EINVAL;
    ...
}

So every lookup the patch now notifies seems to belong to a local
observer.

Does "a send failure to one node says nothing about the others" apply to
announce_servers()? Each service_announce_new(sq, srv) call in its
xa_for_each() loop goes to the same sq, the one node that sent HELLO. The
loop walks servers, not nodes.

The most likely failure there is the -EAGAIN that qrtr_node_enqueue()
returns before HELLO has gone out to that node:

net/qrtr/af_qrtr.c:qrtr_node_enqueue() {
    ...
	if (!node->hello_sent && type != QRTR_TYPE_HELLO) {
		mutex_unlock(&node->ep_lock);
		kfree_skb(skb);
		return -EAGAIN;
	}
    ...
}

In that case every later send fails the same way. With this patch alone,
would the node still learn about none of the servers?

For ctrl_cmd_new_server(), qrtr_bcast_enqueue() always returns 0. That
leaves errors raised in qrtr_sendmsg() before enqueue, which in practice
means -ENOMEM from sock_alloc_send_skb(). Could the commit message say
that this half of the change only matters under allocation failure?

The next patch in the series, "net: qrtr: hold control packets behind the
HELLO gate instead of dropping them", only touches af_qrtr.c and does not
change any of the above. Since this patch has a Fixes: tag, could the
description be made more accurate?

> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> index bcb090ee79d49..dfc2cdad688f7 100644
> --- a/net/qrtr/ns.c
> +++ b/net/qrtr/ns.c
> @@ -232,7 +232,7 @@ static int announce_servers(struct sockaddr_qrtr *sq)
>  				continue;
>  
>  			pr_err("failed to announce new service\n");
> -			return ret;
> +			/* the remote needs the other servers no less */
>  		}
>  	}
>  	return 0;

[Severity: Medium]
With the return removed, announce_servers() walks every local server, up
to QRTR_NS_MAX_SERVERS (256). It calls the non-ratelimited pr_err() once
for each failed send. All sends go to the same sq, so a failure tied to
that destination happens again on every iteration.

Can one HELLO now produce up to 256 KERN_ERR lines, where before it
produced at most one?

An unprivileged local user in init_net seems able to trigger this.
qrtr_create() has no capability check. qrtr_ns_worker() dispatches on the
cmd in the payload, and ctrl_cmd_hello() checks neither origin nor
privilege before calling announce_servers().

Suppose a socket shrinks SO_RCVBUF, never reads, and sends a payload with
cmd QRTR_TYPE_HELLO to (local_node, QRTR_PORT_CTRL). Once its queue is
full, each reply is rejected in qrtr_local_enqueue():

net/qrtr/af_qrtr.c:qrtr_local_enqueue() {
    ...
	if (sock_queue_rcv_skb(&ipc->sk, skb)) {
		qrtr_port_put(ipc);
		kfree_skb(skb);
		return -ENOSPC;
	}
    ...
}

The -ENODEV check does not filter -ENOSPC, so every server logs a line.
The user can also raise the count by registering servers from other
sockets.

The same flood happens on the normal path when a remote HELLO arrives
before the local HELLO has gone out to that node, because
qrtr_node_enqueue() returns -EAGAIN for each announcement. The last patch
in the series turns that -EAGAIN into a backlog of at most 64 packets.
That narrows this path, but it still floods with more than 64 servers.
The -ENOSPC path is unchanged at the end of the series.

Would pr_err_ratelimited(), or a single message after the loop, work
better here?

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch.
ctrl_cmd_bye() and ctrl_cmd_del_client() still stop at the first error,
and in those loops each iteration goes to a different local port:

net/qrtr/ns.c:ctrl_cmd_bye() {
    ...
	xa_for_each(&local_node->servers, index, srv) {
		...
		ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
		if (ret < 0 && ret != -ENODEV) {
			pr_err("failed to send bye cmd\n");
			goto delete_node;
		}
	}
    ...
}

ctrl_cmd_del_client() does the same thing with "return ret".

qrtr_local_enqueue() returns -ENOSPC when a single receiver's queue is
full. Can one local server that never reads its socket then prevent every
server after it, in port order, from learning that a remote node or
client has gone away?

An unprivileged user can set this up, since anyone can bind ports
>= QRTR_MIN_EPH_SOCKET. The later patch in the series, "net: qrtr: hold
control packets behind the HELLO gate instead of dropping them", does not
change these loops.

Should these loops also log and continue?

> @@ -488,10 +488,12 @@ static int ctrl_cmd_new_server(struct sockaddr_qrtr *from,
>  
>  	if (srv->node == qrtr_ns.local_node) {
>  		ret = service_announce_new(&qrtr_ns.bcast_sq, srv);
> -		if (ret < 0) {
> +		/*
> +		 * A failed broadcast must not hide the server from the
> +		 * lookups already registered for it.
> +		 */
> +		if (ret < 0)
>  			pr_err("failed to announce new service\n");
> -			return ret;
> -		}
>  	}
>  
>  	/* Notify any potential lookups about the new server */

[Severity: Low]
This isn't a bug introduced by this patch, but qrtr_bcast_enqueue()
limits which errors this check can ever see:

net/qrtr/af_qrtr.c:qrtr_bcast_enqueue() {
    ...
	list_for_each_entry(node, &qrtr_all_nodes, item) {
		...
		skbn = pskb_copy(skb, GFP_KERNEL);
		if (!skbn)
			break;
		skb_set_owner_w(skbn, skb->sk);
		qrtr_node_enqueue(node, skbn, type, from, to);
	}
	mutex_unlock(&qrtr_node_lock);

	qrtr_local_enqueue(NULL, skb, type, from, to);

	return 0;
}

The per-node results from qrtr_node_enqueue() and qrtr_local_enqueue()
are ignored. A pskb_copy() failure skips all the remaining remote nodes,
and the function still returns 0.

That means the ret < 0 check here only sees errors from qrtr_sendmsg()
before enqueue, mainly -ENOMEM from sock_alloc_send_skb().

Under memory pressure, can the remote nodes after a failed pskb_copy()
miss this NEW_SERVER broadcast without ns.c ever finding out?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019-upstream-qrtr-hello-gate-fix_b4-v1-0-bd420aec171b%40oss.qualcomm.com

Re: [PATCH 2/2] net: qrtr: hold control packets behind the HELLO gate instead of dropping them

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In net/qrtr/af_qrtr.c, held socket buffers keep their owner
  socket (skb->sk) and do not take a reference on it.
- [Medium] The HELLO is only re-armed when qrtr_node_enqueue() returns
  -EAGAIN (`if (rc == -EAGAIN && type == QRTR_TYPE_HELLO)…
- [Medium] qrtr_node_flush_backlog() (af_qrtr.c:353-364) sends each held
  control packet once.
- [Low] The check that refuses DATA packets before the HELLO
  (af_qrtr.c:379-385) and the later decision to queue into the backlog…
- [Low] qrtr_node_flush_backlog() sends up to 64 queued packets through
  node->ep->xmit() in one ep_lock critical section, the same one that…

--

net: qrtr: hold control packets behind the HELLO gate instead of dropping them

With this commit, control packets sent to a node whose HELLO has not gone
out yet are put on a new per-node tx_backlog, bounded to 64 entries.
Before, they were refused with -EAGAIN. The backlog is flushed right after
the HELLO succeeds and purged when the endpoint goes away, and data
packets are still refused before the HELLO.

> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
> index e7b3647424b8f..071fa980fae2e 100644
> --- a/net/qrtr/af_qrtr.c
> +++ b/net/qrtr/af_qrtr.c

[ ... ]

> @@ -340,6 +343,26 @@ static void qrtr_tx_flow_failed(struct qrtr_node *node, int dest_node,
>  	}
>  }
>  
> +/*
> + * Control packets for a node whose HELLO has not gone out yet are held back
> + * and sent right after it; this bounds how many.
> + */
> +#define QRTR_TX_BACKLOG_MAX	64
> +
> +/* Send the control packets held back until the HELLO, in order. */
> +static void qrtr_node_flush_backlog(struct qrtr_node *node)
> +{
> +	struct sk_buff *skb;
> +
> +	lockdep_assert_held(&node->ep_lock);
> +
> +	while ((skb = skb_dequeue(&node->tx_backlog))) {
> +		if (node->ep->xmit(node->ep, skb))
> +			pr_warn_ratelimited("qrtr: held control packet to node %u lost\n",
> +					    node->nid);

[Severity: Medium]
If ep->xmit() fails here, the held packet is gone. The original sender got
rc == 0 when the packet was queued, so nobody retries it. The flush also
keeps going and qrtr_tx_flow_failed() is not called.

Are transient errors likely right after the HELLO? On glink the HELLO uses
up a remote rx intent. The next send can then go through
qcom_glink_request_intent(), which returns -ETIMEDOUT after 10 seconds or
-EAGAIN when the remote declines:

drivers/rpmsg/qcom_glink_native.c:qcom_glink_request_intent() {
    ...
	} else {
		ret = READ_ONCE(channel->intent_req_result) ? 0 : -EAGAIN;
	}
    ...
}

This patch retries the HELLO every 100 ms for that same condition.

On MHI, qcom_mhi_qrtr_send() uses the non-blocking mhi_queue_skb(), and
mhi_queue() fails once the ring is full:

drivers/bus/mhi/host/main.c:mhi_queue() {
    ...
	ret = mhi_is_ring_full(mhi_cntrl, tre_ring);
	if (unlikely(ret))
		return -EAGAIN;
    ...
}

pci_generic.c configures the IPCR UL channel with 8 elements:

	MHI_CHANNEL_CONFIG_UL(20, "IPCR", 8, 0),

Sending up to QRTR_TX_BACKLOG_MAX packets back to back can overflow that
ring.

Could the RMTFS NEW_SERVER announcement still be lost this way? Would it
make sense to stop the flush on error and leave the remaining packets
queued for a retry?

> +	}
> +}

[ ... ]

> @@ -349,8 +372,12 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
>  	size_t len = skb->len;
>  	int rc, confirm_rx;
>  
> +	/*
> +	 * Data before the HELLO is refused: it would count against the flow
> +	 * control window. Control packets are held back further down.
> +	 */
>  	mutex_lock(&node->ep_lock);
> -	if (!node->hello_sent && type != QRTR_TYPE_HELLO) {
> +	if (!node->hello_sent && type == QRTR_TYPE_DATA) {
>  		mutex_unlock(&node->ep_lock);
>  		kfree_skb(skb);
>  		return -EAGAIN;

[ ... ]

> @@ -383,13 +410,29 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
>  
>  	if (!rc) {
>  		mutex_lock(&node->ep_lock);
> -		rc = -ENODEV;
> -		if (node->ep)
> -			rc = node->ep->xmit(node->ep, skb);
> -		else
> +		if (!node->ep) {
>  			kfree_skb(skb);
> -		if (!rc && type == QRTR_TYPE_HELLO)
> -			node->hello_sent = true;
> +			rc = -ENODEV;
> +		} else if (!node->hello_sent && type != QRTR_TYPE_HELLO) {

[Severity: Low]
Can a DATA packet end up on tx_backlog here? The DATA check at the top of
qrtr_node_enqueue() and this check are in separate ep_lock sections. For
DATA, qrtr_tx_wait() can sleep between them.

Suppose qrtr_endpoint_hello() runs in that gap, as it does on the MHI
resume path:

qrtr_endpoint_hello()
	mutex_lock(&node->ep_lock);
	node->hello_sent = false;
	mutex_unlock(&node->ep_lock);

The DATA packet then matches this condition and is queued with rc == 0
instead of being refused. That contradicts "Keep refusing data packets" in
the commit message and the new comment above the first check.

That packet has already been counted in flow->pending and may carry
confirm_rx. If its xmit fails later in qrtr_node_flush_backlog(),
qrtr_tx_flow_failed() is never called. It also takes one of the 64 backlog
slots.

Should the DATA refusal be checked again here?

> +			/*
> +			 * The remote may not take traffic yet (a freshly
> +			 * booted remoteproc without rx intents). Dropping
> +			 * here would lose name server announcements for
> +			 * good, so hold them until the HELLO is out.
> +			 */
> +			if (skb_queue_len(&node->tx_backlog) < QRTR_TX_BACKLOG_MAX) {
> +				skb_queue_tail(&node->tx_backlog, skb);

[Severity: High]
Can the queued skb outlive the socket that owns it? The skb keeps skb->sk,
and no socket reference is taken before it goes on tx_backlog.

One path is closing a bound qrtr socket while a node with an assigned nid
still has hello_sent == false:

qrtr_release()
  qrtr_port_remove()
    skb_set_owner_w(skb, &ipc->sk);
    qrtr_bcast_enqueue()
      skb_set_owner_w(skbn, skb->sk);
      qrtr_node_enqueue()
        skb_queue_tail(&node->tx_backlog, skb);   <- returns 0
    __sock_put(&ipc->sk);
  sock_put(sk);                                   <- sk_refcnt reaches 0

>From then on, only the sk_wmem_alloc charge of the queued skb keeps the
struct sock in memory.

When the HELLO later succeeds, qrtr_node_flush_backlog() calls ep->xmit().
For MHI that is qcom_mhi_qrtr_send():

net/qrtr/mhi.c:qcom_mhi_qrtr_send() {
    ...
	if (skb->sk)
		sock_hold(skb->sk);
    ...
}

Wouldn't this hit the "refcount_t: addition on 0; use-after-free" warning
and saturate sk_refcnt?

The !hello_sent window can be open at three points:

- at probe, after the remote's first packet assigns the nid but before
  the HELLO succeeds
- while the HELLO is being retried every 100 ms
- after qrtr_endpoint_hello() sets hello_sent = false on MHI resume

Before this patch, these skbs were sent or freed synchronously while the
sender still held its references. Would calling skb_orphan() before
skb_queue_tail(), or holding a socket reference for each queued skb, avoid
this?

> +			} else {
> +				kfree_skb(skb);
> +				rc = -EAGAIN;
> +			}

[Severity: Medium]
What happens to the held packets if the HELLO fails with an error other
than -EAGAIN? The retry is only armed for -EAGAIN:

	if (rc == -EAGAIN && type == QRTR_TYPE_HELLO)
		schedule_delayed_work(&node->say_hello, msecs_to_jiffies(100));

qrtr_hello_work() also ignores the return value of qrtr_node_enqueue().

There are a few ways to get a different error. On glink,
qcom_glink_request_intent() returns -ETIMEDOUT when the remote does not
answer the intent request within 10 seconds. skb_put_padto() or
skb_linearize() can also fail with -ENOMEM.

hello_sent then stays false until the endpoint is unregistered or
qrtr_endpoint_hello() is called. Only mhi.c calls it, on resume.

The first 64 control packets to that node are queued with rc == 0 and
never sent. These include announce_servers() replies and the NEW_SERVER,
DEL_SERVER and DEL_CLIENT broadcasts.

Before this patch the senders got -EAGAIN, and ns.c logged "failed to
announce new service". Now the name server sees success for packets that
never go out.

The held skbs also stay charged to their senders' sk_wmem_alloc with no
time limit, and that includes the in-kernel name server socket.

Does the backlog depend on the -EAGAIN retry always succeeding in the end?

> +		} else {
> +			rc = node->ep->xmit(node->ep, skb);
> +			if (!rc && type == QRTR_TYPE_HELLO) {
> +				node->hello_sent = true;
> +				qrtr_node_flush_backlog(node);

[Severity: Low]
This sends up to 64 packets through ep->xmit() while still holding the
ep_lock taken for the HELLO. It runs from qrtr_hello_work() on the system
workqueue.

On glink each send can sleep up to 10 seconds, either in qcom_glink_tx()
waiting for FIFO space or in qcom_glink_request_intent(). The case this
patch targets is a remote that is alive but short of intents. Such a
remote could keep ep_lock held for a long time.

Every other sender to this node blocks on the first
mutex_lock(&node->ep_lock) in qrtr_node_enqueue() for the whole flush,
not for a single xmit as before. That includes the name server worker.

Teardown on glink is not blocked, because qcom_glink_native_remove() sets
abort_tx first, and MHI xmit does not block.

Is it intended to hold ep_lock for the whole drain?

> +			}
> +		}
>  		mutex_unlock(&node->ep_lock);
>  	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019-upstream-qrtr-hello-gate-fix_b4-v1-0-bd420aec171b%40oss.qualcomm.com