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

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 `dmaeng=
ine_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()` successf=
ully 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 <[email protected]>

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 <[email protected]>
Fixes: 282a2feeb9bf ("NTB: Use DMA Engine to Transmit and Receive")
Cc: [email protected]
Signed-off-by: WenTao Liang <[email protected]>

> 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_ent=
ry *entry, void *offset)
>  	return 0;
> =20
>  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 origi=
nal
code, the fallthrough correctly freed the unmap object, but it left txd->un=
map
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,
> =20
>  	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 poin=
ter
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 a=
lso
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 =3D 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 asynchrono=
us
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?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260628083301.9781=
[email protected]?part=3D1