Re: [PATCH v2 06/14] NTB: ntb_transport: Clear link state before QP cleanup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing read memory barrier in ntb_process_rxc() allows out-of-order reads of packet headers and payload.
- [High] ntb_qp_link_work() and ntb_transport_link_down() incorrectly echo the peer's QP link state back to the peer instead of transmitting the local state.
--

commit c193be6a6eafe61af2db594c3da12d870adbadab
Author: Koichiro Den <den@valinux.co.jp>
NTB: ntb_transport: Clear link state before QP cleanup

This commit clears the transport link state before QP cleanup to prevent
subsequent client link-up requests from starting QP link work prematurely.
It also ensures QP link work returns if the transport went down after it
was queued.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 1332d53bcfe7d..8dd1770aaaf15 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
[ ... ]
> @@ -1142,7 +1144,9 @@ static void ntb_qp_link_work(struct work_struct *work)
>  	struct ntb_transport_ctx *nt = qp->transport;
>  	int val;
>  
> -	WARN_ON(!nt->link_is_up);
> +	/* Pair with the link publication in ntb_transport_link_work(). */
> +	if (!smp_load_acquire(&nt->link_is_up))
> +		return;
>  
>  	val = ntb_spad_read(nt->ndev, QP_LINKS);
>  

[Severity: High]
This isn't a bug introduced by this patch, but do ntb_qp_link_work() and
ntb_transport_link_down() incorrectly echo the peer's QP link state back to
the peer instead of transmitting the local state?

When the local side brings up a QP, ntb_qp_link_work() reads the local
scratchpad via ntb_spad_read(). Because the peer writes its state to our
local scratchpad, this reads the peer's state. It then ORs its own QP bit
into this value and writes it back to the peer via ntb_peer_spad_write():

	val = ntb_spad_read(nt->ndev, QP_LINKS);

	ntb_peer_spad_write(nt->ndev, PIDX, QP_LINKS, val | BIT(qp->qp_num));

This same flawed echo logic applies in ntb_transport_link_down(), echoing
the peer's state back to the peer:

	val = ntb_spad_read(qp->ndev, QP_LINKS);

	ntb_peer_spad_write(qp->ndev, PIDX, QP_LINKS, val & ~BIT(qp->qp_num));

Is this echo loop intentional, or does it corrupt the independent QP link
state management by causing the peer to incorrectly believe that the local
side has brought up QPs that the peer itself brought up?

[Severity: High]
This is also a pre-existing issue, but is there a missing read memory
barrier in ntb_process_rxc() that allows out-of-order reads of packet
headers and payload?

In 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) {
		dev_dbg(&qp->ndev->pdev->dev, "link down flag set\n");

Is a dma_rmb() required after validating the DESC_DONE_FLAG to ensure
subsequent memory reads (like hdr->ver, hdr->len, and payload) are not
reordered before the flag check completes?

If the CPU speculatively reads hdr->ver, hdr->len, or the payload data before
the DESC_DONE_FLAG check is retired, and the DMA transfer hasn't fully arrived
at the CPU cache yet, could stale data be processed, leading to data
corruption?

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