Re: [PATCH net] net: atlantic: stop TX again before freeing rings

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 (1) · High: 1 · 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] The fix depends on the TX queue stopped bit set by the new
  second netif_tx_disable() in aq_nic_stop() (aq_nic.c:1454).

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but
  aq_xdp_xmit() (aq_ring.c:416) never checks that the NIC's TX rings…

--

net: atlantic: stop TX again before freeing rings

This patch adds a second netif_tx_disable() and netif_carrier_off() at the
end of aq_nic_stop(). They run after the service work, IRQs and NAPI have
been torn down. The aim is to keep SKB transmits out of rings that
aq_nic_deinit() is about to free during the driver-internal close/open done
by ndo_set_features, ethtool ring changes and XDP setup.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index 42e0fe9248f28..a6ceec0a22dac 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -1448,6 +1448,12 @@ int aq_nic_stop(struct aq_nic_s *self)
>  
>  	aq_ptp_ring_stop(self);
>  
> +	/* Link updates and TX completion may have woken queues after the
> +	 * first disable. Drain SKB transmit again before the rings are freed.
> +	 */
> +	netif_tx_disable(self->ndev);
> +	netif_carrier_off(self->ndev);

[Severity: High]

Can the driver's own ndo_xdp_xmit path clear the stopped bit again after
this netif_tx_disable()?

The commit message says "the stopped bit then keeps later SKB transmits
out". It describes ndo_xdp_xmit only as a path that "is not covered".

aq_nic_ndev_init() always advertises NETDEV_XDP_ACT_NDO_XMIT. During the
internal close done by aq_ndev_set_features(), aq_set_ringparam() and
aq_xdp_setup(), the device stays IFF_UP. So a frame redirected from another
NIC can still reach aq_xdp_xmit()->aq_nic_xmit_xdpf(), which does:

aq_nic_xmit_xdpf() {
	...
	__netif_tx_lock(nq, cpu);

	aq_ring_update_queue_state(tx_ring);

	/* Above status update may stop the queue. Check this. */
	if (__netif_subqueue_stopped(aq_nic_get_ndev(aq_nic), queue_index))
		goto out;
	...
}

aq_ring_update_queue_state() only looks at descriptor accounting:

aq_ring_update_queue_state() {
	if (aq_ring_avail_dx(ring) <= AQ_CFG_SKB_FRAGS_MAX)
		aq_ring_queue_stop(ring);
	else if (aq_ring_avail_dx(ring) > AQ_CFG_RESTART_DESC_THRES)
		aq_ring_queue_wake(ring);
}

A stopped, drained ring has plenty of free descriptors. So this calls
aq_ring_queue_wake()->netif_wake_subqueue(), which clears the stopped bit
and reschedules the qdisc.

Suppose one redirected frame arrives after this new netif_tx_disable() but
before aq_ring_free() in aq_nic_deinit(). That window includes hw_stop(),
with its descriptor cache poll of up to 10 ms, and aq_vec_deinit().

Wouldn't the next qdisc or IGMP timer transmit then pass the stopped check
in sch_direct_xmit() and reach aq_nic_xmit()->aq_nic_map_skb() with
buff_ring == NULL?

That looks like the same NULL + sw_tail store as the oops in the changelog.
An SKB that gets in before the free could also race with
aq_ring_tx_deinit(), which walks buff_ring without a lock.

The driver clears the stopped bit itself, so a second netif_tx_disable()
doesn't seem to be enough. Would one of these be needed instead?

- a driver-side running or rings-valid check in the xmit paths
- skipping the wake in aq_ring_update_queue_state() during close
- netif_device_detach()/netif_device_attach() around the internal
  close/open

[Severity: Critical]

This is a pre-existing issue and was not introduced by this patch, but
aq_xdp_xmit() never checks that the TX rings exist before it uses them:

aq_xdp_xmit() {
	...
	ring = aq_nic->aq_ring_tx[AQ_NIC_CFG_TCVEC2RING(aq_cfg, 0, vec)];
	...
}

In the set_features and xdp_setup close windows, aq_ring_free() has set
buff_ring to NULL, but aq_ring_tx[] still points at the ring structs. After
the wake described above, aq_nic_map_xdp() writes through
&ring->buff_ring[ring->sw_tail]:

	dx = ring->sw_tail;
	dx_buff = &ring->buff_ring[dx];
	dx_buff->flags = 0U;

Is that a NULL + offset write?

The aq_set_ringparam() path looks worse:

aq_set_ringparam() {
	...
	err = aq_nic_realloc_vectors(aq_nic);
	if (err)
		goto err_exit;

	if (ndev_running)
		err = aq_ndev_open(ndev);
	...
}

aq_nic_realloc_vectors()->aq_nic_free_vectors()->aq_vec_free() calls
kfree() on each aq_vec, and struct aq_ring_s is embedded in struct aq_vec_s.

aq_ring_tx[] is never cleared. It is set only by aq_vec_ring_alloc() via
aq_nic_set_tx_ring(), which runs at the next aq_nic_init().

Between the realloc and the reopen, can redirected XDP frames read and write
freed aq_vec memory through aq_ring_tx[]? That would include
ring->stats.tx.queue_restarts, the buff_ring/sw_tail state in
aq_nic_map_xdp(), and the hw_ring_tx_xmit() updates.

If aq_nic_realloc_vectors() or aq_ndev_open() fails, the device stays
IFF_UP, so that window never closes.

>  	return self->aq_hw_ops->hw_stop(self->aq_hw);
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261031032308.2991494-1-hanson.wang%40ugreen.com