Re: [PATCH v2 05/14] NTB: ntb_transport: Avoid losing QP link-up requests

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Race condition between `ntb_qp_link_work` and `ntb_transport_link_down` causes spurious link-up and breaks queue teardown synchronization.
- [High] Missing `dma_rmb()` after checking `DESC_DONE_FLAG` allows speculative reading of stale DMA data.
--

commit 99af086b145ca7c747420cbf914a391c3b3216f2
Author: Koichiro Den <den@valinux.co.jp>

NTB: ntb_transport: Avoid losing QP link-up requests

ntb_netdev_open() can call ntb_transport_link_up() while the transport
worker is completing setup on another CPU. Concurrent transport setup
and a client link-up request can both read the other's flag as false and
leave QP link work unqueued. The QP then stays down until another link
event or client link-up request.

Link: https://lore.kernel.org/r/20260907144701.702E41F00A3A@smtp.kernel.org/

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 75d4a2e021f4e..1332d53bcfe7d 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1104,10 +1104,16 @@ static void ntb_transport_link_work(struct work_struct *work)
>  	/* Publish the link only after every QP has been set up. */
>  	smp_store_release(&nt->link_is_up, true);
>  
> +	/*
> +	 * Prevent both sides from missing each other's flag. Pairs with
> +	 * the barrier in ntb_transport_link_up().
> +	 */
> +	smp_mb();
> +
>  	for (i = 0; i < nt->qp_count; i++) {
>  		struct ntb_transport_qp *qp = &nt->qp_vec[i];
>  
> -		if (qp->client_ready)
> +		if (READ_ONCE(qp->client_ready))
>  			ntb_transport_schedule_qp_link(qp, 0);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does the check for
qp->client_ready in ntb_qp_link_work() safely prevent a spurious link-up if
the client simultaneously tears down the link?

If CPU 1 calls ntb_transport_link_down(), clearing qp->client_ready and
calling cancel_delayed_work_sync(&qp->link_work):

drivers/ntb/ntb_transport.c:ntb_transport_link_down() {
	...
	if (qp->link_is_up)
		ntb_send_link_down(qp);
	else
		cancel_delayed_work_sync(&qp->link_work);
}

Could CPU 0, executing ntb_qp_link_work() concurrently, fail to see
qp->client_ready as false before writing to the remote SPAD and firing the
link-up event handler?

drivers/ntb/ntb_transport.c:ntb_qp_link_work() {
	...
	if (val & BIT(qp->qp_num)) {
		dev_info(&pdev->dev, "qp %d: Link Up\n", qp->qp_num);
		qp->link_is_up = true;
	...
}

This sequence bypasses the client's teardown intent, and forcing the link
back up after teardown may lead to use-after-free or data corruption.

[ ... ]

> @@ -2401,7 +2407,10 @@ void ntb_transport_link_up(struct ntb_transport_qp *qp)
>  	if (!qp)
>  		return;
>  
> -	qp->client_ready = true;
> +	WRITE_ONCE(qp->client_ready, true);
> +
> +	/* Pairs with the barrier in ntb_transport_link_work(). */
> +	smp_mb();
>  
>  	ntb_transport_schedule_qp_link(qp, 0);
>  }

[Severity: High]
This is a pre-existing issue, but does ntb_process_rxc() need a dma_rmb()
barrier after checking DESC_DONE_FLAG?

When processing received packets, the done flag is checked:

drivers/ntb/ntb_transport.c:ntb_process_rxc() {
	...
	if (!(hdr->flags & DESC_DONE_FLAG)) {
		dev_dbg(&qp->ndev->pdev->dev, "done flag not set\n");
		qp->rx_ring_empty++;
		return -EAGAIN;
	}

	if (hdr->flags & LINK_DOWN_FLAG) {
	...
}

Without a dma_rmb() here, can weak memory architectures (like ARM64 or
PowerPC) speculatively fetch subsequent header fields and payload data
(such as hdr->ver or hdr->len) before the flag check completes, leading to
processing stale DMA data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910040836.3792333-1-den@valinux.co.jp?part=5