Re: [PATCH v2] ntb: fix tx descriptor leak on dmaengine_submit failure

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The patch introduces a call to a non-existent function `dmaengine_desc_put()`, which breaks the build, based on a flawed understanding of the original code's fallthrough behavior.

Pre-existing issues:
- [High] The original error path for a failed `dmaengine_submit()` successfully frees the `unmap` object via fallthrough but leaves a dangling pointer in the DMA descriptor, risking a double-free or use-after-free.
- [High] The DMA abort fallback in `ntb_rx_copy_callback` uses the globally advancing `qp->rx_index` instead of the entry-specific index, causing data corruption.
--

commit eef44bc86f40d9c6ef1b2ad54cbff8007792501a
Author: WenTao Liang <vulab@iscas.ac.cn>

ntb: fix tx descriptor leak on dmaengine_submit failure

This commit attempts to fix a perceived kref leak in the ntb transport
when dmaengine_submit fails. It replaces a call to dmaengine_unmap_put
with dmaengine_desc_put in both the rx and tx error handling paths.

Suggested-by: Greg KH <gregkh@linuxfoundation.org>
Fixes: 282a2feeb9bf ("NTB: Use DMA Engine to Transmit and Receive")
Cc: stable@vger.kernel.org
Signed-off-by: WenTao Liang <vulab@iscas.ac.cn>

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1572,7 +1572,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>  	return 0;
>  
>  err_set_unmap:
> -	dmaengine_unmap_put(unmap);
> +	dmaengine_desc_put(txd);

[Severity: Critical]
Does this function exist in the dma engine api? I am unable to locate
dmaengine_desc_put in the tree, which might cause a compilation failure.

[Severity: High]
This is a pre-existing issue, but the original fallthrough behavior properly
balanced the unmap kref.

If dmaengine_submit fails, execution drops into err_set_unmap. In the original
code, the fallthrough correctly freed the unmap object, but it left txd->unmap
pointing to the freed memory. If the dma provider later reclaims the
unsubmitted descriptor and calls dma_descriptor_unmap on it, wouldn't it
dereference and double-free the dangling pointer?

>  err_get_unmap:
>  	dmaengine_unmap_put(unmap);
>  err:

[ ... ]

> @@ -1896,7 +1896,7 @@ static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
>  
>  	return 0;
>  err_set_unmap:
> -	dmaengine_unmap_put(unmap);
> +	dmaengine_desc_put(txd);

[Severity: Critical]
Similar to the rx path, is dmaengine_desc_put available to be called here?

[Severity: High]
This is a pre-existing issue, but the tx path shares the same dangling pointer
risk as the rx path. When dmaengine_submit fails, the unmap object is freed
via fallthrough, but the descriptor still retains the pointer. Could this also
lead to a use-after-free or double-free if the dma provider cleans up the
descriptor?

>  err_get_unmap:
>  	dmaengine_unmap_put(unmap);
>  err:


While reviewing this file, I noticed a separate issue in the rx abort path:

[Severity: High]
This is a pre-existing issue, but does ntb_rx_copy_callback use the correct
index when a dma transfer aborts and falls back to a cpu copy?

In ntb_rx_copy_callback:

ntb_rx_copy_callback() {
    ...
    void *offset = qp->rx_buff + qp->rx_max_frame * qp->rx_index;
    ...
}

Because qp->rx_index is globally shared and increments immediately after
packets are submitted, it might advance significantly before the asynchronous
abort callback executes. Doesn't this cause the fallback copy to write data
into the wrong packet frame, potentially corrupting the payload?

Could this calculation use the constant, packet-specific entry->rx_index
instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260628083301.9781-1-vulab@iscas.ac.cn?part=1