Re: [PATCH net-next v4 05/10] NTB: ntb_transport: Exchange client capabilities at link-up

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing write memory barrier (dma_wmb) between writing capabilities and advertising QP readiness leads to a race condition on weakly-ordered architectures.
--

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

NTB: ntb_transport: Exchange client capabilities at link-up

This commit updates the NTB transport to exchange 31-bit opaque 
capabilities during QP link setup using scratchpads. The capabilities
are passed to the client along with the link-up event.

Link: https://lore.kernel.org/r/xsaocp32a6siggahziu4jqbwgmdasmp6v64pd5aujfkoxhiqws@n5luestlntzc/

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 0b47285ef48b6..ea89eb336472a 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -937,7 +942,7 @@ static void ntb_qp_link_cleanup(struct ntb_transport_qp *qp)
>  	ntb_qp_link_down_reset(qp);
>  
>  	if (qp->event_handler)
> -		qp->event_handler(qp->cb_data, qp->link_is_up);
> +		qp->event_handler(qp->cb_data, qp->link_is_up, 0);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does this cleanup path
contain a deadlock?

The transport cleanup work ntb_transport_link_cleanup_work() acquires
nt->link_event_lock and calls ntb_transport_link_cleanup(). That function
synchronously waits for nt->link_work via cancel_delayed_work_sync().
However, if ntb_transport_link_work() executes concurrently, its very first
action is to attempt to acquire nt->link_event_lock. This lock inversion
can deadlock the workqueues and hang the subsystem.

[Severity: High]
This is also a pre-existing issue, but is there a use-after-free risk
when a queue is freed?

If a client frees a queue using ntb_transport_free_queue(), the
qp->link_cleanup work is never cancelled. If the transport is subsequently
removed, ntb_transport_link_cleanup() only cancels works for currently
allocated QPs, skipping the one the client already freed. When the
transport then frees the nt->qp_vec array, the workqueue subsystem might
subsequently attempt to execute the pending link_cleanup work for the
freed queue, causing a use-after-free crash.

[ ... ]

> @@ -1141,10 +1146,19 @@ static void ntb_qp_link_work(struct work_struct *work)
>  						   link_work.work);
>  	struct pci_dev *pdev = qp->ndev->pdev;
>  	struct ntb_transport_ctx *nt = qp->transport;
> +	u32 peer_caps = 0;
>  	int val;
>  
>  	WARN_ON(!nt->link_is_up);
>  
> +	/* Pair with the release store in ntb_transport_link_up(). */
> +	if (!smp_load_acquire(&qp->client_ready))
> +		return;
> +
> +	/* Publish capabilities before QP readiness. */
> +	if (qp->caps_spad)
> +		ntb_peer_spad_write(nt->ndev, PIDX, qp->caps_spad,
> +				    READ_ONCE(qp->local_caps) | QP_CAPS_VALID);
>  	val = ntb_spad_read(nt->ndev, QP_LINKS);
>  
>  	ntb_qp_up_request(qp, true);

[Severity: High]
Is a write memory barrier needed here between writing capabilities and
advertising QP readiness?

On weakly ordered architectures, the CPU or interconnect can reorder
consecutive memory writes. Without a barrier like dma_wmb(), the peer
might observe the readiness bit from ntb_qp_up_request() before the
capabilities land in memory-backed SPADs. The read side has a dma_rmb(),
but shouldn't the write side also need a dma_wmb() to guarantee order?

[ ... ]

> @@ -2439,12 +2480,17 @@ void ntb_transport_link_down(struct ntb_transport_qp *qp)
>  
>  	qp->client_ready = false;
>  
> +	if (!qp->link_is_up)
> +		cancel_delayed_work_sync(&qp->link_work);
> +

[Severity: High]
This isn't a bug introduced by this patch, but does skipping work
cancellation here create a race condition?

If qp->link_is_up is true, we skip cancel_delayed_work_sync(). If
ntb_qp_link_work() happens to be executing concurrently and gets preempted
right after successfully checking client_ready, the concurrent link_down
will set client_ready to false and clear the SPADs. When ntb_qp_link_work()
later resumes, it will overwrite the cleared SPADs with valid capabilities,
corrupting the teardown state.

> +	/* Stop advertising capabilities before withdrawing QP readiness. */
> +	if (qp->caps_spad)
> +		ntb_peer_spad_write(qp->ndev, PIDX, qp->caps_spad, 0);
> +
>  	ntb_qp_up_request(qp, false);

[Severity: High]
Similar to the link-up path, is a dma_wmb() needed here?

Without a write barrier, the peer could observe the withdrawal of QP
readiness before the capabilities are actually zeroed out in the SPADs.

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