Re: [PATCH 1/2] net: qrtr: ns: keep going when an announcement cannot be 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: 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