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

[netdev-bot+sashiko@kernel.org]

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