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

[netdev-bot+sashiko@kernel.org]

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