Re: [PATCH net 1/4] NTB: ntb_transport: Recycle TX entries before client callbacks

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

Pre-existing issues:
- [High] Memory leak of client data buffers (e.g., network sk_buff) in ntb_process_tx() error path.
- [High] Uninitialized variable qp->last_cookie on the TX DMA path defeats DMA synchronization during queue teardown.
- [High] Pending TX queue entries and their associated payloads are permanently leaked when the DMA engine is terminated.
- [High] TOCTOU race on qp->tx_handler and qp->cb_data leads to a NULL pointer dereference.
--

--- Patch [1]: [PATCH net 1/4] NTB: ntb_transport: Recycle TX entries before client callbacks ---
commit 48d0145779a008cdeb1d647661138f0bd59414bb
Author: Koichiro Den <[email protected]>

NTB: ntb_transport: Recycle TX entries before client callbacks

ntb_tx_copy_callback() invokes the client callback before returning the
entry to tx_free_q. The callback may wake a stopped client queue, only
for the next enqueue to find no local entry and return -EBUSY. The window
is narrow, but the retry is unnecessary.

Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
Cc: [email protected]
Signed-off-by: Koichiro Den <[email protected]>

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index f59f926d4bfaa..e92b96f4afad8 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
[ ... ]
> @@ -1768,15 +1775,13 @@ static void ntb_tx_copy_callback(void *data,
>  	 * "link down" or similar.  Since no payload is being sent in these
>  	 * cases, there is nothing to add to the completion queue.
>  	 */
> -	if (entry->len > 0) {
> -		qp->tx_bytes += entry->len;
> -
> -		if (qp->tx_handler)
> -			qp->tx_handler(qp, qp->cb_data, entry->cb_data,
> -				       entry->len);
> -	}
> +	if (len > 0)
> +		qp->tx_bytes += len;
>  
>  	ntb_list_add(&qp->ntb_tx_free_q_lock, &entry->entry, &qp->tx_free_q);
> +
> +	if (len > 0 && qp->tx_handler)
> +		qp->tx_handler(qp, qp->cb_data, cb_data, len);

[Severity: High]
This is a pre-existing issue, but does this introduce a race condition leading
to a null pointer dereference?

In ntb_tx_copy_callback(), this code checks if qp->tx_handler is non-null
before calling it:

drivers/ntb/ntb_transport.c:ntb_tx_copy_callback() {
    ...
    if (len > 0 && qp->tx_handler)
        qp->tx_handler(qp, qp->cb_data, cb_data, len);
    ...
}

Concurrently, ntb_transport_free_queue() blindly zeroes qp->tx_handler and
qp->cb_data:

drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
    ...
    qp->cb_data = NULL;
    qp->rx_handler = NULL;
    qp->tx_handler = NULL;
    ...
}

Because there is no synchronization protecting the execution of the DMA tasklet
and the teardown clearing of these pointers, can the tasklet evaluate
qp->tx_handler as non-null, but then dereference a subsequently null
qp->tx_handler, or pass a null qp->cb_data pointer to the network callback?


[Severity: High]
This is a pre-existing issue, but does ntb_process_tx() leak the SKB?

If a transmitted packet length exceeds the maximum frame size, ntb_process_tx()
invokes qp->tx_handler() with NULL instead of the original packet buffer:

drivers/ntb/ntb_transport.c:ntb_process_tx() {
    ...
    if (entry->len > qp->tx_max_frame - sizeof(struct ntb_payload_header)) {
        if (qp->tx_handler)
            qp->tx_handler(qp, qp->cb_data, NULL, -EIO);
    ...
}

Because the network callback exits early if data is NULL, it fails to free the
SKB:

drivers/net/ntb_netdev.c:ntb_netdev_tx_handler() {
    ...
    skb = data;
    if (!skb || !ndev)
        return;
    ...
}

Furthermore, ntb_process_tx() returns 0 to the caller, preventing the network
stack from freeing it on failure.


[Severity: High]
This is a pre-existing issue, but does the uninitialized qp->last_cookie defeat
DMA synchronization during queue teardown?

ntb_transport_free_queue() attempts to wait for pending TX DMA transactions:

drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
    ...
    dma_sync_wait(chan, qp->last_cookie);
    ...
}

However, ntb_async_tx_submit() drops the DMA cookie instead of assigning it to
qp->last_cookie:

drivers/ntb/ntb_transport.c:ntb_async_tx_submit() {
    ...
    cookie = dmaengine_submit(txd);
    if (dma_submit_error(cookie))
        goto err_set_unmap;

    dmaengine_unmap_put(unmap);
    dma_async_issue_pending(chan);
    return 0;
    ...
}

Could the TX synchronization wait evaluate either 0 or an unrelated RX cookie on
the TX DMA channel, leading to premature termination of in-flight TX DMA
transfers?


[Severity: High]
This is a pre-existing issue, but are pending TX queue entries and their
associated payloads permanently leaked when the DMA engine is terminated?

During queue teardown, dmaengine_terminate_all() is called:

drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
    ...
    dmaengine_terminate_all(chan);
    ...
}

This aborts any pending or active DMA descriptors. The driver relies entirely
on DMA completion callbacks to return entries to qp->tx_free_q and to invoke
the client network teardown logic.

The teardown routine then only cleans up entries present in the idle lists:

drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
    ...
    while ((entry = ntb_list_rm(&qp->ntb_tx_free_q_lock, &qp->tx_free_q)))
        kfree(entry);

    while ((entry = ntb_list_rm(&qp->ntb_tx_offl_q_lock, &qp->tx_offl_q)))
        kfree(entry);
    ...
}

Will this permanently leak active entries and their corresponding client payloads
if transactions are in-flight during module unload or link down events?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1