[PATCH net-next v2] net: ag71xx: align the IP header behind DSA tags

[PATCH net-next v2] net: ag71xx: align the IP header behind DSA tags

[Rosen Penev]

The receive buffer offset includes NET_IP_ALIGN to get the IP header
aligned behind a 14 byte Ethernet header. Most ag71xx devices use DSA
switches whose tag sits in front of the EtherType, which moves the IP
header by another 2 bytes. Every access to the IP and TCP headers then
traps and gets emulated by the unaligned access handler, around 6 times
per forwarded frame.

Take the headroom needed by the DSA tagger into account when choosing
the offset.

TP-Link Archer C7 v2 (qca8k), median of 3 runs, Mbit/s:

                       before   after
  routed up               743     855
  routed down             660     749
  local receive           554     572

The unaligned instruction counter no longer moves while forwarding.

Assisted-by: LLM
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 v2: rebase and ass Reviewed-by.
 drivers/net/ethernet/atheros/ag71xx.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
index 4e4794c4dfdc..ff30ebce448d 100644
--- a/drivers/net/ethernet/atheros/ag71xx.c
+++ b/drivers/net/ethernet/atheros/ag71xx.c
@@ -38,6 +38,7 @@
 #include <linux/reset.h>
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <net/dsa.h>
 #include <net/selftests.h>
 
 /* For our NAPI weight bigger does *NOT* mean better - it means more
@@ -1401,6 +1402,24 @@ static void ag71xx_hw_disable(struct ag71xx *ag)
 	ag71xx_rings_cleanup(ag);
 }
 
+static unsigned int ag71xx_rx_offset(struct ag71xx *ag)
+{
+	unsigned int align = NET_IP_ALIGN;
+
+	if (ag71xx_is(ag, AR7100) || ag71xx_is(ag, AR9130))
+		return NET_SKB_PAD;
+
+#if IS_ENABLED(CONFIG_NET_DSA)
+	/* A tag inserted in front of the EtherType shifts the IP header,
+	 * compensate for it to avoid unaligned accesses in the stack.
+	 */
+	if (netdev_uses_dsa(ag->ndev))
+		align = (align - ag->ndev->dsa_ptr->tag_ops->needed_headroom) & 3;
+#endif
+
+	return NET_SKB_PAD + align;
+}
+
 static int ag71xx_open(struct net_device *ndev)
 {
 	struct ag71xx *ag = netdev_priv(ndev);
@@ -1415,6 +1434,7 @@ static int ag71xx_open(struct net_device *ndev)
 	}
 
 	max_frame_len = ag71xx_max_frame_len(ndev->mtu);
+	ag->rx_buf_offset = ag71xx_rx_offset(ag);
 	ag->rx_buf_size =
 		SKB_DATA_ALIGN(max_frame_len + NET_SKB_PAD + NET_IP_ALIGN);
 
@@ -1879,10 +1899,6 @@ static int ag71xx_probe(struct platform_device *pdev)
 	ndev->min_mtu = 68;
 	ndev->max_mtu = dcfg->max_frame_len - ag71xx_max_frame_len(0);
 
-	ag->rx_buf_offset = NET_SKB_PAD;
-	if (!ag71xx_is(ag, AR7100) && !ag71xx_is(ag, AR9130))
-		ag->rx_buf_offset += NET_IP_ALIGN;
-
 	if (ag71xx_is(ag, AR7100)) {
 		ag->tx_ring.desc_split = AG71XX_TX_RING_SPLIT;
 		tx_size *= AG71XX_TX_RING_DS_PER_PKT;
-- 
2.56.0

Re: [PATCH net-next v2] net: ag71xx: align the IP header behind DSA tags

[netdev-bot+sashiko]

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

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

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] net: ag71xx: 1-byte DMA and memory buffer overflow in RX path
- [Medium] The DSA-dependent RX offset is only computed in ag71xx_open().

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  ag71xx_change_mtu() can be called on a running interface.
- [High] This problem wasn't introduced by this patch, but if DSA conduit
  setup fails, the conduit keeps a dangling dsa_ptr.

--

net: ag71xx: align the IP header behind DSA tags

This patch moves the rx_buf_offset computation from ag71xx_probe() to
ag71xx_open(). When the netdev is a DSA conduit, it also adjusts
NET_IP_ALIGN by the tagger's needed_headroom, so the IP header behind the
tag is 4-byte aligned. The commit message reports a gain in routing
throughput on a qca8k based board.

> diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
> index 4e4794c4dfdce..ff30ebce448dc 100644
> --- a/drivers/net/ethernet/atheros/ag71xx.c
> +++ b/drivers/net/ethernet/atheros/ag71xx.c

[ ... ]

> @@ -1401,6 +1402,24 @@ static void ag71xx_hw_disable(struct ag71xx *ag)
>  	ag71xx_rings_cleanup(ag);
>  }
>  
> +static unsigned int ag71xx_rx_offset(struct ag71xx *ag)
> +{
> +	unsigned int align = NET_IP_ALIGN;
> +
> +	if (ag71xx_is(ag, AR7100) || ag71xx_is(ag, AR9130))
> +		return NET_SKB_PAD;
> +
> +#if IS_ENABLED(CONFIG_NET_DSA)
> +	/* A tag inserted in front of the EtherType shifts the IP header,
> +	 * compensate for it to avoid unaligned accesses in the stack.
> +	 */
> +	if (netdev_uses_dsa(ag->ndev))
> +		align = (align - ag->ndev->dsa_ptr->tag_ops->needed_headroom) & 3;

[Severity: High]
This is a pre-existing issue in the DSA core, not something this patch
introduced. Still, can dsa_ptr be left dangling when this runs?

dsa_conduit_setup() in net/dsa/conduit.c sets the pointer before two steps
that can fail:

net/dsa/conduit.c:dsa_conduit_setup() {
    ...
	dev->dsa_ptr = cpu_dp;

	dsa_conduit_set_promiscuity(dev, 1);

	ret = dsa_conduit_ethtool_setup(dev);
	if (ret)
		goto out_err_reset_promisc;

	ret = sysfs_create_group(&dev->dev.kobj, &dsa_group);
	if (ret)
		goto out_err_ethtool_teardown;
    ...
out_err_ethtool_teardown:
	dsa_conduit_ethtool_teardown(dev);
out_err_reset_promisc:
	dsa_conduit_set_promiscuity(dev, -1);
	return ret;
}

Neither error label clears dev->dsa_ptr. On that failure, dsa_tree_setup()
jumps to teardown_ports and skips dsa_tree_teardown_conduit(), so
dsa_conduit_teardown() never runs. The CPU dsa_port is then freed:

dsa_switch_probe()
  dsa_tree_setup()
    dsa_tree_setup_conduit()
      dsa_conduit_setup()      <- sets dsa_ptr, then fails
    goto teardown_ports        <- dsa_conduit_teardown() skipped
  dsa_switch_release_ports()
    kfree(dp)                  <- conduit->dsa_ptr still points here

After that, every received frame already reads dsa_ptr->rcv from freed
memory through eth_type_trans()->netdev_uses_dsa(). With this patch,
ag71xx_open() also reads dsa_ptr->tag_ops->needed_headroom from the freed
dsa_port.

Triggering it needs an allocation or sysfs failure during switch probe.
The fix probably belongs in the dsa_conduit_setup() error path rather than
in this driver.

> +#endif
> +
> +	return NET_SKB_PAD + align;
> +}
> +
>  static int ag71xx_open(struct net_device *ndev)
>  {
>  	struct ag71xx *ag = netdev_priv(ndev);
> @@ -1415,6 +1434,7 @@ static int ag71xx_open(struct net_device *ndev)
>  	}
>  
>  	max_frame_len = ag71xx_max_frame_len(ndev->mtu);
> +	ag->rx_buf_offset = ag71xx_rx_offset(ag);

[Severity: High]
ag71xx_rx_offset() can now return anything from NET_SKB_PAD + 0 to
NET_SKB_PAD + 3. The rx_buf_size computed right below is still sized
for NET_SKB_PAD + NET_IP_ALIGN. Is that enough when the tagger's
needed_headroom is 3 mod 4, so that (2 - needed_headroom) & 3 == 3?

ag71xx_fill_rx_buf() maps rx_buf_size bytes from the start of the
buffer. It then points the descriptor at dma_addr + offset. The MAC
may write up to max_frame_len bytes there, FCS included, because that
is what MAC_MFL is programmed with.

With an offset of NET_SKB_PAD + 3, the write ends at
NET_SKB_PAD + 3 + max_frame_len. That is one byte past
max_frame_len + NET_SKB_PAD + NET_IP_ALIGN, which is the value
rx_buf_size was derived from.

The SKB_DATA_ALIGN() rounding usually hides this. On ath79, though,
NET_SKB_PAD and SMP_CACHE_BYTES are both 32 and max_frame_len is
mtu + 22. Whenever mtu + 56 is a multiple of the cache line size there
is no slack at all.

In that case the MAC writes one byte past the region dma_map_single()
covered. That byte lands in the skb_shared_info area, which
napi_build_skb() places at data + rx_buf_size via
ag71xx_buffer_size(). On a non-coherent MIPS system this also means
the byte sits outside the range that was invalidated for the device.

I'm not sure any tagger used with ag71xx today has such a
needed_headroom. Nothing in ag71xx_rx_offset() rules it out, though,
and before this patch the offset could never exceed
NET_SKB_PAD + NET_IP_ALIGN.

Would it make sense to size the buffer from the offset actually in
use, for example SKB_DATA_ALIGN(max_frame_len + ag->rx_buf_offset)?
Alternatively, the offset could be clamped so it never exceeds
NET_SKB_PAD + NET_IP_ALIGN.

[Severity: Medium]
Can this cached offset get out of sync with the DSA state of a running
interface? It is only computed in ag71xx_open(). The refill path,
ag71xx_rx_packets(), the restart work and ag71xx_change_mtu() all reuse
ag->rx_buf_offset.

dsa_tree_setup_conduit() accepts a conduit that is already IFF_UP:

net/dsa/dsa.c:dsa_tree_setup_conduit() {
    ...
	bool admin_up = (conduit->flags & IFF_UP) && !qdisc_tx_is_noop(conduit);

	err = dsa_conduit_setup(conduit, cpu_dp);
    ...
}

dsa_conduit_setup() sets dev->dsa_ptr, and afterwards only the admin and
oper state is replayed. There is no ndo_stop/ndo_open cycle.

The dev_set_mtu() inside dsa_conduit_setup() only reaches
ag71xx_change_mtu(), which writes MAC_MFL. dsa_user_open() calls
dev_open(conduit), but that returns early because the conduit is already
up.

So if the switch binds after eth0 is up (late module load, deferred probe,
ip= or nfsroot bring-up), tagged traffic keeps NET_SKB_PAD + NET_IP_ALIGN
until the interface is cycled. In that case the patch has no effect.

The reverse case looks like a new regression. If the switch is unbound
while eth0 stays up, dsa_conduit_teardown() clears the pointer without
closing the conduit:

net/dsa/conduit.c:dsa_conduit_teardown() {
    ...
	dsa_conduit_set_promiscuity(dev, -1);

	dev->dsa_ptr = NULL;
    ...
}

For the qca and ar9331 taggers the cached offset is NET_SKB_PAD + 0,
because (2 - 2) & 3 == 0. Plain untagged frames then put the IP header at
an offset that is 2 mod 4, so every IP and TCP header access goes through
unaligned access emulation. Before this patch, the non-DSA case was always
aligned. dsa_switch_shutdown() closes the conduits before it clears
dsa_ptr, so it is not affected.

Could the offset be recomputed when a DSA tree attaches to or detaches from
the conduit, or could this limitation be documented?

>  	ag->rx_buf_size =
>  		SKB_DATA_ALIGN(max_frame_len + NET_SKB_PAD + NET_IP_ALIGN);

[Severity: High]
This isn't a bug introduced by this patch, but rx_buf_size is only derived
from the MTU here in ag71xx_open(). Can ag71xx_change_mtu() let the MAC
overflow the RX buffers if the MTU is raised while the interface is
running?

drivers/net/ethernet/atheros/ag71xx.c:ag71xx_change_mtu() {
	struct ag71xx *ag = netdev_priv(ndev);

	WRITE_ONCE(ndev->mtu, new_mtu);
	ag71xx_wr(ag, AG71XX_REG_MAC_MFL,
		  ag71xx_max_frame_len(ndev->mtu));

	return 0;
}

There is no netif_running() check, and the RX ring is not rebuilt. The RX
descriptor has no buffer length field. Once MFL is raised, the MAC can DMA
frames up to the new limit into buffers that ag71xx_fill_rx_buf() mapped
with the old rx_buf_size.

ag71xx_rx_packets() then does:

		skb = napi_build_skb(ring->buf[i].rx.rx_buf, ag71xx_buffer_size(ag));
		...
		skb_reserve(skb, offset);
		skb_put(skb, pktlen);

Take a SoC with a 1540 byte max frame length and the non-DSA offset of 34.
Raising the MTU from 1500 to 1518 while up leaves rx_buf_size at
ALIGN(1556, 32) = 1568. Offset 34 plus pktlen 1536 is 1570, so skb_put()
hits skb_over_panic().

On AR934x/QCA953x, where MFL can be up to 16K - 1, an MTU of 9000 would let
the DMA write several KB past the end of the page frag.

dsa_user_change_mtu() can reach the same path by raising the conduit MTU
while it is up. ag71xx_fast_reset() also reprograms MFL from ndev->mtu
without resizing the buffers.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009223024.545607-1-rosenp%40gmail.com