Re: [PATCH v2 13/14] NTB: ntb_transport: Abort link setup on QP MW allocation failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Lockless access to `remote_rx_info` can lead to Use-After-Free during link down.
- [Critical] Pending TX transactions and DMA operations are not flushed on link down, causing data corruption across link sessions or PCIe AER faults.
- [High] Aborting link setup on error path `out1` leaves the remote peer with an initialized `VERSION` scratchpad, desynchronizing the link state and potentially causing PCIe AER faults.
--

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

NTB: ntb_transport: Abort link setup on QP MW allocation failure

This patch updates ntb_transport_link_work() to abort link setup if QP memory
window allocation fails. It ensures that the worker does not publish a QP with
only a partially initialized memory-window state.

Fixes: a754a8fcaf38 ("NTB: allocate number transport entries depending on size of ring size")
Cc: stable@vger.kernel.org

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 096be87e5ede9..c1e3a077c45ec 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1152,7 +1152,9 @@ static void ntb_transport_link_work(struct work_struct *work)
>  	}
>  
>  	for (i = 0; i < nt->qp_count; i++) {
> -		ntb_transport_setup_qp_mw(nt, i);
> +		rc = ntb_transport_setup_qp_mw(nt, i);
> +		if (rc)
> +			goto out1;

[Severity: High]
This isn't a bug introduced by this patch, but does jumping to out1 here
leave the remote peer with an initialized VERSION scratchpad?

Earlier in ntb_transport_link_work(), readiness is signaled to the remote peer:

    ntb_peer_spad_write(ndev, PIDX, VERSION, NTB_TRANSPORT_VERSION);
    /* Query the remote side for its info */

If ntb_transport_setup_qp_mw() fails, the error path cleans up local
translations but returns immediately without rescinding the version scratchpad:

out1:
    for (i = 0; i < nt->mw_count; i++)
        ntb_free_mw(nt, i);

    /* if there's an actual failure, we should just bail */
    if (rc < 0)
        return;

Could this leave the peer stranded in an invalid 'up' state, transmitting
memory-mapped I/O that hits unconfigured BARs and generates PCIe AER faults?

>  		ntb_transport_setup_qp_peer_msi(nt, i);
>  	}
>  

[Severity: Critical]
This is a pre-existing issue, but can lockless access to remote_rx_info
lead to a use-after-free regression during link down?

The ntb_transport_tx_free_entry() function reads the pointer locklessly:

drivers/ntb/ntb_transport.c:ntb_transport_tx_free_entry() {
    struct ntb_rx_info *remote_rx_info = READ_ONCE(qp->remote_rx_info);
    unsigned int head = qp->tx_index;
    unsigned int tail;

    if (!remote_rx_info)
        return 0;

    tail = remote_rx_info->entry;
    ...
}

When the link goes down, ntb_transport_link_cleanup() invokes ntb_free_mw()
to free the DMA memory window:

drivers/ntb/ntb_transport.c:ntb_free_mw() {
    ...
    for (i = num_mw; i < nt->qp_count; i += nt->mw_count) {
        nt->qp_vec[i].rx_buff = NULL;
        WRITE_ONCE(nt->qp_vec[i].remote_rx_info, NULL);
    }

    ntb_mw_clear_trans(nt->ndev, PIDX, num_mw);
    dma_free_attrs(dma_dev, mw->alloc_size, mw->alloc_addr,
                   mw->original_dma_addr, DMA_ATTR_FORCE_CONTIGUOUS);
}

Since the memory is freed immediately without waiting for concurrent readers
or an RCU grace period, could a reader preempted after reading the non-NULL
pointer dereference it after it is freed?

[Severity: Critical]
This is a pre-existing issue, but are pending TX transactions and DMA
operations properly flushed on link down?

When the link goes down, ntb_qp_link_cleanup() waits for RX to idle and resets
the QP state:

drivers/ntb/ntb_transport.c:ntb_qp_link_cleanup() {
    ...
    while (!ntb_transport_rx_idle(qp))
        fsleep(1000);

    ntb_qp_link_down_reset(qp);
    ...
}

However, it does not appear to synchronize, wait for, or cancel the TX offload
thread. The background thread can continue processing old packets:

drivers/ntb/ntb_transport.c:ntb_tx_memcpy_kthread() {
    ...
    list_for_each_entry_safe(entry, tmp, &local_list, entry) {
        list_del(&entry->entry);
        offset = qp->tx_mw + qp->tx_max_frame * entry->tx_index;
        ntb_memcpy_tx_on_stack(entry, offset);
    ...
}

Could delayed transactions from the old session execute and copy their
payloads to the remote memory window at their old tx_index, potentially
causing data corruption on the remote peer or PCIe AER faults if the
remote memory window is unmapped?

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