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

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

[Hanson Wang]

aq_nic_stop() disables TX and drops the carrier, then cancels the
service work, frees the link IRQ and disables NAPI. A link update or
TX completion can turn the carrier back on and wake queues after that
first netif_tx_disable(). ndo_set_features, ethtool ring changes and
XDP setup call aq_ndev_close() directly, without dev_deactivate(), so
a re-woken queue is still reached from ndo_start_xmit. aq_nic_deinit()
then frees buff_ring and leaves sw_tail unchanged.

On a UGREEN DXP2800 GT running 6.18.15 this oopses in aq_nic_map_skb()
from the IGMP timer softirq. The faulting store is dx_buff->flags = 0
at R10+0x28 with R10 == 0xc00: buff_ring is NULL and sw_tail is still
non-zero. Every module is unsigned, so the kernel is tainted E; the
atlantic driver itself is the in-tree one.

 BUG: kernel NULL pointer dereference, address: 0000000000000c28
 #PF: supervisor write access in kernel mode
 Oops: Oops: 0002 [#1] SMP NOPTI
 CPU: 4 UID: 0 PID: 0 Comm: swapper/4 Tainted: G E 6.18.15
 Hardware name: UGREEN DXP2800 GT/DXP2800 GT, BIOS APS15021 04/14/2026
 RIP: 0010:aq_nic_map_skb+0x302/0x570 [atlantic]
 Code: 44 0f b6 78 06 <49> c7 42 28 00 00 00 00
 R10: 0000000000000c00
 CR2: 0000000000000c28
 Call Trace:
  <IRQ>
  aq_nic_xmit+0xc6/0x150 [atlantic]
  dev_hard_start_xmit+0x63/0x1c0
  sch_direct_xmit+0x154/0x3a0
  __dev_queue_xmit+0xb8c/0xe80
  ip_finish_output2+0x25f/0x580
  ip_output+0x65/0x100
  igmp_ifc_timer_expire+0x1d5/0x420
  ...

Stop and drain SKB transmit again after those wake sources are gone,
and drop the carrier in the same place, before hw_stop(). The rings
are freed by the caller only after aq_nic_stop() returns.
netif_tx_disable() holds each queue lock, so it waits until
sch_direct_xmit() has left ndo_start_xmit; the stopped bit then keeps
later SKB transmits out. hw_stop() only masks interrupts and does not
wake queues.

ndo_xdp_xmit is a separate path and is not covered.

The barrier below has not been booted on the failing NAS. The oops is
from the unpatched kernel. The diagnosis and this changelog were
prepared with an AI coding assistant from that oops and the driver
sources.

Fixes: 3aec6412e007 ("aquantia: Fix Tx queue hangups")
Cc: stable@vger.kernel.org
Signed-off-by: Hanson Wang <hanson.wang@ugreen.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index 42e0fe9248f2..a6ceec0a22da 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);
+
 	return self->aq_hw_ops->hw_stop(self->aq_hw);
 }
 

base-commit: 21a7f44fd0765fcc5194130b28767786eba1d1e7

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

[Hanson Wang]

Igor Russkikh is no longer at Marvell. Cc'ing the current atlantic maintainer.

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

[netdev-bot+sashiko]

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