[PATCH net v2] net: atl1c: free the RX buffer when the descriptor reports an error

[PATCH net v2] net: atl1c: free the RX buffer when the descriptor reports an error

[Federico Musto]

atl1c_clean_rx() drops a frame whose return descriptor carries
RRS_RX_ERR_SUM or RRS_802_3_LEN_ERR by calling atl1c_clean_rfd(), which
only marks the RFD free and clears the skb pointer.  Nothing unmaps the DMA
mapping and nothing frees the skb, so every errored frame - CRC, alignment,
truncation, runt - leaks one skb and one DMA mapping.  The driver uses a
32-bit DMA mask, so on a host without an IOMMU each of those mappings also
pins a swiotlb slot.

Unmap and free before the RFD is recycled.  Factor the RFD lookup and
DMA unmap into a helper shared with the good path, using le32_to_cpu() when
decoding the descriptor index.  The hardware already counts these frames in
its own error counters, which atl1c_get_stats() reports, so no software
counter is added here.

Fixes: 43250ddd75a3 ("atl1c: Atheros L1C Gigabit Ethernet driver")
Cc: stable@vger.kernel.org
Signed-off-by: Federico Musto <federico@rimpianto.com>
---
v1 -> v2, after Paolo Abeni's review:
 - decode rrs->word0 with le32_to_cpu() before extracting the RFD index;
   this fixes the sparse warning on the new code
 - factor the RFD lookup and DMA unmap into atl1c_unmap_rx_buffer(), shared
   by the error and good paths instead of duplicating them

v1: https://lore.kernel.org/netdev/20261027115156.17128-1-federico@rimpianto.com/

Spotted while reviewing a fix for a different problem in the same function
(skb_over_panic on oversized RX frames), by the Sashiko review bot.  It is
independent of that patch and applies on its own.

The guard on buffer_info->skb is there because the slot can be empty after a
reset; rfd_num != 1 stays on the old path, since multi-RFD is not supported
by this driver anywhere else either.
---
 .../net/ethernet/atheros/atl1c/atl1c_main.c   | 33 +++++++++++++++----
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index eddd840..689941e 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -1864,6 +1864,23 @@ static void atl1c_clean_rfd(struct atl1c_rfd_ring *rfd_ring,
 	rfd_ring->next_to_clean = rfd_index;
 }
 
+static struct atl1c_buffer *
+atl1c_unmap_rx_buffer(struct pci_dev *pdev, struct atl1c_rfd_ring *rfd_ring,
+		      struct atl1c_recv_ret_status *rrs)
+{
+	struct atl1c_buffer *buffer_info;
+	u16 rfd_index;
+
+	rfd_index = (le32_to_cpu(rrs->word0) >> RRS_RX_RFD_INDEX_SHIFT) &
+		    RRS_RX_RFD_INDEX_MASK;
+	buffer_info = &rfd_ring->buffer_info[rfd_index];
+	if (buffer_info->skb)
+		dma_unmap_single(&pdev->dev, buffer_info->dma,
+				 buffer_info->length, DMA_FROM_DEVICE);
+
+	return buffer_info;
+}
+
 /**
  * atl1c_clean_rx - NAPI Rx polling callback
  * @napi: napi info
@@ -1874,7 +1891,7 @@ static int atl1c_clean_rx(struct napi_struct *napi, int budget)
 	struct atl1c_rrd_ring *rrd_ring =
 		container_of(napi, struct atl1c_rrd_ring, napi);
 	struct atl1c_adapter *adapter = rrd_ring->adapter;
-	u16 rfd_num, rfd_index;
+	u16 rfd_num;
 	u16 length;
 	struct pci_dev *pdev = adapter->pdev;
 	struct net_device *netdev  = adapter->netdev;
@@ -1908,6 +1925,14 @@ static int atl1c_clean_rx(struct napi_struct *napi, int budget)
 rrs_checked:
 		atl1c_clean_rrd(rrd_ring, rrs, rfd_num);
 		if (rrs->word3 & (RRS_RX_ERR_SUM | RRS_802_3_LEN_ERR)) {
+			/* atl1c_clean_rfd() drops the only pointer to the skb. */
+			if (likely(rfd_num == 1)) {
+				buffer_info = atl1c_unmap_rx_buffer(pdev, rfd_ring,
+								    rrs);
+				if (buffer_info->skb)
+					kfree_skb_reason(buffer_info->skb,
+							 SKB_DROP_REASON_NOT_SPECIFIED);
+			}
 			atl1c_clean_rfd(rfd_ring, rrs, rfd_num);
 			if (netif_msg_rx_err(adapter))
 				dev_warn(&pdev->dev,
@@ -1920,11 +1945,7 @@ rrs_checked:
 				RRS_PKT_SIZE_MASK);
 		/* Good Receive */
 		if (likely(rfd_num == 1)) {
-			rfd_index = (rrs->word0 >> RRS_RX_RFD_INDEX_SHIFT) &
-					RRS_RX_RFD_INDEX_MASK;
-			buffer_info = &rfd_ring->buffer_info[rfd_index];
-			dma_unmap_single(&pdev->dev, buffer_info->dma,
-					 buffer_info->length, DMA_FROM_DEVICE);
+			buffer_info = atl1c_unmap_rx_buffer(pdev, rfd_ring, rrs);
 			skb = buffer_info->skb;
 		} else {
 			/* TODO */
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v2] net: atl1c: free the RX buffer when the descriptor reports an error

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

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

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.