Re: [PATCH net 3/4] NTB: ntb_transport: Fail TX enqueue when the QP link is down

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

Pre-existing issues:
- [High] Data race and state corruption during concurrent link down and packet transmission.
- [Medium] Silent memory leak of SKBs when the QP link transitions down.
--

--- Patch [3]: [PATCH net 3/4] NTB: ntb_transport: Fail TX enqueue when the QP link is down ---
commit d31ae522b81066694c3712e62ed7ae6b70c6a06a
Author: Koichiro Den <[email protected]>

NTB: ntb_transport: Fail TX enqueue when the QP link is down

Commit f195a1a6fe41 meant to make ntb_transport_tx_enqueue() drop packets
submitted while the qp link is down, but it only returns 0 without
consuming the packet. This patch returns an error instead, restoring the
contract that a non-zero return leaves the buffer owned by the caller.

Fixes: f195a1a6fe41 ("ntb: Drop packets when qp link is down")
Cc: [email protected]

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index e92b96f4afad8..4bdd81edcb87a 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -2353,9 +2353,8 @@ int ntb_transport_tx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
>  	if (!qp || !len)
>  		return -EINVAL;
>  
> -	/* If the qp link is down already, just ignore. */
>  	if (!qp->link_is_up)
> -		return 0;
> +		return -ENOLINK;

[Severity: High]
This isn't a bug introduced by this patch, but does the lockless qp reset in
ntb_transport_link_down() race with concurrent packet transmission?

If an administrator changes the mtu, ntb_netdev_change_mtu() calls
ntb_transport_link_down() without first stopping the tx queues via
netif_tx_disable():

drivers/net/ntb_netdev.c:ntb_netdev_change_mtu() {
	/* Bring down the link and dispose of posted rx entries */
	for (q = 0; q < dev->num_queues; q++)
		ntb_transport_link_down(dev->queues[q].qp);
}

Concurrently, the network stack can call ntb_netdev_start_xmit() and reach
ntb_transport_tx_enqueue(), which processes tx entries.
Meanwhile, ntb_transport_link_down() triggers ntb_send_link_down() and
ntb_qp_link_down_reset(), which resets qp->tx_index to 0 locklessly.

drivers/ntb/ntb_transport.c:ntb_send_link_down() {
	rc = ntb_process_tx(qp, entry);
	...
	ntb_qp_link_down_reset(qp);
}

Since ntb_async_tx() writes to tx_index without synchronization:

drivers/ntb/ntb_transport.c:ntb_async_tx() {
	WRITE_ONCE(qp->tx_index, (qp->tx_index + 1) % qp->tx_max_entry);
}

Could this lead to concurrent modifications of qp->tx_index, qp->tx_pkts, and
hardware io memory window corruption?

[Severity: Medium]
This is a pre-existing issue, but does dropping the client_ready flag in
ntb_transport_link_down() cause a silent memory leak of skb objects?

When the interface link goes down, ntb_transport_link_down() disables the
client:

drivers/ntb/ntb_transport.c:ntb_transport_link_down() {
	qp->client_ready = false;
}

Later, the receive tasklet ntb_complete_rxc() processes dma completions from
rx_post_q. Since client_ready is false, it skips calling the network receive
handler that would normally free the skb:

drivers/ntb/ntb_transport.c:ntb_complete_rxc() {
		if (qp->rx_handler && qp->client_ready)
			qp->rx_handler(qp, qp->cb_data, cb_data, len);
}

The cleanup function only drains rx_pend_q:

drivers/ntb/ntb_transport.c:ntb_transport_rx_remove() {
	entry = ntb_list_rm(&qp->ntb_rx_q_lock, &qp->rx_pend_q);
}

Does this leave skb objects in rx_post_q stranded without being freed?

>  
>  	entry = ntb_list_rm(&qp->ntb_tx_free_q_lock, &qp->tx_free_q);
>  	if (!entry) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3