Re: [PATCH v2 14/14] NTB: ntb_transport: Remove clients before freeing transport resources

[Dave Jiang <dave.jiang@intel.com>]

On 9/10/26 1:48 AM, Koichiro Den wrote:
> On Thu, Sep 10, 2026 at 04:36:13AM +0000, sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>>
>> New issues:
>> - [Critical] Skipping ntb_qp_link_cleanup() during transport teardown fails to drain rx_post_q, causing freed memory to be accessed by in-flight IOAT DMA.
> 
> I believe this is a false positive. See my inline comment below.
> 
>>
>> Pre-existing issues:
>> - [Critical] Double-fetch of `hdr->len` from shared memory allows buffer overflow via TOCTOU race.
>> - [High] Racy read-modify-write of `qp_bitmap_free` bitmask allows queue state corruption.
>> --
>>
>> commit 68ca6cad9a88600dd4607f3c4ccd6bd32385b0e7
>> Author: Koichiro Den <den@valinux.co.jp>
>> Subject: NTB: ntb_transport: Remove clients before freeing transport resources
>>
>> Unbinding ntb_transport can call ntb_transport_free() while ntb_netdev
>> is still bound. This patch reorders the teardown process to unregister
>> clients before freeing resources, intending to fix unsafe memory accesses
>> during intensive TX loads.
>>
>>> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
>>> index c1e3a077c45ec..f24555b38d1b5 100644
>>> --- a/drivers/ntb/ntb_transport.c
>>> +++ b/drivers/ntb/ntb_transport.c
>>> @@ -1487,9 +1487,11 @@ static void ntb_transport_free(struct ntb_client *self, struct ntb_dev *ndev)
>>>  
>>>  	debugfs_remove_recursive(nt->debugfs_node_dir);
>>>  
>>> -	ntb_transport_link_cleanup(nt);
>>> -	cancel_work_sync(&nt->link_cleanup);
>>> -	cancel_delayed_work_sync(&nt->link_work);
>>> +	/* Stop transport work before clients release their QPs. */
>>> +	disable_delayed_work_sync(&nt->link_work);
>>> +	disable_work_sync(&nt->link_cleanup);
>>> +
>>> +	ntb_bus_remove(nt);
>>
>> [Severity: Critical]
>> By moving ntb_bus_remove() before ntb_transport_link_cleanup(), does this
>> introduce a DMA use-after-free regression?
>>
>> Calling ntb_bus_remove() first prompts client drivers to free their QPs via
>> ntb_transport_free_queue(), which blindly frees all entries in rx_post_q
>> without polling:
>>
>> drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
>>     ...
>> 	while ((entry = ntb_list_rm(&qp->ntb_rx_q_lock, &qp->rx_post_q))) {
>> 		dev_warn(&pdev->dev, "Freeing item from non-empty rx_post_q\n");
>> 		kfree(entry);
>> 	}
>>     ...
>> }
> 
> Yes, but ntb_transport_free_queue() calls dma_release_channel() before freeing
> those entries. For IOAT, this goes through ioat_free_chan_resources(), which
> calls ioat_stop() and ioat_reset_hw() to synchronize callbacks and stop the DMA
> channel.

The refute looks reasonable to me for ioat. Any concerns for other host DMA engines? 

DJ

> 
> I believe Jon and Dave know IOAT well too, so I'd appreciate any comments from
> them. I guess the mention on IOAT in an earlier patch in this series prompted
> Sashiko to focus on it here.
> 
> Best regards,
> Koichiro
> 
>>
>> Because IOAT DMA engines could still be actively copying into those buffers,
>> freeing them allows the hardware DMA engine to write to freed kernel memory.
>> Furthermore, the DMA drain wait in ntb_qp_link_cleanup() is subsequently
>> skipped entirely because qp_bitmap_alloc evaluates to 0 during the later
>> call to ntb_transport_link_cleanup().
>>
>> [ ... ]
>>
>>> @@ -1736,17 +1736,17 @@ static void ntb_process_rxc(struct ntb_transport_qp *qp)
>>>  
>>>  	if (hdr->len > entry->len) {
>>>  		dev_dbg(&qp->ndev->pdev->dev,
>>>  			"receive buffer overflow! Wanted %d got %d\n",
>>>  			hdr->len, entry->len);
>>>  		qp->rx_err_oflow++;
>>>  
>>>  		entry->len = -EIO;
>>>  		entry->flags |= DESC_DONE_FLAG;
>>>  
>>>  		ntb_complete_rxc(qp);
>>>  	} else {
>>>  		dev_dbg(&qp->ndev->pdev->dev,
>>>  			"RX OK index %u ver %u size %d into buf size %d\n",
>>>  			qp->rx_index, hdr->ver, hdr->len, entry->len);
>>>  
>>>  		qp->rx_bytes += hdr->len;
>>>  		qp->rx_pkts++;
>>>  
>>>  		entry->len = hdr->len;
>>
>> [Severity: Critical]
>> This is a pre-existing issue, but is there a time-of-check to time-of-use
>> race condition in this bounds check on hdr->len?
>>
>> Since hdr points to a payload header located inside a memory window writable
>> by the remote NTB peer (shared memory), hdr->len is read directly from
>> shared memory twice (once for the bounds check, once for the assignment).
>>
>> Can a remote peer swap hdr->len to a much larger value immediately after
>> the bounds check but before the assignment, bypassing the bounds check and
>> causing a buffer overflow when copied?
>>
>> [ ... ]
>>
>>> @@ -2130,7 +2130,7 @@ struct ntb_transport_qp *
>>>  ntb_transport_create_queue(void *data, struct device *client_dev,
>>>  			   const struct ntb_queue_handlers *handlers)
>>>  {
>> [ ... ]
>>>  	free_queue--;
>>>  
>>>  	qp = &nt->qp_vec[free_queue];
>>>  	qp_bit = BIT_ULL(qp->qp_num);
>>>  
>>>  	nt->qp_bitmap_free &= ~qp_bit;
>>
>> [Severity: High]
>> This isn't a bug introduced by this patch, but could this read-modify-write
>> of nt->qp_bitmap_free lead to queue state corruption?
>>
>> The qp_bitmap_free bitmask is modified here using non-atomic bitwise
>> operations without holding any lock. It is also modified concurrently during
>> driver unbind:
>>
>> drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
>>     ...
>> 	qp->transport->qp_bitmap_free |= qp_bit;
>>     ...
>> }
>>
>> Since these are exported functions called by NTB client drivers, could
>> concurrent probe and remove operations race and lose bit updates, leading
>> to a leaked queue or two clients being assigned the same queue?
>>
>> -- 
>> Sashiko AI review · https://sashiko.dev/#/patchset/20260910040836.3792333-1-den@valinux.co.jp?part=14
>