* [PATCH 2/2] net: thunderbolt: Prevent Tx descriptor leak in tbnet_start_xmit()
2026-11-03 12:56 [PATCH 1/2] net: thunderbolt: Stop queueing Rx buffers if allocation fails Mika Westerberg
@ 2026-11-03 12:56 ` Mika Westerberg
2026-11-03 12:57 ` [PATCH 1/2] net: thunderbolt: Stop queueing Rx buffers if allocation fails netdev-bot+sinfo
2026-11-07 15:55 ` netdev-bot+sashiko
2 siblings, 0 replies; 5+ messages in thread
From: Mika Westerberg @ 2026-11-03 12:56 UTC (permalink / raw)
To: netdev
Cc: Yehezkel Bernat, Andrew Lunn, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 1/2] net: thunderbolt: Stop queueing Rx buffers if allocation fails
2026-11-03 12:56 [PATCH 1/2] net: thunderbolt: Stop queueing Rx buffers if allocation fails Mika Westerberg
2026-11-03 12:56 ` [PATCH 2/2] net: thunderbolt: Prevent Tx descriptor leak in tbnet_start_xmit() Mika Westerberg
2026-11-03 12:57 ` [PATCH 1/2] net: thunderbolt: Stop queueing Rx buffers if allocation fails netdev-bot+sinfo
@ 2026-11-07 15:55 ` netdev-bot+sashiko
2026-11-10 14:21 ` Mika Westerberg
2 siblings, 1 reply; 5+ messages in thread
From: netdev-bot+sashiko @ 2026-11-07 15:55 UTC (permalink / raw)
To: mika.westerberg
Cc: netdev, YehezkelShB, andrew+netdev, davem, edumazet, kuba, pabeni
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
^ permalink raw reply [flat|nested] 5+ messages in thread