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

[netdev-bot+sashiko@kernel.org]

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