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

[PATCH net] 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, the same way the good path does.
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>
---
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.
---
 drivers/net/ethernet/atheros/atl1c/atl1c_main.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index e58f1d2..2d68955 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -1908,6 +1908,23 @@ 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)) {
+			/* The RFD is recycled below, and that drops the only
+			 * pointer to the buffer: unmap and free it first, or the
+			 * skb and its DMA mapping leak on every errored frame.
+			 */
+			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];
+				if (buffer_info->skb) {
+					dma_unmap_single(&pdev->dev,
+							 buffer_info->dma,
+							 buffer_info->length,
+							 DMA_FROM_DEVICE);
+					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,
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net] 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.

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

[Federico Musto]

The leak was found by inspection, not by triggering it.

While reviewing a separate change in the same function - the oversized-RX
drop, which removes an skb_over_panic that I did hit on this hardware - the
review pointed at this branch: atl1c_clean_rfd() clears the skb pointer
without unmapping or freeing, so every frame carrying RRS_RX_ERR_SUM or
RRS_802_3_LEN_ERR leaks one skb and one DMA mapping.  I have not reproduced
the leak with an injected error stream, so the change is untested beyond
compiling and applying: it only moves the unmap/free that the good path
already performs to before the RFD is recycled.

Hardware the driver work is done on: MikroTik CCR2004-1G-2XS-PCIe (4x
AR8151) in a Proxmox VE 9.2 host, kernel 7.0.14, atl1c built out of tree
with DKMS.  On that box the hardware error counters do move
(rx_len_err/rx_crc_err) when the link partner sends frames the host MTU
cannot take, which is how this branch gets reached in practice; I did not
isolate the leak itself on it.

If there is a deterministic way to force error descriptors on this device -
a specific register write or a known-bad traffic pattern - I am happy to run
it and report what the counters and /proc/slabinfo do.

Federico