[PATCH 1/2] net: thunderbolt: Stop queueing Rx buffers if allocation fails

[PATCH 1/2] net: thunderbolt: Stop queueing Rx buffers if allocation fails

[Mika Westerberg]

Sashiko pointed out that there is a bug in the driver when Rx buffer
allocation fails, we silently release the memory but leave the
descriptor in the hardware ring. This could cause all kinds of memory
corruption to happen. Fix this by making tbnet_alloc_rx_buffers() not
release the buffers already queued and modify the call sites accordingly.

Fixes: e69b6c02b4c3 ("net: Add support for networking over Thunderbolt cable")
Closes: https://sashiko.dev/#/patchset/20260902082128.1148463-1-mika.westerberg%40linux.intel.com
Cc: stable@vger.kernel.org
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/net/thunderbolt/main.c | 49 +++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
index cf51b9c39f4e..30b3568a83fc 100644
--- a/drivers/net/thunderbolt/main.c
+++ b/drivers/net/thunderbolt/main.c
@@ -506,7 +506,7 @@ static unsigned int tbnet_available_buffers(const struct tbnet_ring *ring)
 static int tbnet_alloc_rx_buffers(struct tbnet *net, unsigned int nbuffers)
 {
 	struct tbnet_ring *ring = &net->rx_ring;
-	int ret;
+	int ret = 0;
 
 	while (nbuffers--) {
 		struct device *dma_dev = tb_ring_dma_device(ring->ring);
@@ -524,14 +524,16 @@ static int tbnet_alloc_rx_buffers(struct tbnet *net, unsigned int nbuffers)
 		tf->page = dev_alloc_pages(TBNET_RX_PAGE_ORDER);
 		if (!tf->page) {
 			ret = -ENOMEM;
-			goto err_free;
+			break;
 		}
 
 		dma_addr = dma_map_page(dma_dev, tf->page, 0,
 					TBNET_RX_PAGE_SIZE, DMA_FROM_DEVICE);
 		if (dma_mapping_error(dma_dev, dma_addr)) {
+			__free_pages(tf->page, TBNET_RX_PAGE_ORDER);
+			tf->page = NULL;
 			ret = -ENOMEM;
-			goto err_free;
+			break;
 		}
 
 		tf->frame.buffer_phy = dma_addr;
@@ -546,10 +548,6 @@ static int tbnet_alloc_rx_buffers(struct tbnet *net, unsigned int nbuffers)
 	}
 
 	tb_ring_notify(ring->ring);
-	return 0;
-
-err_free:
-	tbnet_free_buffers(ring);
 	return ret;
 }
 
@@ -678,7 +676,7 @@ static void tbnet_connected_work(struct work_struct *work)
 
 	ret = tbnet_alloc_tx_buffers(net);
 	if (ret)
-		goto err_free_rx_buffers;
+		goto err_stop_rings;
 
 	ret = tb_xdomain_enable_paths(net->xd, net->local_transmit_path,
 				      net->tx_ring.ring->hop,
@@ -686,7 +684,7 @@ static void tbnet_connected_work(struct work_struct *work)
 				      net->rx_ring.ring->hop);
 	if (ret) {
 		netdev_err(net->dev, "failed to enable DMA paths\n");
-		goto err_free_tx_buffers;
+		goto err_stop_rings;
 	}
 
 	netif_carrier_on(net->dev);
@@ -695,13 +693,11 @@ static void tbnet_connected_work(struct work_struct *work)
 	netdev_dbg(net->dev, "network traffic started\n");
 	return;
 
-err_free_tx_buffers:
-	tbnet_free_buffers(&net->tx_ring);
-err_free_rx_buffers:
-	tbnet_free_buffers(&net->rx_ring);
 err_stop_rings:
 	tb_ring_stop(net->rx_ring.ring);
 	tb_ring_stop(net->tx_ring.ring);
+	tbnet_free_buffers(&net->rx_ring);
+	tbnet_free_buffers(&net->tx_ring);
 	tb_xdomain_release_in_hopid(net->xd, net->remote_transmit_path);
 	tbnet_connect_failed(net);
 }
@@ -826,9 +822,13 @@ static bool tbnet_check_frame(struct tbnet *net, const struct tbnet_frame *tf,
 static int tbnet_poll(struct napi_struct *napi, int budget)
 {
 	struct tbnet *net = container_of(napi, struct tbnet, napi);
-	unsigned int cleaned_count = tbnet_available_buffers(&net->rx_ring);
 	struct device *dma_dev = tb_ring_dma_device(net->rx_ring.ring);
-	unsigned int rx_packets = 0;
+	unsigned int cleaned_count, rx_packets = 0;
+	bool no_buffers = false;
+
+	/* Buffers a previous run was not able to return to the ring */
+	cleaned_count = tb_ring_size(net->rx_ring.ring) -
+			tbnet_available_buffers(&net->rx_ring);
 
 	while (rx_packets < budget) {
 		const struct thunderbolt_ip_frame_header *hdr;
@@ -845,8 +845,14 @@ static int tbnet_poll(struct napi_struct *napi, int budget)
 		 * time.
 		 */
 		if (cleaned_count >= MAX_SKB_FRAGS) {
-			tbnet_alloc_rx_buffers(net, cleaned_count);
+			int ret;
+
+			ret = tbnet_alloc_rx_buffers(net, cleaned_count);
 			cleaned_count = 0;
+			if (ret) {
+				no_buffers = true;
+				break;
+			}
 		}
 
 		frame = tb_ring_poll(net->rx_ring.ring);
@@ -917,12 +923,19 @@ static int tbnet_poll(struct napi_struct *napi, int budget)
 		}
 	}
 
-	if (cleaned_count)
-		tbnet_alloc_rx_buffers(net, cleaned_count);
+	if (cleaned_count && tbnet_alloc_rx_buffers(net, cleaned_count))
+		no_buffers = true;
 
 	if (rx_packets >= budget)
 		return budget;
 
+	/*
+	 * We were not able to fill the RX ring for more buffers on this
+	 * run. Keep the polling enabled to retry the allocation.
+	 */
+	if (no_buffers && !tbnet_available_buffers(&net->rx_ring))
+		return budget;
+
 	napi_complete_done(napi, rx_packets);
 	/* Re-enable the ring interrupt */
 	tb_ring_poll_complete(net->rx_ring.ring);
-- 
2.50.1

[PATCH 2/2] net: thunderbolt: Prevent Tx descriptor leak in tbnet_start_xmit()

[Mika Westerberg]

Sashiko pointed out that tbnet_start_xmit() may leak one Tx
descriptor on three error paths jumping to err_drop because frame_index
is not yet updated but the Tx descriptor is already used. This will
accumulate over time and in the worst case run out of Tx descriptors
completely.

Fix this by subtracting the correct number on error path.

Fixes: e69b6c02b4c3 ("net: Add support for networking over Thunderbolt cable")
Closes: https://sashiko.dev/#/patchset/20260902082128.1148463-1-mika.westerberg%40linux.intel.com
Cc: stable@vger.kernel.org
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/net/thunderbolt/main.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
index 30b3568a83fc..febf5e5a8be3 100644
--- a/drivers/net/thunderbolt/main.c
+++ b/drivers/net/thunderbolt/main.c
@@ -1148,13 +1148,13 @@ static netdev_tx_t tbnet_start_xmit(struct sk_buff *skb,
 	struct tbnet_frame *frames[MAX_SKB_FRAGS];
 	u16 frame_id = atomic_read(&net->frame_id);
 	struct thunderbolt_ip_frame_header *hdr;
+	bool unmap = false, index_used = false;
 	unsigned int len = skb_headlen(skb);
 	unsigned int data_len = skb->len;
 	unsigned int nframes, i;
 	unsigned int frag = 0;
 	void *src = skb->data;
 	u32 frame_index = 0;
-	bool unmap = false;
 	void *dest;
 
 	trace_tbnet_tx_skb(skb);
@@ -1208,6 +1208,7 @@ static netdev_tx_t tbnet_start_xmit(struct sk_buff *skb,
 				src = tbnet_kmap_frag(skb, frag++, &len);
 				unmap = true;
 			} else if (unlikely(size_left > 0)) {
+				index_used = true;
 				goto err_drop;
 			}
 		} while (size_left > 0);
@@ -1244,6 +1245,7 @@ static netdev_tx_t tbnet_start_xmit(struct sk_buff *skb,
 			src = tbnet_kmap_frag(skb, frag++, &len);
 			unmap = true;
 		} else if (unlikely(data_len > 0)) {
+			index_used = true;
 			goto err_drop;
 		}
 	}
@@ -1253,8 +1255,10 @@ static netdev_tx_t tbnet_start_xmit(struct sk_buff *skb,
 	if (unmap)
 		kunmap_local(src);
 
-	if (!tbnet_xmit_csum_and_map(net, skb, frames, frame_index + 1))
+	if (!tbnet_xmit_csum_and_map(net, skb, frames, frame_index + 1)) {
+		index_used = true;
 		goto err_drop;
+	}
 
 	for (i = 0; i < frame_index + 1; i++)
 		tb_ring_tx_more(net->tx_ring.ring, &frames[i]->frame);
@@ -1274,6 +1278,9 @@ static netdev_tx_t tbnet_start_xmit(struct sk_buff *skb,
 err_drop:
 	/* We can re-use the buffers */
 	net->tx_ring.cons -= frame_index;
+	/* Return back the current slot as well */
+	if (index_used)
+		net->tx_ring.cons--;
 
 	dev_kfree_skb_any(skb);
 	net->stats.tx_errors++;
-- 
2.50.1

Re: [PATCH 1/2] net: thunderbolt: Stop queueing Rx buffers if allocation fails

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH 1/2] net: thunderbolt: Stop queueing Rx buffers if allocation fails

[netdev-bot+sashiko]

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

Critical: 0 · High: 3 · Medium: 0 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] tbnet_poll() (drivers/net/thunderbolt/main.c) can stop Rx
  permanently when a refill fails.
- [High] The new deficit-based cleaned_count in tbnet_poll() (lines
  829-831) lets a NAPI poll that runs after teardown allocate and strand…
- [High] tbnet_poll() no longer completes NAPI while allocations keep
  failing and the ring is empty (the new `return budget` at line 936),…
- [Low] The commit message understates what changed.
- [Low] The new comment in tbnet_poll() (lines 932-935) says that when
  the Rx ring could not be refilled, polling is kept enabled to retry…

--

net: thunderbolt: Stop queueing Rx buffers if allocation fails

On an allocation or DMA mapping failure, tbnet_alloc_rx_buffers() now
stops instead of freeing every Rx buffer, including the ones already
posted to the hardware ring. The patch also reorders the
tbnet_connected_work() error path so the rings are stopped before
buffers are freed. tbnet_poll() now refills based on the ring deficit
and keeps polling when the ring cannot be refilled.

Closes: https://sashiko.dev/#/patchset/20260902082128.1148463-1-mika.westerberg%40linux.intel.com

> corruption to happen. Fix this by making tbnet_alloc_rx_buffers() not
> release the buffers already queued and modify the call sites accordingly.

[Severity: Low]
This isn't a bug, but could the commit message say more about what
changes? The patch is tagged for stable, and these tbnet_poll() changes
are not visible from the description:

- cleaned_count now starts from the ring deficit,
  tb_ring_size() - (prod - cons), instead of prod - cons
- a refill failure now breaks out of the Rx loop early
- NAPI now returns budget without completing while the ring is empty
  and the order-1 GFP_ATOMIC allocations keep failing

The patch also includes two separate fixes.

First, tbnet_connected_work() now stops both rings before freeing
buffers. The old err_free_tx_buffers/err_free_rx_buffers labels freed
them while the rings were still running.

Second, the dma_mapping_error() path in tbnet_alloc_rx_buffers() now
frees the page locally. The old goto err_free reached
tbnet_free_buffers(), which called dma_unmap_page() on a stale
tf->frame.buffer_phy left over from an earlier use of the slot.

> diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
> index cf51b9c39f4ee..30b3568a83fc2 100644
> --- a/drivers/net/thunderbolt/main.c
> +++ b/drivers/net/thunderbolt/main.c

[ ... ]

> @@ -826,9 +822,13 @@ static bool tbnet_check_frame(struct tbnet *net, const struct tbnet_frame *tf,
>  static int tbnet_poll(struct napi_struct *napi, int budget)
>  {
>  	struct tbnet *net = container_of(napi, struct tbnet, napi);
> -	unsigned int cleaned_count = tbnet_available_buffers(&net->rx_ring);
>  	struct device *dma_dev = tb_ring_dma_device(net->rx_ring.ring);
> -	unsigned int rx_packets = 0;
> +	unsigned int cleaned_count, rx_packets = 0;
> +	bool no_buffers = false;
> +
> +	/* Buffers a previous run was not able to return to the ring */
> +	cleaned_count = tb_ring_size(net->rx_ring.ring) -
> +			tbnet_available_buffers(&net->rx_ring);

[Severity: High]
Can a poll that runs after teardown fill the stopped ring with a full
set of DMA-mapped pages here?

tbnet_tear_down() is called from tbnet_disconnect_work(),
tbnet_suspend() and tbnet_shutdown() without napi_disable(). Only
tbnet_stop() disables NAPI. In tbnet_tear_down(),
tbnet_free_buffers(&net->rx_ring) resets prod = cons = 0 after the ring
is stopped. A later poll then computes:

    cleaned_count = 256 - 0 = 256

and calls tbnet_alloc_rx_buffers(net, 256). For each slot it allocates
an order-1 page, maps it, stores it in tf->page and calls
tb_ring_rx_more(). On a stopped ring, __tb_ring_enqueue() takes this
branch:

	} else {
		ret = -ESHUTDOWN;
	}

The return value is ignored, and ring->prod++ still runs.

A poll can run after teardown in three ways: NAPI was already scheduled
when teardown ran, the new no_buffers path below keeps NAPI
rescheduling itself across a disconnect, or socket busy polling calls
the poll directly.

On ifdown or removal, tbnet_stop() calls tbnet_tear_down(), which skips
tbnet_free_buffers() because login_sent and login_received are already
false. tb_ring_free() then drops frames[]. Does this leak up to 256
order-1 pages and their DMA mappings?

On reconnect, tbnet_connected_work() calls
tbnet_alloc_rx_buffers(net, TBNET_RING_SIZE) while frames[0].page is
still set, so this check:

		if (tf->page)
			break;

returns 0 without posting anything. The link then comes up with carrier
on and no Rx descriptors in the hardware ring. Would Rx stay dead in
that case?

Before this patch, cleaned_count was prod - cons, which is 0 after
teardown, so a late poll did nothing.

[ ... ]

> @@ -845,8 +845,14 @@ static int tbnet_poll(struct napi_struct *napi, int budget)
>  		 * time.
>  		 */
>  		if (cleaned_count >= MAX_SKB_FRAGS) {
> -			tbnet_alloc_rx_buffers(net, cleaned_count);
> +			int ret;
> +
> +			ret = tbnet_alloc_rx_buffers(net, cleaned_count);
>  			cleaned_count = 0;
> +			if (ret) {
> +				no_buffers = true;
> +				break;
> +			}
>  		}

[Severity: High]
Can this break stop Rx permanently under memory pressure?

The break comes before tb_ring_poll(), so frames the NHI has already
completed are not reaped, even though reaping needs no allocation.

The keep-polling check at the end of tbnet_poll() is:

	if (no_buffers && !tbnet_available_buffers(&net->rx_ring))
		return budget;

That check uses prod - cons, which also counts descriptors that are
completed but not yet reaped. Unless every buffer has been reaped, NAPI
completes and tb_ring_poll_complete() unmasks the interrupt.

Consider this sequence:

1. One poll reaps 17 (MAX_SKB_FRAGS) frames and the refill fails,
   leaving prod - cons = 239.

2. On every later interrupt, cleaned_count = 256 - 239 = 17, so the
   refill runs first. If it fails, the poll breaks with zero frames
   reaped, and NAPI completes because available is 239.

3. The peer keeps sending until all 239 descriptors are completed. The
   last completion raises the final interrupt. That poll also fails the
   refill and unmasks.

4. With no free descriptor, the NHI cannot complete another frame.
   nhi_unmask_interrupt() only writes the mask register, so no further
   interrupt arrives.

Once memory is available again, what would schedule NAPI? It looks like
Rx stays stalled until the link is torn down.

[ ... ]

> @@ -917,12 +923,19 @@ static int tbnet_poll(struct napi_struct *napi, int budget)
>  		}
>  	}
>  
> -	if (cleaned_count)
> -		tbnet_alloc_rx_buffers(net, cleaned_count);
> +	if (cleaned_count && tbnet_alloc_rx_buffers(net, cleaned_count))
> +		no_buffers = true;
>  
>  	if (rx_packets >= budget)
>  		return budget;
>  
> +	/*
> +	 * We were not able to fill the RX ring for more buffers on this
> +	 * run. Keep the polling enabled to retry the allocation.
> +	 */
> +	if (no_buffers && !tbnet_available_buffers(&net->rx_ring))
> +		return budget;

[Severity: Low]
The comment says polling stays enabled whenever the refill fails. The
condition only keeps polling when no Rx buffers are posted at all.

After a partial refill failure, NAPI completes and the retry depends on
a later Rx interrupt. Could the comment describe the actual condition?

[Severity: High]
This return keeps NAPI polling while allocations fail, and
tbnet_tear_down() does not disable NAPI. Can this poll loop outlive a
disconnect and race with the next connection's tbnet_connected_work()?

Both paths call tbnet_alloc_rx_buffers() on the same net->rx_ring with
no shared lock:

tbnet_connected_work() (process context)
    tb_ring_start(net->rx_ring.ring);
    tbnet_alloc_rx_buffers(net, TBNET_RING_SIZE);

tbnet_poll() (softirq)
    tbnet_alloc_rx_buffers(net, cleaned_count);

Inside tbnet_alloc_rx_buffers():

		unsigned int index = ring->prod & (TBNET_RING_SIZE - 1);
		struct tbnet_frame *tf = &ring->frames[index];
		...
		if (tf->page)
			break;
		...
		tb_ring_rx_more(ring->ring, &tf->frame);

		ring->prod++;

Both callers can read the same ring->prod and see tf->page == NULL.
Each then allocates and maps its own page. The second overwrites
tf->page and tf->frame.buffer_phy, which leaks the first page and its
mapping.

Both then call tb_ring_rx_more() on the same ring_frame. ring->lock in
__tb_ring_enqueue() serializes the list operations. However, a second
list_add_tail() of a frame already on in_flight would corrupt
ring->queue and in_flight, and the same buffer could end up in two
descriptors.

The non-atomic ring->prod++ from both contexts would also break the
prod/cons accounting. Reaping the duplicate completion could then see
tf->page == NULL and pass it to page_address().

Busy polling (net.core.busy_read) also looks like a way to run
tbnet_poll() concurrently with tbnet_connected_work().

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103125621.1111198-1-mika.westerberg%40linux.intel.com

Re: [PATCH 1/2] net: thunderbolt: Stop queueing Rx buffers if allocation fails

[Mika Westerberg]

Hi,

On Sat, Nov 07, 2026 at 03:55:08PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 3 · Medium: 0 · Low: 2
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] tbnet_poll() (drivers/net/thunderbolt/main.c) can stop Rx
>   permanently when a refill fails.
> - [High] The new deficit-based cleaned_count in tbnet_poll() (lines
>   829-831) lets a NAPI poll that runs after teardown allocate and strand…
> - [High] tbnet_poll() no longer completes NAPI while allocations keep
>   failing and the ring is empty (the new `return budget` at line 936),…

I agree all of them. I will submit v2 with these fixed.

pw-bot: cr