[PATCH net] net: ag71xx: reallocate rx buffers when changing the MTU

[PATCH net] net: ag71xx: reallocate rx buffers when changing the MTU

[Rosen Penev]

The rx buffers are sized from the MTU in ag71xx_open(), but
ag71xx_change_mtu() writes the new maximum frame length straight to
the MAC while the interface is running. The rx descriptors carry no
buffer length, so after raising the MTU the MAC accepts and DMAs
frames larger than the mapped buffer, overwriting the skb_shared_info
at the end of the fragment and whatever follows it.

This is reachable without user action: DSA raises the MTU of a
running conduit to make room for the switch tag.

Restart the interface when the MTU changes while it is up, so the
rings are refilled with buffers sized for the new MTU before the MAC
is programmed with the new frame length.

Fixes: d51b6ce441d3 ("net: ethernet: add ag71xx driver")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/atheros/ag71xx.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
index 8a3c689f6825..3c9de8c2ba64 100644
--- a/drivers/net/ethernet/atheros/ag71xx.c
+++ b/drivers/net/ethernet/atheros/ag71xx.c
@@ -1761,11 +1761,19 @@ static irqreturn_t ag71xx_interrupt(int irq, void *dev_id)
 
 static int ag71xx_change_mtu(struct net_device *ndev, int new_mtu)
 {
-	struct ag71xx *ag = netdev_priv(ndev);
+	bool running = netif_running(ndev);
+
+	/* The rx buffers are sized for the MTU at open time and the rx
+	 * descriptors carry no buffer length, so a larger MTU must not
+	 * reach the MAC before the rings have been refilled.
+	 */
+	if (running)
+		ag71xx_stop(ndev);
 
 	WRITE_ONCE(ndev->mtu, new_mtu);
-	ag71xx_wr(ag, AG71XX_REG_MAC_MFL,
-		  ag71xx_max_frame_len(ndev->mtu));
+
+	if (running)
+		return ag71xx_open(ndev);
 
 	return 0;
 }
-- 
2.56.0

Re: [PATCH net] net: ag71xx: reallocate rx buffers when changing the MTU

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

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - 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: ag71xx: reallocate rx buffers when changing the MTU

[Rosen Penev]

On Sun, Oct 18, 2026 at 6:48 PM <netdev-bot+sinfo@kernel.org> wrote:
>
> Hi!
>
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.
>
>  - 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.
Sashiko flagged this on a performance improvement for net-next.
>
>  - 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: ag71xx: reallocate rx buffers when changing the MTU

[Andrew Lunn]

On Sun, Oct 18, 2026 at 06:44:38PM -0700, Rosen Penev wrote:
> The rx buffers are sized from the MTU in ag71xx_open(), but
> ag71xx_change_mtu() writes the new maximum frame length straight to
> the MAC while the interface is running. The rx descriptors carry no
> buffer length, so after raising the MTU the MAC accepts and DMAs
> frames larger than the mapped buffer, overwriting the skb_shared_info
> at the end of the fragment and whatever follows it.
> 
> This is reachable without user action: DSA raises the MTU of a
> running conduit to make room for the switch tag.
> 
> Restart the interface when the MTU changes while it is up, so the
> rings are refilled with buffers sized for the new MTU before the MAC
> is programmed with the new frame length.
> 
> Fixes: d51b6ce441d3 ("net: ethernet: add ag71xx driver")
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/net/ethernet/atheros/ag71xx.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
> index 8a3c689f6825..3c9de8c2ba64 100644
> --- a/drivers/net/ethernet/atheros/ag71xx.c
> +++ b/drivers/net/ethernet/atheros/ag71xx.c
> @@ -1761,11 +1761,19 @@ static irqreturn_t ag71xx_interrupt(int irq, void *dev_id)
>  
>  static int ag71xx_change_mtu(struct net_device *ndev, int new_mtu)
>  {
> -	struct ag71xx *ag = netdev_priv(ndev);
> +	bool running = netif_running(ndev);
> +
> +	/* The rx buffers are sized for the MTU at open time and the rx
> +	 * descriptors carry no buffer length, so a larger MTU must not
> +	 * reach the MAC before the rings have been refilled.
> +	 */
> +	if (running)
> +		ag71xx_stop(ndev);

This appears to free the rings.

>  
>  	WRITE_ONCE(ndev->mtu, new_mtu);
> -	ag71xx_wr(ag, AG71XX_REG_MAC_MFL,
> -		  ag71xx_max_frame_len(ndev->mtu));
> +
> +	if (running)
> +		return ag71xx_open(ndev);

And this tries to allocate them again. If this fails, there is no way
back, the interface is dead.

This is not how MTU change should work. If it fails, it should leave
the old MTU in place.

    Andrew

---
pw-bot: cr

Re: [PATCH net] net: ag71xx: reallocate rx buffers when changing the MTU

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 0 · 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] ag71xx_change_mtu() does not handle a failed reopen.
- [High] ag71xx_change_mtu() calls ag71xx_stop() and ag71xx_open()
  directly while the netdev is still running and its qdisc and TX path…

--

net: ag71xx: reallocate rx buffers when changing the MTU

This patch makes ag71xx_change_mtu() stop and reopen a running interface.
The rx rings are then refilled with buffers sized for the new MTU before
the MAC frame length register is programmed. Before this, the new max frame
length was written straight to the MAC while the rings still held buffers
sized for the old MTU.

> diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
> index 4e4794c4dfdce..6db5ab8be6aef 100644
> --- a/drivers/net/ethernet/atheros/ag71xx.c
> +++ b/drivers/net/ethernet/atheros/ag71xx.c
> @@ -1766,11 +1766,19 @@ static irqreturn_t ag71xx_interrupt(int irq, void *dev_id)
>  
>  static int ag71xx_change_mtu(struct net_device *ndev, int new_mtu)
>  {
> -	struct ag71xx *ag = netdev_priv(ndev);
> +	bool running = netif_running(ndev);
> +
> +	/* The rx buffers are sized for the MTU at open time and the rx
> +	 * descriptors carry no buffer length, so a larger MTU must not
> +	 * reach the MAC before the rings have been refilled.
> +	 */
> +	if (running)
> +		ag71xx_stop(ndev);

[Severity: High]

Is it safe to call ag71xx_stop() here while the qdisc and tx path are
still live?

On the normal ndo_stop path, __dev_close_many() clears __LINK_STATE_START
and runs dev_deactivate_many() before it calls ndo_stop, so no xmit is in
flight. This path skips that step. ag71xx_hw_disable() only does this:

ag71xx_hw_disable() {
	netif_stop_queue(ag->ndev);

	ag71xx_hw_stop(ag);
	ag71xx_dma_reset(ag);

	napi_disable(&ag->napi);
	timer_delete_sync(&ag->oom_timer);

	ag71xx_rings_cleanup(ag);
}

netif_stop_queue() does not take the tx lock, so it doesn't wait for a CPU
that is already inside ag71xx_hard_start_xmit().

NAPI is also still enabled during ag71xx_hw_stop() and ag71xx_dma_reset(),
which can mdelay for several ms. A poll in that window can re-open the
queue:

ag71xx_poll()
  ag71xx_tx_packets()
    if ((ring->curr - ring->dirty) < (ring_size * 3) / 4)
        netif_wake_queue(ag->ndev);

ag71xx_hard_start_xmit() takes no driver lock. It writes
ring->buf[i].tx.skb and the descriptors, updates ring->curr and BQL, and
sets TX_CTRL_TXE. That restarts the tx DMA engine that ag71xx_dma_reset()
just stopped.

Could this let an xmit run at the same time as ag71xx_rings_cleanup() and
ag71xx_rings_free(), which kfree(tx->buf) and dma_free_coherent() the
descriptors?

If the queue was woken, an xmit after the rings are freed but before
ag71xx_open() would dereference tx->descs_cpu and tx->buf after they have
been set to NULL. An xmit that races with ag71xx_ring_tx_init() and
netdev_reset_queue() during the reopen could also corrupt curr/dirty and
the BQL state.

On DSA setups this can happen while other user ports are still
transmitting through the conduit, because dsa_user_change_mtu() calls
dev_set_mtu() on the conduit.

ag71xx_restart_work_func() already uses the same unquiesced
hw_disable/hw_enable pattern. This patch adds a new trigger for it that is
reached routinely.

>  
>  	WRITE_ONCE(ndev->mtu, new_mtu);
> -	ag71xx_wr(ag, AG71XX_REG_MAC_MFL,
> -		  ag71xx_max_frame_len(ndev->mtu));
> +
> +	if (running)
> +		return ag71xx_open(ndev);

[Severity: High]

What happens to the device if ag71xx_open() fails here?

ag71xx_open() can fail in two places:

- phylink_of_phy_connect()
- ag71xx_hw_enable()->ag71xx_rings_init(), which returns -ENOMEM if any
  rx buffer allocation fails. Those buffers are now sized for the new,
  possibly larger, MTU.

Both failure paths return before napi_enable():

ag71xx_hw_enable() {
	ret = ag71xx_rings_init(ag);
	if (ret)
		return ret;

	napi_enable(&ag->napi);
	...
}

netif_set_mtu_ext() just passes the error up and does not close the device.
So netif_running() stays true while NAPI is disabled, the rings are freed
and the PHY is detached.

Any later ag71xx_hw_disable() would then call napi_disable() a second time.
That can come from ifdown or unregister through ag71xx_stop(), from another
MTU change, or from a pending restart_work that ag71xx_stop() doesn't
cancel.

Wouldn't napi_disable_locked() then spin forever with RTNL held?

net/core/dev.c:napi_disable_locked() {
    ...
		while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
			usleep_range(20, 200);
			val = READ_ONCE(n->state);
		}
    ...
}

The device state is also left inconsistent. ndev->mtu is already set to
new_mtu even though an error is returned, and NETDEV_CHANGEMTU is never
sent. Retrying with the same MTU returns 0 right away because of this
check in netif_set_mtu_ext():

	if (new_mtu == dev->mtu)
		return 0;

After a NETDEV_CHANGEMTU veto, netif_set_mtu_ext() rolls back by calling
__netif_set_mtu(dev, orig_mtu) and ignores the result. A failed reopen on
that path would leave the device in the same state without any error being
reported.

Before this patch, the handler could not fail. Should the error path
restore the old MTU and bring the device back up? Or should it at least
leave NAPI and the rings in a state that a later ag71xx_stop() can handle?

>  
>  	return 0;
>  }

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