Re: [PATCH 07/11] NTB: ntb_transport: Clear link state before QP cleanup

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Coding Pattern Violation: Mixing guard() and goto-based cleanup

Pre-existing issues:
- [High] Memory Ordering Race leading to lost wakeups
- [High] Data Race in ntb_qp_link_cleanup allowing concurrent execution
- [High] DMA Memory Corruption due to unterminated transactions
- [High] Missing read memory barrier (dma_rmb) when reading payload after checking DESC_DONE_FLAG
- [High] Stale Data Transmission due to missing flush on link down
--

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

    NTB: ntb_transport: Clear link state before QP cleanup

    Cleanup leaves the transport link marked up after releasing its MWs, allowing
    client link-up requests to start QP link work prematurely. Clear link_is_up
    before cancelling QP work and releasing MWs. Additionally, ensure 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 411017873a83b..4c0a29693ac23 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -990,6 +990,8 @@ static void ntb_transport_link_cleanup(struct ntb_transport_ctx *nt)
>  
>  	guard(mutex)(&nt->link_event_lock);
>  
> +	WRITE_ONCE(nt->link_is_up, false);
> +
>  	qp_bitmap_alloc = nt->qp_bitmap & ~nt->qp_bitmap_free;
>  
>  	/* Pass along the info to any clients */

[Severity: High]
This is a pre-existing issue, but does this cleanup routine allow concurrent
execution?

Looking at ntb_transport_link_cleanup(), it calls ntb_qp_link_cleanup() on all
active QPs, and then synchronizes cancellation using cancel_work_sync():

drivers/ntb/ntb_transport.c:ntb_transport_link_cleanup() {
    ...
    for (i = 0; i < nt->qp_count; i++)
        if (nt->qp_bitmap_alloc & BIT_ULL(i)) {
            struct ntb_transport_qp *qp = &nt->qp_vec[i];
            ntb_qp_link_cleanup(qp);
            cancel_work_sync(&qp->link_cleanup);
        }
    ...
}

If the ntb_qp_link_cleanup_work() worker is concurrently executing, it will
also invoke ntb_qp_link_cleanup(). Since no lock synchronizes
ntb_qp_link_cleanup(), it can execute concurrently across threads. This could
lead to data races when resetting QP indices and cause duplicate concurrent
calls to the client's event_handler, potentially corrupting the client driver
state.

[Severity: High]
This is a pre-existing issue, but is it possible for active DMA transactions to
corrupt freed memory here?

Later in ntb_transport_link_cleanup(), it frees the backing DMA memory:

drivers/ntb/ntb_transport.c:ntb_transport_link_cleanup() {
    ...
    for (i = 0; i < nt->mw_count; i++)
        ntb_free_mw(nt, i);
    ...
}

ntb_free_mw() calls dma_free_attrs() without ensuring the DMA channel is
stopped. Prior to this, ntb_qp_link_cleanup() kills the RX tasklet with
tasklet_kill(&qp->rxc_db_work), but does not terminate active DMA engine
transactions (e.g., via dmaengine_terminate_all()).

If a DMA transfer was previously submitted, could it continue asynchronously in
hardware, writing into the freed memory pages and leading to IOMMU faults or
memory corruption?

[Severity: High]
This is a pre-existing issue, but does resetting the link context leave stale
data in the transmit queues?

When the link goes down, ntb_qp_link_down_reset() is called:

drivers/ntb/ntb_transport.c:ntb_qp_link_down_reset() {
    ...
    ntb_qp_link_context_reset(qp);
    remote_rx_info = READ_ONCE(qp->remote_rx_info);
    ...
}

The QP context and indices (like qp->tx_index) are reset to 0, but pending
entries in qp->tx_offl_q or other transmit queues are not flushed. These
pending entries already have their entry->tx_index populated with old index
values from before the link dropped.

When the tx_offload_thread processes them, could it write the data to the
old, stale index locations instead of starting correctly at index 0, causing
the remote receiver to hang or receive out-of-sequence packets?

[ ... ]

[Severity: Medium]
Does ntb_transport_link_work() mix scope-based guard macros with legacy
goto-based error cleanup?

The cleanup.md documentation explicitly mandates that usage of "goto" and
cleanup helpers must never be mixed in the same function.

drivers/ntb/ntb_transport.c:ntb_transport_link_work() {
    ...
    guard(mutex)(&nt->link_event_lock);
    ...
        rc = ntb_set_mw(nt, i, val64);
        if (rc)
            goto out1;
    ...
out1:
    for (i = 0; i < nt->mw_count; i++)
        ntb_free_mw(nt, i);
    ...
}

This function uses guard(mutex)(&nt->link_event_lock) at the beginning, but
continues to use goto out1; and goto out; for error handling and legacy cleanup
paths. Does this violate the established architectural cleanup pattern?

[ ... ]

> @@ -1149,7 +1151,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 is a pre-existing issue, but does this Dekker's algorithm pattern lack a
full memory barrier, potentially leading to lost wakeups?

In ntb_transport_link_work(), nt->link_is_up is stored and qp->client_ready is
read:

drivers/ntb/ntb_transport.c:ntb_transport_link_work() {
    ...
    smp_store_release(&nt->link_is_up, true);

    for (i = 0; i < nt->qp_count; i++) {
        struct ntb_transport_qp *qp = &nt->qp_vec[i];

        if (qp->client_ready)
            ntb_transport_schedule_qp_link(qp, 0);
    }
    ...
}

Concurrently, ntb_transport_link_up() stores qp->client_ready and reads
nt->link_is_up:

drivers/ntb/ntb_transport.c:ntb_transport_link_up() {
    ...
    qp->client_ready = true;
    ntb_transport_schedule_qp_link(qp, 0);
    ...
}

Inside ntb_transport_schedule_qp_link(), it checks
smp_load_acquire(&nt->link_is_up). Because smp_store_release() allows
subsequent loads to be reordered before the store, and smp_load_acquire()
allows preceding stores to be reordered after the load, both threads can read
false for the counterpart flag.

Could this result in a lost wakeup where the QP link fails to initialize?

[ ... ]

[Severity: High]
This is a pre-existing issue, but is a read memory barrier required after
checking the DESC_DONE_FLAG?

In ntb_process_rxc(), the descriptor's done flag is read from the DMA-mapped
memory to verify packet completion:

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;
    }
    ...
}

However, there is no dma_rmb() issued after observing the flag. Could this
allow the CPU to speculatively read the subsequent header fields (e.g.,
hdr->ver, hdr->len) and the payload buffer before the done flag read
completes, resulting in stale data being processed?

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