[PATCH net v2] net: atl1c: drop oversized RX frames instead of panicking

[PATCH net v2] net: atl1c: drop oversized RX frames instead of panicking

[Federico Musto]

atl1c sizes its receive buffers from the netdev MTU plus a fixed set of L2
overheads, which comes to 1522 bytes at the default MTU.  A link partner
that sends a longer frame - in practice a MikroTik CCR2004-1G-2XS-PCIe,
whose RouterOS side defaults to l2mtu 1600 while the host stays at MTU 1500
- makes the length reported by the RX return descriptor exceed what the
hardware wrote, and skb_put() then either includes bytes that were never
received or trips skb_over_panic and takes the host down.

Compare the descriptor length against buffer_info->length, the length the
hardware was given, rather than against skb_tailroom(): the skb carries 64
bytes of slack for the DMA erratum, so tailroom is larger than the DMA
buffer and a frame in between would pass the check and still be delivered
with a tail the device never wrote.  The same test rejects frames shorter
than ETH_HLEN + ETH_FCS_LEN; before this change a reported length below
ETH_FCS_LEN underflowed in skb_put().

The frame is dropped with kfree_skb_reason(SKB_DROP_REASON_PKT_TOO_BIG) so
drop_monitor and the kfree_skb tracepoint see it, and counted through
dev_core_stats_rx_dropped_inc(), which survives atl1c_get_stats()
overwriting the hardware counters on every read.  It also counts as work:
atl1c_clean_rfd() has already recycled the RFD, and the refill at the end
of the poll only runs when work_done is non-zero, so sustained oversized
traffic would otherwise drain the ring and stop RX.

Tested on a MikroTik CCR2004-1G-2XS-PCIe (4x AR8151) in a Proxmox VE host,
with atl1c built from a DKMS tree carrying this change: the l2mtu-1600
traffic that panics the stock driver is dropped and counted with the patch
applied, and received normally once the host MTU is raised to match.

This complements commit 36c2009d90f2 ("net: atl1c: fix soft lockup on
out-of-range tpd_cons read"), which fixed the TX side of the same
"host dies while the card resets" problem.

Fixes: 43250ddd75a3 ("atl1c: Atheros L1C Gigabit Ethernet driver")
Cc: stable@vger.kernel.org
Signed-off-by: Federico Musto <federico@rimpianto.com>
---
v1 -> v2, after the Sashiko review:
 - bound the length by buffer_info->length instead of skb_tailroom(): the skb
   is allocated with 64 bytes of slack for the DMA erratum, so tailroom is
   larger than what the device wrote and a frame in between would have been
   delivered with stale tail bytes
 - count the dropped frame as work, so the RX ring is refilled; without it
   sustained oversized traffic could drain the RFD ring and stop RX
 - kfree_skb_reason() with SKB_DROP_REASON_PKT_TOO_BIG instead of
   dev_kfree_skb(), which is consume_skb(), and drop the rx_length_errors
   increment that atl1c_get_stats() overwrites anyway
 - the commit message now mentions the runt rejection as well

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

Not touched here: the RRS_RX_ERR_SUM branch frees neither the skb nor its
DMA mapping (atl1c_clean_rfd() only marks the slot free and drops the
pointer).  That is pre-existing and independent of the panic, so it belongs
in a separate patch.
---
 .../net/ethernet/atheros/atl1c/atl1c_main.c   | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index e58f1d2..eddd840 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -1934,6 +1934,39 @@ rrs_checked:
 			break;
 		}
 		atl1c_clean_rfd(rfd_ring, rrs, rfd_num);
+
+		/* A link partner whose l2mtu is larger than ours can hand us a
+		 * frame that does not fit the buffer the hardware was given:
+		 * the descriptor reports the untruncated length while at most
+		 * buffer_info->length bytes were written.  skb_put() of such a
+		 * length would pull in whatever the tail of the buffer holds
+		 * and, past the end of the skb, trips skb_over_panic and takes
+		 * the host down.  Bound it by what the device actually wrote
+		 * rather than by skb_tailroom(): the skb carries 64 bytes of
+		 * slack for the DMA erratum, so the two are not the same.
+		 *
+		 * Frames shorter than an Ethernet header plus its FCS are
+		 * rejected by the same test; before this, a length below
+		 * ETH_FCS_LEN underflowed in skb_put().
+		 */
+		if (unlikely(length < ETH_HLEN + ETH_FCS_LEN ||
+			     length > buffer_info->length)) {
+			dev_core_stats_rx_dropped_inc(netdev);
+			if (netif_msg_rx_err(adapter))
+				dev_warn_ratelimited(&pdev->dev,
+						     "dropped rx frame: %u bytes, buffer %u\n",
+						     length, buffer_info->length);
+			kfree_skb_reason(skb, SKB_DROP_REASON_PKT_TOO_BIG);
+			/* atl1c_clean_rfd() has already recycled the RFD, so
+			 * count the drop as work: the refill at the end of the
+			 * poll only runs when work_done is non-zero, and
+			 * sustained oversized traffic would otherwise drain the
+			 * ring and stop RX for good.
+			 */
+			work_done++;
+			continue;
+		}
+
 		skb_put(skb, length - ETH_FCS_LEN);
 		skb->protocol = eth_type_trans(skb, netdev);
 		atl1c_rx_checksum(adapter, skb, rrs);
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v2] net: atl1c: drop oversized RX frames instead of panicking

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (3) · Medium: 1 (2) · Low: 1

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

- [Medium] In atl1c_clean_rx()
  (drivers/net/ethernet/atheros/atl1c/atl1c_main.c:1952-1953), the new…
- [Low] atl1c_clean_rx() sends runt frames (length < ETH_HLEN +
  ETH_FCS_LEN) down the same drop path as oversized frames and frees…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the
  RRS_RX_ERR_SUM / RRS_802_3_LEN_ERR branch of atl1c_clean_rx()…
- [High] This problem wasn't introduced by this patch (the author
  explicitly leaves it out of scope), but the RRS_RX_ERR_SUM /…
- [High] This problem wasn't introduced by this patch, but the link-
  change path of atl1c_common_task() (line 349-353) resets and refills…
- [Medium] This problem wasn't introduced by this patch, but when an RRD
  reports rfd_num != 1, atl1c_clean_rx() calls atl1c_clean_rrd(rrd_ring,…
- [Medium] This problem wasn't introduced by this patch, but struct
  atl1c_recv_ret_status declares word0 and word3 as __le32, and…

--

net: atl1c: drop oversized RX frames instead of panicking

This patch adds a length check to atl1c_clean_rx(). It drops RX frames
whose descriptor length is shorter than ETH_HLEN + ETH_FCS_LEN or longer
than buffer_info->length, so skb_put() can no longer panic. Each dropped
frame is freed with SKB_DROP_REASON_PKT_TOO_BIG, counted as rx_dropped,
and counted as NAPI work so the RFD ring still gets refilled.

> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index e58f1d2c26bdf..eddd8406867f7 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -1934,6 +1934,39 @@ static int atl1c_clean_rx(struct napi_struct *napi, int budget)
>  			break;
>  		}

[Severity: Medium]
This is a pre-existing issue, but the new check only runs when
rfd_num == 1. What happens to the RFDs behind a descriptor with
rfd_num != 1?

By the time this break is reached, atl1c_clean_rrd() has already consumed
the RRD. The branch then exits without dma_unmap_single(), without freeing
the skb and without atl1c_clean_rfd(). Those slots stay
ATL1C_BUFFER_BUSY.

Refill in atl1c_alloc_rx_buffer() stops at the first slot that is not
free:

    while (next_info->flags & ATL1C_BUFFER_FREE) {

Once next_to_use reaches one of these slots, does RX stall for good? The
skbs and DMA mappings would leak as well.

With REG_MTU larger than REG_RX_BUF_SIZE for MTUs 1501-1522 (see below),
any frame the hardware splits across RFDs would come through here. It is
not clear when this hardware reports rfd_num > 1, though. The 1600 byte
frames in the commit message arrived with rfd_num == 1.

>  		atl1c_clean_rfd(rfd_ring, rrs, rfd_num);
> +
> +		/* A link partner whose l2mtu is larger than ours can hand us a

[ ... ]

> +		if (unlikely(length < ETH_HLEN + ETH_FCS_LEN ||
> +			     length > buffer_info->length)) {

[Severity: Medium]
With an MTU between 1501 and 1522, can this bound drop frames that fit
the MTU and that the MAC was set up to accept?

atl1c_set_rxbufsize() compares the L3 MTU with AT_RX_BUF_SIZE. That
constant is an L2 frame size, ETH_FRAME_LEN + VLAN_HLEN + ETH_FCS_LEN =
1522:

    adapter->rx_buffer_len = mtu > AT_RX_BUF_SIZE ?
        roundup(mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN, 8) : AT_RX_BUF_SIZE;

So for mtu <= 1522, rx_buffer_len, REG_RX_BUF_SIZE and buffer_info->length
all stay at 1522. atl1c_configure_mac() meanwhile programs:

    AT_WRITE_REG(hw, REG_MTU, hw->max_frame_size + ETH_HLEN +
            VLAN_HLEN + ETH_FCS_LEN);

In that MTU range this comes to as much as 1544.

Take MTU 1508 (RFC 4638 PPPoE). A full-size untagged frame is
1508 + 14 + 4 = 1526 bytes. The MAC accepts it, but this check drops it
as SKB_DROP_REASON_PKT_TOO_BIG. Before the patch the 64 bytes of slack
meant skb_put() took the frame and it was delivered.

The REG_MTU and REG_RX_BUF_SIZE mismatch was already there. With this
check it becomes silent loss of traffic that is within the MTU.

Could rx_buffer_len be sized from mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN
instead, for example max(AT_RX_BUF_SIZE, roundup(...))? Then the bound
enforced here would match what the MAC accepts.

[Severity: Medium]
This is a pre-existing issue and the patch does not make it worse, but is
length correct on big-endian hosts? struct atl1c_recv_ret_status declares
word0 and word3 as __le32. atl1c_clean_rx() masks the raw word and only
then byte-swaps a 16-bit value:

    length = le16_to_cpu((rrs->word3 >> RRS_PKT_SIZE_SHIFT) &
            RRS_PKT_SIZE_MASK);

rfd_num and rfd_index are read from rrs->word0 without conversion, and
RRS_RXD_IS_VALID(rrs->word3) also tests the raw word.

On big-endian, this bound would compare a meaningless length against the
buffer_info of the wrong slot. Should the length be
(le32_to_cpu(rrs->word3) >> RRS_PKT_SIZE_SHIFT) & RRS_PKT_SIZE_MASK
instead?

> +			dev_core_stats_rx_dropped_inc(netdev);
> +			if (netif_msg_rx_err(adapter))
> +				dev_warn_ratelimited(&pdev->dev,
> +						     "dropped rx frame: %u bytes, buffer %u\n",
> +						     length, buffer_info->length);
> +			kfree_skb_reason(skb, SKB_DROP_REASON_PKT_TOO_BIG);

[Severity: Low]
Runt frames (length < ETH_HLEN + ETH_FCS_LEN) take this path too. Won't
drop_monitor and the kfree_skb tracepoint then report them as
SKB_DROP_REASON_PKT_TOO_BIG?

SKB_DROP_REASON_PKT_TOO_SMALL already exists for that case. Could the
reason depend on which bound failed?

[Severity: High]
This is a pre-existing issue, but could the link-change worker already
have freed this skb?

atl1c_common_task() resets the rings on a link change without stopping
NAPI:

    if (test_and_clear_bit(ATL1C_WORK_EVENT_LINK_CHANGE,
        &adapter->work_event)) {
        atl1c_irq_disable(adapter);
        atl1c_check_link_status(adapter);
        atl1c_irq_enable(adapter);
    }

atl1c_irq_disable() masks interrupts and calls synchronize_irq(). That
waits for the hardirq handler, but not for a NAPI poll already running on
another CPU.

On link down the worker runs:

atl1c_check_link_status()
    netif_carrier_off()
    atl1c_reset_mac()
    atl1c_reset_dma_ring()->atl1c_clean_rx_ring()->atl1c_clean_buffer()
    atl1c_configure()->atl1c_alloc_rx_buffer(..., false)

A poll that passed its netif_carrier_ok() check before the carrier went
off may still hold skb = buffer_info->skb. After that it passes the skb to
kfree_skb_reason() here, or to skb_put()/napi_gro_receive(). It can also
run atl1c_alloc_rx_buffer() at the same time as the worker.

Can this end in a double free or use-after-free of the RX skbs, and a
corrupted ring? atl1c_clean_rx() also rewrites REG_IMR at the end, which
undoes the irq disable. atl1c_down() quiesces NAPI properly. Should the
link-change path do the same?

> +			/* atl1c_clean_rfd() has already recycled the RFD, so
> +			 * count the drop as work: the refill at the end of the
> +			 * poll only runs when work_done is non-zero, and
> +			 * sustained oversized traffic would otherwise drain the
> +			 * ring and stop RX for good.
> +			 */
> +			work_done++;
> +			continue;
> +		}

[Severity: High]
This is a pre-existing issue, but the error branch earlier in
atl1c_clean_rx() seems to have the same ring drain problem that this
comment describes:

    if (rrs->word3 & (RRS_RX_ERR_SUM | RRS_802_3_LEN_ERR)) {
        atl1c_clean_rfd(rfd_ring, rrs, rfd_num);
        if (netif_msg_rx_err(adapter))
            dev_warn(&pdev->dev,
                 "wrong packet! rrs word3 is %x\n",
                 rrs->word3);
        continue;
    }

atl1c_clean_rfd() marks the slot ATL1C_BUFFER_FREE and advances
next_to_clean, but work_done is never incremented. The refill only runs
when work_done is non-zero:

    if (work_done)
        atl1c_alloc_rx_buffer(adapter, rrd_ring->num, true);

Suppose a run of errored frames (CRC errors, or a crafted 802.3 length
field mismatch) uses up every posted RFD with no good frame in between.
Each poll then ends with work_done == 0 and the ring is never refilled.
The ISR_OVER handler in atl1c_intr() only logs.

Can RX stop permanently in that case? This branch also does not count
against the NAPI budget, so one poll can consume any number of errored
descriptors.

[Severity: High]
Also pre-existing, and the v2 notes say it is deliberately left for a
separate patch, but the same error branch leaks one skb and one streaming
DMA mapping per errored frame. Since it is High severity, it is
reported here as well.

The branch runs before dma_unmap_single() and before the skb is fetched.
atl1c_clean_rfd() only does:

    buffer_info[rfd_index].skb = NULL;
    ATL1C_SET_BUFFER_STATE(&buffer_info[rfd_index],
                ATL1C_BUFFER_FREE);

Teardown cannot recover the slot either, because atl1c_clean_buffer()
skips free slots:

    if (buffer_info->flags & ATL1C_BUFFER_FREE)
        return;

atl1c_alloc_rx_buffer() then overwrites buffer_info->skb and
buffer_info->dma.

Could a peer sending CRC errored or length mismatched frames grow this
leak without bound, using up memory and IOMMU or swiotlb space? It might
be worth fixing in the follow-up patch, together with the missing
work_done accounting above.

>  		skb_put(skb, length - ETH_FCS_LEN);
>  		skb->protocol = eth_type_trans(skb, netdev);
>  		atl1c_rx_checksum(adapter, skb, rrs);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027115151.17092-1-federico%40rimpianto.com