[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.