public inbox for ntb@lists.linux.dev
 help / color / mirror / Atom feed
* [PATCH] NTB: ntb_transport: fix use after free
@ 2025-11-03 17:23 Baruch Siach
  2025-11-03 17:45 ` Dave Jiang
  2025-11-04  0:32 ` Koichiro Den
  0 siblings, 2 replies; 6+ messages in thread
From: Baruch Siach @ 2025-11-03 17:23 UTC (permalink / raw)
  To: Jon Mason, Dave Jiang, Allen Hubbe; +Cc: Koichiro Den, ntb, Baruch Siach

Don't call dmaengine_unmap_put() twice for the same pointer. This
results in mempool_free() being called on a freed element.

Fixes: 6f57fd0578df ("NTB: convert to dmaengine_unmap_data")
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/ntb/ntb_transport.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index eb875e3db2e3..809fb09658b4 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -1573,14 +1573,14 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
 	unmap->addr[0] = dma_map_page(device->dev, virt_to_page(offset),
 				      pay_off, len, DMA_TO_DEVICE);
 	if (dma_mapping_error(device->dev, unmap->addr[0]))
-		goto err_get_unmap;
+		goto err_unmap;
 
 	unmap->to_cnt = 1;
 
 	unmap->addr[1] = dma_map_page(device->dev, virt_to_page(buf),
 				      buff_off, len, DMA_FROM_DEVICE);
 	if (dma_mapping_error(device->dev, unmap->addr[1]))
-		goto err_get_unmap;
+		goto err_unmap;
 
 	unmap->from_cnt = 1;
 
@@ -1588,7 +1588,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
 					     unmap->addr[0], len,
 					     DMA_PREP_INTERRUPT);
 	if (!txd)
-		goto err_get_unmap;
+		goto err_unmap;
 
 	txd->callback_result = ntb_rx_copy_callback;
 	txd->callback_param = entry;
@@ -1596,7 +1596,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
 
 	cookie = dmaengine_submit(txd);
 	if (dma_submit_error(cookie))
-		goto err_set_unmap;
+		goto err_unmap;
 
 	dmaengine_unmap_put(unmap);
 
@@ -1606,9 +1606,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
 
 	return 0;
 
-err_set_unmap:
-	dmaengine_unmap_put(unmap);
-err_get_unmap:
+err_unmap:
 	dmaengine_unmap_put(unmap);
 err:
 	return -ENXIO;
@@ -1854,14 +1852,14 @@ static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
 	unmap->addr[0] = dma_map_page(device->dev, virt_to_page(buf),
 				      buff_off, len, DMA_TO_DEVICE);
 	if (dma_mapping_error(device->dev, unmap->addr[0]))
-		goto err_get_unmap;
+		goto err_unmap;
 
 	unmap->to_cnt = 1;
 
 	txd = device->device_prep_dma_memcpy(chan, dest, unmap->addr[0], len,
 					     DMA_PREP_INTERRUPT);
 	if (!txd)
-		goto err_get_unmap;
+		goto err_unmap;
 
 	txd->callback_result = ntb_tx_copy_callback;
 	txd->callback_param = entry;
@@ -1869,16 +1867,14 @@ static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
 
 	cookie = dmaengine_submit(txd);
 	if (dma_submit_error(cookie))
-		goto err_set_unmap;
+		goto err_unmap;
 
 	dmaengine_unmap_put(unmap);
 
 	dma_async_issue_pending(chan);
 
 	return 0;
-err_set_unmap:
-	dmaengine_unmap_put(unmap);
-err_get_unmap:
+err_unmap:
 	dmaengine_unmap_put(unmap);
 err:
 	return -ENXIO;
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] NTB: ntb_transport: fix use after free
  2025-11-03 17:23 [PATCH] NTB: ntb_transport: fix use after free Baruch Siach
@ 2025-11-03 17:45 ` Dave Jiang
  2025-11-04  0:32 ` Koichiro Den
  1 sibling, 0 replies; 6+ messages in thread
From: Dave Jiang @ 2025-11-03 17:45 UTC (permalink / raw)
  To: Baruch Siach, Jon Mason, Allen Hubbe; +Cc: Koichiro Den, ntb



On 11/3/25 10:23 AM, Baruch Siach wrote:
> Don't call dmaengine_unmap_put() twice for the same pointer. This
> results in mempool_free() being called on a freed element.
> 
> Fixes: 6f57fd0578df ("NTB: convert to dmaengine_unmap_data")
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>> ---
>  drivers/ntb/ntb_transport.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index eb875e3db2e3..809fb09658b4 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1573,14 +1573,14 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>  	unmap->addr[0] = dma_map_page(device->dev, virt_to_page(offset),
>  				      pay_off, len, DMA_TO_DEVICE);
>  	if (dma_mapping_error(device->dev, unmap->addr[0]))
> -		goto err_get_unmap;
> +		goto err_unmap;
>  
>  	unmap->to_cnt = 1;
>  
>  	unmap->addr[1] = dma_map_page(device->dev, virt_to_page(buf),
>  				      buff_off, len, DMA_FROM_DEVICE);
>  	if (dma_mapping_error(device->dev, unmap->addr[1]))
> -		goto err_get_unmap;
> +		goto err_unmap;
>  
>  	unmap->from_cnt = 1;
>  
> @@ -1588,7 +1588,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>  					     unmap->addr[0], len,
>  					     DMA_PREP_INTERRUPT);
>  	if (!txd)
> -		goto err_get_unmap;
> +		goto err_unmap;
>  
>  	txd->callback_result = ntb_rx_copy_callback;
>  	txd->callback_param = entry;
> @@ -1596,7 +1596,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>  
>  	cookie = dmaengine_submit(txd);
>  	if (dma_submit_error(cookie))
> -		goto err_set_unmap;
> +		goto err_unmap;
>  
>  	dmaengine_unmap_put(unmap);
>  
> @@ -1606,9 +1606,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>  
>  	return 0;
>  
> -err_set_unmap:
> -	dmaengine_unmap_put(unmap);
> -err_get_unmap:
> +err_unmap:
>  	dmaengine_unmap_put(unmap);
>  err:
>  	return -ENXIO;
> @@ -1854,14 +1852,14 @@ static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
>  	unmap->addr[0] = dma_map_page(device->dev, virt_to_page(buf),
>  				      buff_off, len, DMA_TO_DEVICE);
>  	if (dma_mapping_error(device->dev, unmap->addr[0]))
> -		goto err_get_unmap;
> +		goto err_unmap;
>  
>  	unmap->to_cnt = 1;
>  
>  	txd = device->device_prep_dma_memcpy(chan, dest, unmap->addr[0], len,
>  					     DMA_PREP_INTERRUPT);
>  	if (!txd)
> -		goto err_get_unmap;
> +		goto err_unmap;
>  
>  	txd->callback_result = ntb_tx_copy_callback;
>  	txd->callback_param = entry;
> @@ -1869,16 +1867,14 @@ static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
>  
>  	cookie = dmaengine_submit(txd);
>  	if (dma_submit_error(cookie))
> -		goto err_set_unmap;
> +		goto err_unmap;
>  
>  	dmaengine_unmap_put(unmap);
>  
>  	dma_async_issue_pending(chan);
>  
>  	return 0;
> -err_set_unmap:
> -	dmaengine_unmap_put(unmap);
> -err_get_unmap:
> +err_unmap:
>  	dmaengine_unmap_put(unmap);
>  err:
>  	return -ENXIO;


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] NTB: ntb_transport: fix use after free
  2025-11-03 17:23 [PATCH] NTB: ntb_transport: fix use after free Baruch Siach
  2025-11-03 17:45 ` Dave Jiang
@ 2025-11-04  0:32 ` Koichiro Den
  2025-11-04  5:42   ` Baruch Siach
  1 sibling, 1 reply; 6+ messages in thread
From: Koichiro Den @ 2025-11-04  0:32 UTC (permalink / raw)
  To: Baruch Siach; +Cc: Jon Mason, Dave Jiang, Allen Hubbe, ntb

On Mon, Nov 03, 2025 at 07:23:19PM +0200, Baruch Siach wrote:
> Don't call dmaengine_unmap_put() twice for the same pointer. This
> results in mempool_free() being called on a freed element.

Hi,

While the second dmaengine_unmap_put() looks redundant at first glance, my
understanding was that it compensates for the refcount decrement that would
normally happen on the dmaengine completion path, which never occurs when
dmaengine_submit() fails. Am I missing something?

-Koichiro

> 
> Fixes: 6f57fd0578df ("NTB: convert to dmaengine_unmap_data")
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/ntb/ntb_transport.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index eb875e3db2e3..809fb09658b4 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1573,14 +1573,14 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>  	unmap->addr[0] = dma_map_page(device->dev, virt_to_page(offset),
>  				      pay_off, len, DMA_TO_DEVICE);
>  	if (dma_mapping_error(device->dev, unmap->addr[0]))
> -		goto err_get_unmap;
> +		goto err_unmap;
>  
>  	unmap->to_cnt = 1;
>  
>  	unmap->addr[1] = dma_map_page(device->dev, virt_to_page(buf),
>  				      buff_off, len, DMA_FROM_DEVICE);
>  	if (dma_mapping_error(device->dev, unmap->addr[1]))
> -		goto err_get_unmap;
> +		goto err_unmap;
>  
>  	unmap->from_cnt = 1;
>  
> @@ -1588,7 +1588,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>  					     unmap->addr[0], len,
>  					     DMA_PREP_INTERRUPT);
>  	if (!txd)
> -		goto err_get_unmap;
> +		goto err_unmap;
>  
>  	txd->callback_result = ntb_rx_copy_callback;
>  	txd->callback_param = entry;
> @@ -1596,7 +1596,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>  
>  	cookie = dmaengine_submit(txd);
>  	if (dma_submit_error(cookie))
> -		goto err_set_unmap;
> +		goto err_unmap;
>  
>  	dmaengine_unmap_put(unmap);
>  
> @@ -1606,9 +1606,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>  
>  	return 0;
>  
> -err_set_unmap:
> -	dmaengine_unmap_put(unmap);
> -err_get_unmap:
> +err_unmap:
>  	dmaengine_unmap_put(unmap);
>  err:
>  	return -ENXIO;
> @@ -1854,14 +1852,14 @@ static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
>  	unmap->addr[0] = dma_map_page(device->dev, virt_to_page(buf),
>  				      buff_off, len, DMA_TO_DEVICE);
>  	if (dma_mapping_error(device->dev, unmap->addr[0]))
> -		goto err_get_unmap;
> +		goto err_unmap;
>  
>  	unmap->to_cnt = 1;
>  
>  	txd = device->device_prep_dma_memcpy(chan, dest, unmap->addr[0], len,
>  					     DMA_PREP_INTERRUPT);
>  	if (!txd)
> -		goto err_get_unmap;
> +		goto err_unmap;
>  
>  	txd->callback_result = ntb_tx_copy_callback;
>  	txd->callback_param = entry;
> @@ -1869,16 +1867,14 @@ static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
>  
>  	cookie = dmaengine_submit(txd);
>  	if (dma_submit_error(cookie))
> -		goto err_set_unmap;
> +		goto err_unmap;
>  
>  	dmaengine_unmap_put(unmap);
>  
>  	dma_async_issue_pending(chan);
>  
>  	return 0;
> -err_set_unmap:
> -	dmaengine_unmap_put(unmap);
> -err_get_unmap:
> +err_unmap:
>  	dmaengine_unmap_put(unmap);
>  err:
>  	return -ENXIO;
> -- 
> 2.51.0
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] NTB: ntb_transport: fix use after free
  2025-11-04  0:32 ` Koichiro Den
@ 2025-11-04  5:42   ` Baruch Siach
  2025-11-06  3:59     ` Koichiro Den
  0 siblings, 1 reply; 6+ messages in thread
From: Baruch Siach @ 2025-11-04  5:42 UTC (permalink / raw)
  To: Koichiro Den; +Cc: Jon Mason, Dave Jiang, Allen Hubbe, ntb

Hi Koichiro,

Thanks for taking the time to review this.

On Tue, Nov 04 2025, Koichiro Den wrote:
> On Mon, Nov 03, 2025 at 07:23:19PM +0200, Baruch Siach wrote:
>> Don't call dmaengine_unmap_put() twice for the same pointer. This
>> results in mempool_free() being called on a freed element.
>
> While the second dmaengine_unmap_put() looks redundant at first glance, my
> understanding was that it compensates for the refcount decrement that would
> normally happen on the dmaengine completion path, which never occurs when
> dmaengine_submit() fails. Am I missing something?

I guess you are right. Now I see that dma_set_unmap() increments the
refcount. But I can't find the second refcount decrement in completion
path. I only see one call to dmaengine_unmap_put(). Can you enlighten
me?

Thanks,
baruch

>> Fixes: 6f57fd0578df ("NTB: convert to dmaengine_unmap_data")
>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>> ---
>>  drivers/ntb/ntb_transport.c | 22 +++++++++-------------
>>  1 file changed, 9 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
>> index eb875e3db2e3..809fb09658b4 100644
>> --- a/drivers/ntb/ntb_transport.c
>> +++ b/drivers/ntb/ntb_transport.c
>> @@ -1573,14 +1573,14 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>>  	unmap->addr[0] = dma_map_page(device->dev, virt_to_page(offset),
>>  				      pay_off, len, DMA_TO_DEVICE);
>>  	if (dma_mapping_error(device->dev, unmap->addr[0]))
>> -		goto err_get_unmap;
>> +		goto err_unmap;
>>  
>>  	unmap->to_cnt = 1;
>>  
>>  	unmap->addr[1] = dma_map_page(device->dev, virt_to_page(buf),
>>  				      buff_off, len, DMA_FROM_DEVICE);
>>  	if (dma_mapping_error(device->dev, unmap->addr[1]))
>> -		goto err_get_unmap;
>> +		goto err_unmap;
>>  
>>  	unmap->from_cnt = 1;
>>  
>> @@ -1588,7 +1588,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>>  					     unmap->addr[0], len,
>>  					     DMA_PREP_INTERRUPT);
>>  	if (!txd)
>> -		goto err_get_unmap;
>> +		goto err_unmap;
>>  
>>  	txd->callback_result = ntb_rx_copy_callback;
>>  	txd->callback_param = entry;
>> @@ -1596,7 +1596,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>>  
>>  	cookie = dmaengine_submit(txd);
>>  	if (dma_submit_error(cookie))
>> -		goto err_set_unmap;
>> +		goto err_unmap;
>>  
>>  	dmaengine_unmap_put(unmap);
>>  
>> @@ -1606,9 +1606,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>>  
>>  	return 0;
>>  
>> -err_set_unmap:
>> -	dmaengine_unmap_put(unmap);
>> -err_get_unmap:
>> +err_unmap:
>>  	dmaengine_unmap_put(unmap);
>>  err:
>>  	return -ENXIO;
>> @@ -1854,14 +1852,14 @@ static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
>>  	unmap->addr[0] = dma_map_page(device->dev, virt_to_page(buf),
>>  				      buff_off, len, DMA_TO_DEVICE);
>>  	if (dma_mapping_error(device->dev, unmap->addr[0]))
>> -		goto err_get_unmap;
>> +		goto err_unmap;
>>  
>>  	unmap->to_cnt = 1;
>>  
>>  	txd = device->device_prep_dma_memcpy(chan, dest, unmap->addr[0], len,
>>  					     DMA_PREP_INTERRUPT);
>>  	if (!txd)
>> -		goto err_get_unmap;
>> +		goto err_unmap;
>>  
>>  	txd->callback_result = ntb_tx_copy_callback;
>>  	txd->callback_param = entry;
>> @@ -1869,16 +1867,14 @@ static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
>>  
>>  	cookie = dmaengine_submit(txd);
>>  	if (dma_submit_error(cookie))
>> -		goto err_set_unmap;
>> +		goto err_unmap;
>>  
>>  	dmaengine_unmap_put(unmap);
>>  
>>  	dma_async_issue_pending(chan);
>>  
>>  	return 0;
>> -err_set_unmap:
>> -	dmaengine_unmap_put(unmap);
>> -err_get_unmap:
>> +err_unmap:
>>  	dmaengine_unmap_put(unmap);
>>  err:
>>  	return -ENXIO;
>> -- 
>> 2.51.0
>> 

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] NTB: ntb_transport: fix use after free
  2025-11-04  5:42   ` Baruch Siach
@ 2025-11-06  3:59     ` Koichiro Den
  2025-11-06  6:02       ` Baruch Siach
  0 siblings, 1 reply; 6+ messages in thread
From: Koichiro Den @ 2025-11-06  3:59 UTC (permalink / raw)
  To: Baruch Siach; +Cc: Jon Mason, Dave Jiang, Allen Hubbe, ntb

On Tue, Nov 04, 2025 at 07:42:53AM +0200, Baruch Siach wrote:
> Hi Koichiro,
> 
> Thanks for taking the time to review this.
> 
> On Tue, Nov 04 2025, Koichiro Den wrote:
> > On Mon, Nov 03, 2025 at 07:23:19PM +0200, Baruch Siach wrote:
> >> Don't call dmaengine_unmap_put() twice for the same pointer. This
> >> results in mempool_free() being called on a freed element.
> >
> > While the second dmaengine_unmap_put() looks redundant at first glance, my
> > understanding was that it compensates for the refcount decrement that would
> > normally happen on the dmaengine completion path, which never occurs when
> > dmaengine_submit() fails. Am I missing something?
> 
> I guess you are right. Now I see that dma_set_unmap() increments the
> refcount. But I can't find the second refcount decrement in completion
> path. I only see one call to dmaengine_unmap_put(). Can you enlighten
> me?

Apologies for the late response. The dma_descriptor_unmap() wrapper should
handle that, shouldn't it?

Thanks,
-Koichiro

> 
> Thanks,
> baruch
> 
> >> Fixes: 6f57fd0578df ("NTB: convert to dmaengine_unmap_data")
> >> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> >> ---
> >>  drivers/ntb/ntb_transport.c | 22 +++++++++-------------
> >>  1 file changed, 9 insertions(+), 13 deletions(-)
> >> 
> >> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> >> index eb875e3db2e3..809fb09658b4 100644
> >> --- a/drivers/ntb/ntb_transport.c
> >> +++ b/drivers/ntb/ntb_transport.c
> >> @@ -1573,14 +1573,14 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
> >>  	unmap->addr[0] = dma_map_page(device->dev, virt_to_page(offset),
> >>  				      pay_off, len, DMA_TO_DEVICE);
> >>  	if (dma_mapping_error(device->dev, unmap->addr[0]))
> >> -		goto err_get_unmap;
> >> +		goto err_unmap;
> >>  
> >>  	unmap->to_cnt = 1;
> >>  
> >>  	unmap->addr[1] = dma_map_page(device->dev, virt_to_page(buf),
> >>  				      buff_off, len, DMA_FROM_DEVICE);
> >>  	if (dma_mapping_error(device->dev, unmap->addr[1]))
> >> -		goto err_get_unmap;
> >> +		goto err_unmap;
> >>  
> >>  	unmap->from_cnt = 1;
> >>  
> >> @@ -1588,7 +1588,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
> >>  					     unmap->addr[0], len,
> >>  					     DMA_PREP_INTERRUPT);
> >>  	if (!txd)
> >> -		goto err_get_unmap;
> >> +		goto err_unmap;
> >>  
> >>  	txd->callback_result = ntb_rx_copy_callback;
> >>  	txd->callback_param = entry;
> >> @@ -1596,7 +1596,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
> >>  
> >>  	cookie = dmaengine_submit(txd);
> >>  	if (dma_submit_error(cookie))
> >> -		goto err_set_unmap;
> >> +		goto err_unmap;
> >>  
> >>  	dmaengine_unmap_put(unmap);
> >>  
> >> @@ -1606,9 +1606,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
> >>  
> >>  	return 0;
> >>  
> >> -err_set_unmap:
> >> -	dmaengine_unmap_put(unmap);
> >> -err_get_unmap:
> >> +err_unmap:
> >>  	dmaengine_unmap_put(unmap);
> >>  err:
> >>  	return -ENXIO;
> >> @@ -1854,14 +1852,14 @@ static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
> >>  	unmap->addr[0] = dma_map_page(device->dev, virt_to_page(buf),
> >>  				      buff_off, len, DMA_TO_DEVICE);
> >>  	if (dma_mapping_error(device->dev, unmap->addr[0]))
> >> -		goto err_get_unmap;
> >> +		goto err_unmap;
> >>  
> >>  	unmap->to_cnt = 1;
> >>  
> >>  	txd = device->device_prep_dma_memcpy(chan, dest, unmap->addr[0], len,
> >>  					     DMA_PREP_INTERRUPT);
> >>  	if (!txd)
> >> -		goto err_get_unmap;
> >> +		goto err_unmap;
> >>  
> >>  	txd->callback_result = ntb_tx_copy_callback;
> >>  	txd->callback_param = entry;
> >> @@ -1869,16 +1867,14 @@ static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
> >>  
> >>  	cookie = dmaengine_submit(txd);
> >>  	if (dma_submit_error(cookie))
> >> -		goto err_set_unmap;
> >> +		goto err_unmap;
> >>  
> >>  	dmaengine_unmap_put(unmap);
> >>  
> >>  	dma_async_issue_pending(chan);
> >>  
> >>  	return 0;
> >> -err_set_unmap:
> >> -	dmaengine_unmap_put(unmap);
> >> -err_get_unmap:
> >> +err_unmap:
> >>  	dmaengine_unmap_put(unmap);
> >>  err:
> >>  	return -ENXIO;
> >> -- 
> >> 2.51.0
> >> 
> 
> -- 
>                                                      ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] NTB: ntb_transport: fix use after free
  2025-11-06  3:59     ` Koichiro Den
@ 2025-11-06  6:02       ` Baruch Siach
  0 siblings, 0 replies; 6+ messages in thread
From: Baruch Siach @ 2025-11-06  6:02 UTC (permalink / raw)
  To: Koichiro Den; +Cc: Jon Mason, Dave Jiang, Allen Hubbe, ntb

Hi Koichiro,

On Thu, Nov 06 2025, Koichiro Den wrote:
> On Tue, Nov 04, 2025 at 07:42:53AM +0200, Baruch Siach wrote:
>> Thanks for taking the time to review this.
>> 
>> On Tue, Nov 04 2025, Koichiro Den wrote:
>> > On Mon, Nov 03, 2025 at 07:23:19PM +0200, Baruch Siach wrote:
>> >> Don't call dmaengine_unmap_put() twice for the same pointer. This
>> >> results in mempool_free() being called on a freed element.
>> >
>> > While the second dmaengine_unmap_put() looks redundant at first glance, my
>> > understanding was that it compensates for the refcount decrement that would
>> > normally happen on the dmaengine completion path, which never occurs when
>> > dmaengine_submit() fails. Am I missing something?
>> 
>> I guess you are right. Now I see that dma_set_unmap() increments the
>> refcount. But I can't find the second refcount decrement in completion
>> path. I only see one call to dmaengine_unmap_put(). Can you enlighten
>> me?
>
> Apologies for the late response. The dma_descriptor_unmap() wrapper should
> handle that, shouldn't it?

Probably. The dmaengine driver I'm using is dw-axi-dmac, and I see no
call to dma_descriptor_unmap() there. Not sure where the bug is, but
it's definitely not in the NTB scope.

Thanks for your help. Sorry for the noise.

baruch

>> >> Fixes: 6f57fd0578df ("NTB: convert to dmaengine_unmap_data")
>> >> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>> >> ---
>> >>  drivers/ntb/ntb_transport.c | 22 +++++++++-------------
>> >>  1 file changed, 9 insertions(+), 13 deletions(-)
>> >> 
>> >> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
>> >> index eb875e3db2e3..809fb09658b4 100644
>> >> --- a/drivers/ntb/ntb_transport.c
>> >> +++ b/drivers/ntb/ntb_transport.c
>> >> @@ -1573,14 +1573,14 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>> >>  	unmap->addr[0] = dma_map_page(device->dev, virt_to_page(offset),
>> >>  				      pay_off, len, DMA_TO_DEVICE);
>> >>  	if (dma_mapping_error(device->dev, unmap->addr[0]))
>> >> -		goto err_get_unmap;
>> >> +		goto err_unmap;
>> >>  
>> >>  	unmap->to_cnt = 1;
>> >>  
>> >>  	unmap->addr[1] = dma_map_page(device->dev, virt_to_page(buf),
>> >>  				      buff_off, len, DMA_FROM_DEVICE);
>> >>  	if (dma_mapping_error(device->dev, unmap->addr[1]))
>> >> -		goto err_get_unmap;
>> >> +		goto err_unmap;
>> >>  
>> >>  	unmap->from_cnt = 1;
>> >>  
>> >> @@ -1588,7 +1588,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>> >>  					     unmap->addr[0], len,
>> >>  					     DMA_PREP_INTERRUPT);
>> >>  	if (!txd)
>> >> -		goto err_get_unmap;
>> >> +		goto err_unmap;
>> >>  
>> >>  	txd->callback_result = ntb_rx_copy_callback;
>> >>  	txd->callback_param = entry;
>> >> @@ -1596,7 +1596,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>> >>  
>> >>  	cookie = dmaengine_submit(txd);
>> >>  	if (dma_submit_error(cookie))
>> >> -		goto err_set_unmap;
>> >> +		goto err_unmap;
>> >>  
>> >>  	dmaengine_unmap_put(unmap);
>> >>  
>> >> @@ -1606,9 +1606,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>> >>  
>> >>  	return 0;
>> >>  
>> >> -err_set_unmap:
>> >> -	dmaengine_unmap_put(unmap);
>> >> -err_get_unmap:
>> >> +err_unmap:
>> >>  	dmaengine_unmap_put(unmap);
>> >>  err:
>> >>  	return -ENXIO;
>> >> @@ -1854,14 +1852,14 @@ static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
>> >>  	unmap->addr[0] = dma_map_page(device->dev, virt_to_page(buf),
>> >>  				      buff_off, len, DMA_TO_DEVICE);
>> >>  	if (dma_mapping_error(device->dev, unmap->addr[0]))
>> >> -		goto err_get_unmap;
>> >> +		goto err_unmap;
>> >>  
>> >>  	unmap->to_cnt = 1;
>> >>  
>> >>  	txd = device->device_prep_dma_memcpy(chan, dest, unmap->addr[0], len,
>> >>  					     DMA_PREP_INTERRUPT);
>> >>  	if (!txd)
>> >> -		goto err_get_unmap;
>> >> +		goto err_unmap;
>> >>  
>> >>  	txd->callback_result = ntb_tx_copy_callback;
>> >>  	txd->callback_param = entry;
>> >> @@ -1869,16 +1867,14 @@ static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
>> >>  
>> >>  	cookie = dmaengine_submit(txd);
>> >>  	if (dma_submit_error(cookie))
>> >> -		goto err_set_unmap;
>> >> +		goto err_unmap;
>> >>  
>> >>  	dmaengine_unmap_put(unmap);
>> >>  
>> >>  	dma_async_issue_pending(chan);
>> >>  
>> >>  	return 0;
>> >> -err_set_unmap:
>> >> -	dmaengine_unmap_put(unmap);
>> >> -err_get_unmap:
>> >> +err_unmap:
>> >>  	dmaengine_unmap_put(unmap);
>> >>  err:
>> >>  	return -ENXIO;
>> >> -- 
>> >> 2.51.0
>> >> 
>> 
>> -- 
>>                                                      ~. .~   Tk Open Systems
>> =}------------------------------------------------ooO--U--Ooo------------{=
>>    - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-11-06  6:02 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-03 17:23 [PATCH] NTB: ntb_transport: fix use after free Baruch Siach
2025-11-03 17:45 ` Dave Jiang
2025-11-04  0:32 ` Koichiro Den
2025-11-04  5:42   ` Baruch Siach
2025-11-06  3:59     ` Koichiro Den
2025-11-06  6:02       ` Baruch Siach

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox