[PATCH net v2] net: qrtr: announce local services again once our HELLO is sent

[PATCH net v2] net: qrtr: announce local services again once our HELLO is sent

[Joonhoe Kim]

Since commit 544d85de4dc2 ("net: qrtr: Send HELLO message on endpoint
register"), qrtr_node_enqueue() drops every packet but HELLO with
-EAGAIN until our HELLO has gone out to the node. When the remote's
HELLO arrives first, the NEW_SERVER replies from the name service are
dropped, and so are the broadcasts for services registered before our
HELLO goes out. Nothing announces these services again, so the remote
never learns about them.

On Kaanapali the modem sends its HELLO before it accepts ours. With
rmtfs started first, the modem never learns about rmtfs and its
watchdog resets the SoC about 40 s later.

Remember that the remote has sent a HELLO. Once our HELLO has been
sent to such a node, pass a HELLO from it to the local name service
again, so that it announces the services it has at that point.

The flag is not cleared when qrtr_endpoint_hello() restarts the
handshake on resume, so a HELLO that the remote sent before then is
not lost. The cost is that the remote gets the same NEW_SERVER
announcements twice when its HELLO arrives before ours goes out but
the name service handles it after, and on every resume.

Fixes: 544d85de4dc2 ("net: qrtr: Send HELLO message on endpoint register")
Cc: stable@vger.kernel.org
Signed-off-by: Joonhoe Kim <26rote@gmail.com>
---
Changes in v2:
- Replay the remote's HELLO to the name service from the core once our
  HELLO is out, instead of retrying in announce_servers(). Replaying
  the announcements once hello_sent is set was suggested in the Sashiko
  review of v1. This is also not the delayed work in the name service
  that I mentioned to Mani: that would still miss the broadcasts
  dropped before our HELLO. Nothing sleeps in the name service worker
  and there is no time limit.
- v1: https://lore.kernel.org/all/20261016124600.31-1-26rote@gmail.com/

Tested on a Lenovo Legion Tab Y700 Gen 5 (out-of-tree board support)
with rmtfs started before the modem: the modem's HELLO is still handled
before ours is out ("failed to announce new service"), the replayed
HELLO follows right after, rmtfs gets the modem_fs1/2 opens, and the
modem stays up.

 net/qrtr/af_qrtr.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
index e7b364742..508a2dd2d 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
+ * @hello_rcvd: remote has sent a hello packet
  * @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;
+	bool hello_rcvd;
 
 	struct sk_buff_head rx_queue;
 	struct list_head item;
@@ -541,6 +543,11 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
 		qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
 	}
 
+	if (cb->type == QRTR_TYPE_HELLO) {
+		/* Pairs with smp_load_acquire() in qrtr_hello_work() */
+		smp_store_release(&node->hello_rcvd, true);
+	}
+
 	if (cb->type == QRTR_TYPE_RESUME_TX) {
 		qrtr_tx_resume(node, skb);
 	} else {
@@ -619,7 +626,20 @@ static void qrtr_hello_work(struct work_struct *work)
 	pkt->cmd = cpu_to_le32(QRTR_TYPE_HELLO);
 	from.sq_node = qrtr_local_nid;
 	to.sq_node = node->nid;
-	qrtr_node_enqueue(node, skb, QRTR_TYPE_HELLO, &from, &to);
+	if (qrtr_node_enqueue(node, skb, QRTR_TYPE_HELLO, &from, &to))
+		goto out;
+
+	/* Pairs with smp_store_release() in qrtr_endpoint_post() */
+	if (!smp_load_acquire(&node->hello_rcvd))
+		goto out;
+
+	skb = qrtr_alloc_ctrl_packet(&pkt, GFP_KERNEL);
+	if (skb) {
+		pkt->cmd = cpu_to_le32(QRTR_TYPE_HELLO);
+		to.sq_node = READ_ONCE(node->nid);
+		qrtr_local_enqueue(NULL, skb, QRTR_TYPE_HELLO, &to, &from);
+	}
+out:
 	qrtr_port_put(ctrl);
 }
 

base-commit: abeacca61eea4f6803df4a595b656ea45c2d3249
-- 
2.43.0

Re: [PATCH net v2] net: qrtr: announce local services again once our HELLO is sent

[Manivannan Sadhasivam]

On Fri, Oct 23, 2026 at 01:31:16AM +0900, Joonhoe Kim wrote:
> Since commit 544d85de4dc2 ("net: qrtr: Send HELLO message on endpoint
> register"), qrtr_node_enqueue() drops every packet but HELLO with
> -EAGAIN until our HELLO has gone out to the node. When the remote's
> HELLO arrives first, the NEW_SERVER replies from the name service are
> dropped, and so are the broadcasts for services registered before our
> HELLO goes out. Nothing announces these services again, so the remote
> never learns about them.
> 
> On Kaanapali the modem sends its HELLO before it accepts ours. With
> rmtfs started first, the modem never learns about rmtfs and its
> watchdog resets the SoC about 40 s later.
> 
> Remember that the remote has sent a HELLO. Once our HELLO has been
> sent to such a node, pass a HELLO from it to the local name service
> again, so that it announces the services it has at that point.
> 
> The flag is not cleared when qrtr_endpoint_hello() restarts the
> handshake on resume, so a HELLO that the remote sent before then is
> not lost. The cost is that the remote gets the same NEW_SERVER
> announcements twice when its HELLO arrives before ours goes out but
> the name service handles it after, and on every resume.
> 
> Fixes: 544d85de4dc2 ("net: qrtr: Send HELLO message on endpoint register")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joonhoe Kim <26rote@gmail.com>

This version looks good to me, although I left a few comments inline.

> ---
> Changes in v2:
> - Replay the remote's HELLO to the name service from the core once our
>   HELLO is out, instead of retrying in announce_servers(). Replaying
>   the announcements once hello_sent is set was suggested in the Sashiko
>   review of v1. This is also not the delayed work in the name service
>   that I mentioned to Mani: that would still miss the broadcasts
>   dropped before our HELLO. Nothing sleeps in the name service worker
>   and there is no time limit.
> - v1: https://lore.kernel.org/all/20261016124600.31-1-26rote@gmail.com/
> 
> Tested on a Lenovo Legion Tab Y700 Gen 5 (out-of-tree board support)
> with rmtfs started before the modem: the modem's HELLO is still handled
> before ours is out ("failed to announce new service"), the replayed
> HELLO follows right after, rmtfs gets the modem_fs1/2 opens, and the
> modem stays up.
> 
>  net/qrtr/af_qrtr.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
> index e7b364742..508a2dd2d 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
> + * @hello_rcvd: remote has sent a hello packet
>   * @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;
> +	bool hello_rcvd;
>  
>  	struct sk_buff_head rx_queue;
>  	struct list_head item;
> @@ -541,6 +543,11 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
>  		qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
>  	}
>  
> +	if (cb->type == QRTR_TYPE_HELLO) {
> +		/* Pairs with smp_load_acquire() in qrtr_hello_work() */
> +		smp_store_release(&node->hello_rcvd, true);

smp_store_{release/acquire}() accessors are only needed if there is an ordering
requirement. But I don't see that exist with this 'hello_rcvd' bool flag.
Wouldn't {READ/WRITE}_ONCE() suffice?

> +	}
> +
>  	if (cb->type == QRTR_TYPE_RESUME_TX) {
>  		qrtr_tx_resume(node, skb);
>  	} else {
> @@ -619,7 +626,20 @@ static void qrtr_hello_work(struct work_struct *work)
>  	pkt->cmd = cpu_to_le32(QRTR_TYPE_HELLO);
>  	from.sq_node = qrtr_local_nid;
>  	to.sq_node = node->nid;
> -	qrtr_node_enqueue(node, skb, QRTR_TYPE_HELLO, &from, &to);
> +	if (qrtr_node_enqueue(node, skb, QRTR_TYPE_HELLO, &from, &to))
> +		goto out;
> +
> +	/* Pairs with smp_store_release() in qrtr_endpoint_post() */
> +	if (!smp_load_acquire(&node->hello_rcvd))
> +		goto out;
> +
> +	skb = qrtr_alloc_ctrl_packet(&pkt, GFP_KERNEL);
> +	if (skb) {
> +		pkt->cmd = cpu_to_le32(QRTR_TYPE_HELLO);
> +		to.sq_node = READ_ONCE(node->nid);

There is no READ_ONCE() in the earlier read. So please justify why you added it.

> +		qrtr_local_enqueue(NULL, skb, QRTR_TYPE_HELLO, &to, &from);

What happens to the return value?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH net v2] net: qrtr: announce local services again once our HELLO is sent

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 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] In net/qrtr/af_qrtr.c, qrtr_hello_work() replays the HELLO
  once and never retries, so a failed replay is lost for good.
- [Low] In net/qrtr/af_qrtr.c, the new HELLO replay in qrtr_hello_work()
  is not serialized against qrtr_endpoint_unregister(), so the replayed…

--

net: qrtr: announce local services again once our HELLO is sent

The patch records that the remote node has sent a HELLO. Once our own
HELLO has gone out, qrtr_hello_work() passes a synthetic HELLO from that
node to the local name service. The name service then announces the
local services again, replacing the NEW_SERVER packets that
qrtr_node_enqueue() dropped with -EAGAIN before hello_sent was set.

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

[ ... ]

> @@ -619,7 +626,20 @@ static void qrtr_hello_work(struct work_struct *work)
>  	pkt->cmd = cpu_to_le32(QRTR_TYPE_HELLO);
>  	from.sq_node = qrtr_local_nid;
>  	to.sq_node = node->nid;
> -	qrtr_node_enqueue(node, skb, QRTR_TYPE_HELLO, &from, &to);
> +	if (qrtr_node_enqueue(node, skb, QRTR_TYPE_HELLO, &from, &to))
> +		goto out;
> +
> +	/* Pairs with smp_store_release() in qrtr_endpoint_post() */
> +	if (!smp_load_acquire(&node->hello_rcvd))
> +		goto out;
> +
> +	skb = qrtr_alloc_ctrl_packet(&pkt, GFP_KERNEL);
> +	if (skb) {
> +		pkt->cmd = cpu_to_le32(QRTR_TYPE_HELLO);
> +		to.sq_node = READ_ONCE(node->nid);
> +		qrtr_local_enqueue(NULL, skb, QRTR_TYPE_HELLO, &to, &from);
> +	}

[Severity: Medium]
What happens if this replay is lost? If qrtr_alloc_ctrl_packet() returns
NULL, the replay is skipped without any message.

The return value of qrtr_local_enqueue() is also ignored. That function
frees the skb and returns an error in two cases. It returns -ENODEV if
the CTRL port lookup fails. It returns -ENOSPC if sock_queue_rcv_skb()
rejects the skb because the name service socket's receive queue is full:

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;
	}
    ...
}

At this point hello_sent is already true. So this re-arm in
qrtr_node_enqueue() can no longer fire:

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

The work only runs again after qrtr_endpoint_hello() on resume.

Without the replayed HELLO, ctrl_cmd_hello()->announce_servers() never
runs. The NEW_SERVER packets dropped with -EAGAIN before hello_sent was
set then stay lost, which is the outcome the commit message describes
for rmtfs and the modem.

Earlier in the same function, the first allocation failure reschedules
say_hello:

	skb = qrtr_alloc_ctrl_packet(&pkt, GFP_KERNEL);
	if (!skb) {
		qrtr_port_put(ctrl);
		schedule_delayed_work(&node->say_hello, msecs_to_jiffies(100));
		return;
	}

Should a failed replay also be retried, or at least logged? The commit
message lists duplicate NEW_SERVER announcements as the only cost. It
does not say that the recovery is best effort.

[Severity: Low]
Can this replayed HELLO reach the name service after the node's BYE?

After qrtr_node_enqueue() returns 0, ep_lock is no longer held, and
nothing here checks node->ep again. The GFP_KERNEL allocation can also
sleep. A concurrent teardown could run like this:

CPU1                               CPU2
qrtr_hello_work()
  qrtr_node_enqueue()
    xmit HELLO
    hello_sent = true
                                   qrtr_endpoint_unregister()
                                     node->ep = NULL
                                     queue QRTR_TYPE_BYE to NS
                                     qrtr_node_release()
                                       radix entries removed
                                       cancel_delayed_work_sync()
  qrtr_alloc_ctrl_packet()
  qrtr_local_enqueue(HELLO)

The NS worker would handle the BYE first in ctrl_cmd_bye(), then the
stale HELLO. ctrl_cmd_hello()->announce_servers() would then send
NEW_SERVER for every local server to the dead node id.

qrtr_sendmsg() then fails at qrtr_node_lookup() with -ECONNRESET. That
is not -ENODEV, so announce_servers() logs "failed to announce new
service" and qrtr_ns_worker() logs "failed while handling packet". If a
new endpoint has already registered with the same node id, it may get
duplicate announcements.

cancel_delayed_work_sync() prevents a use-after-free, so the effect
looks limited to log noise and extra announcements. Before this patch,
qrtr_hello_work() never sent anything to the local name service, so
this reordering is new.

Should the replay be ordered against qrtr_endpoint_unregister()?

> +out:
>  	qrtr_port_put(ctrl);
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022163116.1042-1-26rote%40gmail.com

Re: [PATCH net v2] net: qrtr: announce local services again once our HELLO is sent

[Filippo Falezza]

Hello Joonhoe,

I ran into the same problem on a Motorola Moto G 5G Plus (SM7250),
which I'm bringing up on linux-next (next-20261021). Since the HELLO
gate went in, the name service's first announcement to the modem fails
with -EAGAIN on every boot, and so far I had been carrying a local
retry loop in ns.c to get rmtfs announced to it.

I dropped that loop, applied your v2 instead, and booted the phone
seven times in a row. The modem came up and registered on the network
on every boot, with rmtfs serving it, and the phone never reset.
"failed to announce new service" still shows up once for a remote
whose HELLO arrives before ours (one boot logged it for both the ADSP
and the modem, others once or not at all), and the modem carries on
normally afterwards, which matches the replay doing its job.

One thing I couldn't cover is a modem restart: on this phone it
resets the whole SoC with or without your patch, so that's a separate
problem here and says nothing about the patch.

I did the testing with the help of an AI coding assistant (Claude Code);
the results above are from the phone.

Tested-by: Filippo Falezza <effeffe99@gmail.com> # Motorola Moto G 5G Plus nairo (SM7250)

Thanks,
Filippo