Re: [PATCH net] net: qrtr: ns: retry service announcements on -EAGAIN

[netdev-bot+sashiko@kernel.org]

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

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

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] The fix only covers one of the two paths that send NEW_SERVER.
- [Medium] The new retry loop in announce_servers() (ns.c:232-236) can
  stall the whole name service for about 10 s per remote HELLO.
- [Medium] The NS side gives up after a fixed 10 s, but the core keeps
  retrying our HELLO with no limit.

Pre-existing issues:
- [Medium] A retry can re-advertise a service that was closed during the
  retry window, and the remote then keeps the stale entry.

--

net: qrtr: ns: retry service announcements on -EAGAIN

This patch makes announce_servers() retry service_announce_new() every
100 ms, for up to 10 s per server, while it returns -EAGAIN. The goal is
that NEW_SERVER replies to a remote's HELLO are not lost before our own
HELLO has gone out. It targets a modem watchdog reset on Kaanapali when
rmtfs registers before the modem comes up.

> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> index bcb090ee79d49..33a9c4449d6f8 100644
> --- a/net/qrtr/ns.c
> +++ b/net/qrtr/ns.c

[ ... ]

> @@ -226,7 +227,13 @@ static int announce_servers(struct sockaddr_qrtr *sq)
>  
>  	/* Announce the list of servers registered in this node */
>  	xa_for_each(&node->servers, index, srv) {
> +		unsigned long timeout = jiffies + 10 * HZ;
> +
>  		ret = service_announce_new(sq, srv);

[Severity: High]

This retries the unicast NEW_SERVER replies sent from announce_servers().
Can the broadcast path still lose announcements at the same HELLO gate?

Take a local service that registers after the remote's HELLO has been
handled, but before our HELLO has gone out. It is announced from
ctrl_cmd_new_server():

net/qrtr/ns.c:ctrl_cmd_new_server() {
    ...
	if (srv->node == qrtr_ns.local_node) {
		ret = service_announce_new(&qrtr_ns.bcast_sq, srv);
    ...
}

That reaches qrtr_bcast_enqueue(). It skips only nodes that have no nid
yet, and it ignores the return value of qrtr_node_enqueue():

net/qrtr/af_qrtr.c:qrtr_bcast_enqueue() {
    ...
		if (READ_ONCE(node->nid) == QRTR_EP_NID_AUTO)
			continue;
    ...
		qrtr_node_enqueue(node, skbn, type, from, to);
	}
    ...
	return 0;
}

qrtr_endpoint_post()->qrtr_node_assign() sets the nid when the remote's
first packet (its HELLO) arrives. From then on the node receives
broadcasts, even while hello_sent is still false. qrtr_node_enqueue()
then frees the NEW_SERVER and returns -EAGAIN:

	if (!node->hello_sent && type != QRTR_TYPE_HELLO) {
		mutex_unlock(&node->ep_lock);
		kfree_skb(skb);
		return -EAGAIN;
	}

ctrl_cmd_new_server() sees success. If node->servers was empty when the
remote's HELLO was handled, announce_servers() had nothing to retry.
Nothing calls announce_servers() again once hello_sent becomes true.

The same window opens again after qrtr_endpoint_hello() clears hello_sent
on resume, because the nid stays assigned. DEL_SERVER broadcasts from
server_del() are dropped the same way.

In that case, wouldn't rmtfs (or tqftpserv, pd-mapper) stay unannounced,
leading to the modem watchdog reset described in the commit message?

Could this be handled in the core instead of by polling in one caller?
For example, packets could be queued in qrtr_node_enqueue(), or the
announcements replayed once hello_sent becomes true.

> +		while (ret == -EAGAIN && time_before(jiffies, timeout)) {
> +			msleep(100);

[Severity: Medium]

Can this stall the whole name service for up to 10 s per remote HELLO?

announce_servers() is called from ctrl_cmd_hello() inside
qrtr_ns_worker(). That worker is the only reader of the NS socket, and
it runs as a single work item on an ordered workqueue:

	qrtr_ns.workqueue = alloc_ordered_workqueue("qrtr_ns_handler", 0);

While this loop sleeps, everything else waits behind it: local
NEW_SERVER and NEW_LOOKUP, plus the BYE, DEL_CLIENT and DEL_SERVER
cleanup for every other node and port. Stalls caused by several remotes
add up one after another.

hello_sent can stay false for a long time, or for good. When the remote
rejects an intent request, qcom_glink_request_intent() returns -EAGAIN.
qrtr_node_enqueue() reschedules say_hello only on -EAGAIN:

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

A HELLO that fails with -ETIMEDOUT or -ENOMEM leaves hello_sent false
for good. After that, every later HELLO from that node costs the full
10 s.

Control packets keep arriving through sock_queue_rcv_skb() while the
worker sleeps, from both qrtr_endpoint_post() and qrtr_local_enqueue().
Once sk_rcvbuf is full they are dropped, which can leave stale lookup
and server state. qrtr_ns_remove() also has to wait in
cancel_work_sync() for the sleep to finish.

The commit message only says the worker "can sleep". Could the retry be
moved out of the shared worker, or at least could the commit message
describe these effects?

> +			ret = service_announce_new(sq, srv);

[Severity: Medium]

This is a pre-existing issue, but the retry makes the window wider. Can
this re-advertise a service whose port was closed while the loop was
sleeping?

Sequence:

1. announce_servers() is sleeping in msleep(100), with srv pointing at
   local server S on port P.
2. qrtr_hello_work() succeeds and hello_sent becomes true.
3. Port P is closed. qrtr_port_remove() broadcasts DEL_CLIENT to the
   remote through qrtr_bcast_enqueue(), which now gets through. It also
   queues a local copy for the NS.
4. The NS wakes up and sends NEW_SERVER for P to the remote.
5. Later the NS handles the local DEL_CLIENT. ctrl_cmd_del_client()
   calls server_del(node, port, false), which sends no DEL_SERVER.

The remote is left with an entry for a closed service. This is not a
use-after-free, because only the NS worker frees entries in
node->servers.

The same ordering race already exists whenever the NS worker handles a
remote HELLO before a queued local DEL_CLIENT. The retry adds a window
of up to 100 ms right after hello_sent becomes true.

> +		}
>  		if (ret < 0) {
>  			if (ret == -ENODEV)
>  				continue;

[Severity: Medium]

What happens if the remote accepts our HELLO only after the 10 s
deadline?

When the deadline passes, ret is still -EAGAIN. announce_servers() then
prints "failed to announce new service" and returns, skipping the
remaining servers. qrtr_ns_worker() only logs the error.

The core keeps retrying HELLO every 100 ms through qrtr_hello_work(),
with no limit. So hello_sent can become true after the NS has already
given up:

		if (!rc && type == QRTR_TYPE_HELLO)
			node->hello_sent = true;

At that point nothing in ns.c or af_qrtr.c calls announce_servers()
again, and the remote's HELLO has already been handled. Wouldn't every
service registered before then stay unannounced? That is the same rmtfs
failure this patch is meant to fix.

On glink the firmware controls this delay. qcom_glink_request_intent()
can wait up to 10 * HZ for each intent request while the HELLO xmit
holds node->ep_lock.

The first service_announce_new() blocks on that mutex at the hello_sent
check in qrtr_node_enqueue(). The deadline was set before that call, so
it keeps running, and one slow intent round trip can use up the whole
window. The window also opens again after every qrtr_endpoint_hello()
on resume.

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