Re: [PATCH v2 10/14] NTB: ntb_transport: Wait for RX completions before resetting a QP

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Data corruption during fallback RX memory copy.
- [High] Use-After-Free of `ntb_queue_entry` elements during transport queue teardown.
--

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

NTB: ntb_transport: Wait for RX completions before resetting a QP

This commit fixes a race condition where transport link cleanup can free a
memory window still in use by RX DMA. It adds a wait loop for the RX post
queue to become idle before resetting the QP.

Link: https://lore.kernel.org/r/20260907144257.767281F00A3A@smtp.kernel.org/

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 36797ea3ff45c..7ccba2c04f548 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c

[ ... ]

> @@ -959,6 +966,17 @@ static void ntb_qp_link_cleanup(struct ntb_transport_qp *qp)
>  	disable_delayed_work_sync(&qp->link_work);
>  	ntb_transport_set_qp_active(qp, false);
>  	tasklet_kill(&qp->rxc_db_work);
> +	/*
> +	 * Some DMA engines lack terminate/synchronize ops (e.g. IOAT), and
> +	 * DMA_COMPLETION_NO_ORDER rules out cookie-based waits.
> +	 *
> +	 * Waiting for rx_post_q to empty suffices: ntb_complete_rxc() finishes
> +	 * its MW accesses before removing each entry under ntb_rx_q_lock.
> +	 * qp->active is false and rxc_db_work is stopped, so no new RX DMA
> +	 * can be submitted.
> +	 */
> +	while (!ntb_transport_rx_idle(qp))
> +		fsleep(1000);
>  
>  	ntb_qp_link_down_reset(qp);

[Severity: High]
This is a pre-existing issue, but does ntb_transport_free_queue() also need
this same wait loop to prevent a use-after-free?

When tearing down a queue, ntb_transport_free_queue() calls
dmaengine_terminate_all() and dma_release_channel(). However, as noted in the
patch comment here, some DMA engines lack terminate/synchronize ops. The
cleanup routine then immediately frees the rx_post_q entries:

drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
    ...
	while ((entry = ntb_list_rm(&qp->ntb_rx_q_lock, &qp->rx_post_q))) {
		dev_warn(&pdev->dev, "Freeing item from non-empty rx_post_q\n");
		kfree(entry);
	}
    ...
}

If a DMA callback is pending or executing asynchronously while the queue is
being torn down, won't it dereference the freed entry pointer after
kfree(entry) is called?

[Severity: High]
This is another pre-existing issue, but can the fallback copy corrupt data
during a DMA error?

If a DMA transfer fails and ntb_rx_copy_callback() falls back to a software
copy, it calculates the offset using the globally incremented qp->rx_index:

drivers/ntb/ntb_transport.c:ntb_rx_copy_callback() {
    ...
		case DMA_TRANS_ABORTED:
		{
			struct ntb_transport_qp *qp = entry->qp;
			void *offset = qp->rx_buff + qp->rx_max_frame *
					qp->rx_index;

			ntb_memcpy_rx(entry, offset);
			qp->rx_memcpy++;
			return;
		}
    ...
}

Because qp->rx_index was already advanced to the next available ring buffer
frame when the transaction was originally submitted in ntb_process_rxc(),
won't this offset calculation point to the wrong frame and copy the wrong
payload? Should this calculation use the entry-specific index
(entry->rx_index) instead?

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