The hip04 remove path frees the TX/RX rings before unregistering the
netdev. If the interface is still up, unregister_netdev() then runs
.ndo_stop, whose TX reclaim and NAPI poll touch the already-freed DMA
ring memory. The TX coalesce timer and the platform IRQ also outlive the
netdev private data they dereference.

Reorder hip04_remove() so the netdev is unregistered (which runs .ndo_stop
synchronously, stopping NAPI and the TX queue) before the rings are freed.
Free the devm-managed IRQ explicitly before free_netdev(), so
hip04_mac_interrupt() (whose dev_id is the netdev) cannot fire against
freed memory: devm would otherwise release it only after .remove returns.

hip04_mac_stop() must quiesce both arming sites of the coalesce timer.
The NAPI poll arms it, and napi_disable() returns once the poll calls
napi_complete_done(), not when the poll function returns, so move that
arm before napi_complete_done().  The early-exit paths in hip04_rx_poll()
do not call napi_complete_done(); NAPI stays scheduled and will be polled
again, so there is no need to arm the coalesce timer on that pass.  The
Tx path also arms it, and mac_stop() is reached directly from
hip04_tx_timeout_task() as well as via .ndo_stop, so use netif_tx_disable()
rather than netif_stop_queue() to wait for an in-flight
hip04_mac_start_xmit() to finish.  The timer is then drained with
hrtimer_cancel().  A "closing" flag, checked at the single arming site,
guards against a later arm.

hip04_tx_timeout_task() restarts the device with mac_stop() + mac_open().
Serialize that restart against .ndo_open/.ndo_stop with a driver-private
state_lock instead of rtnl_lock(): mac_open()/mac_stop() are split into
internal __hip04_mac_open()/__hip04_mac_stop() helpers, and the
.ndo_open/.ndo_stop wrappers as well as the timeout worker take the lock.
This avoids extending RTNL usage; netdev_lock() is not sufficient here, as
hip04 does not opt into netdev-ops locking.  If timeout recovery is in
progress when a close begins, .ndo_stop waits on the lock and performs the
final MAC stop; if .ndo_stop acquires the lock first, the worker sees the
device is no longer running and returns.  Either way, recovery cannot
re-enable the MAC
during teardown.

This issue was found by an in-house static analysis tool.

Fixes: a41ea46a9a12 ("net: hisilicon: new hip04 ethernet driver")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.5
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---

v3: address review from Przemek Kitszel.

- Drop the rtnl_lock()/rtnl_unlock() and the #include <linux/rtnetlink.h>
  added to hip04_tx_timeout_task() in v2. Serialize the tx-timeout restart
  against .ndo_open/.ndo_stop with a driver-private state_lock instead:
  mac_open()/mac_stop() are split into __hip04_mac_open()/__hip04_mac_stop()
  helpers, and the .ndo_open/.ndo_stop wrappers and the timeout worker all
  take the lock. This avoids new RTNL usage. netdev_lock() was considered
  but does not help: hip04 does not opt into netdev-ops locking, so its
  .ndo_open/.ndo_stop run under RTNL and the netdev instance lock does not
  protect them.

- Rephrase the sentence about napi_complete_done() / early-exit paths that
  was hard to read.

- s/Tx xmit/Tx/.

v2: Address review comments from Simon Horman. Use netif_tx_disable() rather
   than netif_stop_queue() in hip04_mac_stop() so that an in-flight
   hip04_mac_start_xmit() finishes before the TX ring is reclaimed;
   mac_stop() is also called directly from the tx-timeout work, not only
   via .ndo_stop. Arm the coalesce timer in hip04_rx_poll() before
   napi_complete_done(), so that napi_disable() observes the final arm and
   the subsequent cancel cannot miss it. Free the devm-managed IRQ
   explicitly in hip04_remove() before free_netdev() so that
   hip04_mac_interrupt() cannot run against freed memory, placing it after
   unregister_netdev() so that the device is stopped first. Reorder
   hip04_remove() so that the netdev is unregistered (running .ndo_stop)
   before the PHY is disconnected and the TX/RX rings are freed. Check the
   return value of hip04_mac_open() in the tx-timeout restart path.

v2: https://lore.kernel.org/netdev/20260710015730.630775-1-fanwu01@zju.edu.cn/
v1: https://lore.kernel.org/netdev/20260703050133.2445155-1-fanwu01@zju.edu.cn/

 drivers/net/ethernet/hisilicon/hip04_eth.c | 107 ++++++++++++++++++++++++++----
 1 file changed, 96 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index 18376bcc7..4bd192094 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -15,6 +15,7 @@
 #include <linux/of_net.h>
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
+#include <linux/mutex.h>
 
 #define SC_PPE_RESET_DREQ		0x026C
 
@@ -232,6 +233,8 @@ struct hip04_priv {
 	int tx_coalesce_frames;
 	int tx_coalesce_usecs;
 	struct hrtimer tx_coalesce_timer;
+	bool closing;
+	struct mutex state_lock; /* Serializes MAC open/stop and timeout recovery. */
 
 	unsigned char *rx_buf[RX_DESC_NUM];
 	dma_addr_t rx_phys[RX_DESC_NUM];
@@ -497,6 +500,12 @@ static void hip04_start_tx_timer(struct hip04_priv *priv)
 {
 	unsigned long ns = priv->tx_coalesce_usecs * NSEC_PER_USEC / 2;
 
+	/* Do not (re-)arm the Tx coalesce timer once teardown has begun.
+	 * Both arming sites (Tx and NAPI Rx poll) go through here.
+	 */
+	if (smp_load_acquire(&priv->closing))
+		return;
+
 	/* allow timer to fire after half the time at the earliest */
 	hrtimer_start_range_ns(&priv->tx_coalesce_timer, ns_to_ktime(ns),
 			       ns, HRTIMER_MODE_REL);
@@ -649,12 +658,15 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
 		priv->reg_inten |= RCV_INT;
 		writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
 	}
+	/* Arm the coalesce timer BEFORE napi_complete_done(): napi_disable()
+	 * in hip04_mac_stop() returns once SCHED is cleared here, not when
+	 * the poll function returns, so arming afterwards can slip past the
+	 * stop path's hrtimer_cancel().
+	 */
+	if (tx_remaining)
+		hip04_start_tx_timer(priv);
 	napi_complete_done(napi, rx);
 done:
-	/* start a new timer if necessary */
-	if (rx < budget && tx_remaining)
-		hip04_start_tx_timer(priv);
-
 	return rx;
 }
 
@@ -720,7 +732,7 @@ static void hip04_adjust_link(struct net_device *ndev)
 	}
 }
 
-static int hip04_mac_open(struct net_device *ndev)
+static int __hip04_mac_open(struct net_device *ndev)
 {
 	struct hip04_priv *priv = netdev_priv(ndev);
 	int i;
@@ -743,6 +755,13 @@ static int hip04_mac_open(struct net_device *ndev)
 		hip04_set_recv_desc(priv, phys);
 	}
 
+	/* RX mappings are established; clear the closing flag before
+	 * re-enabling traffic.  The store-release pairs with the load-acquire
+	 * in hip04_start_tx_timer(); state_lock serializes this against
+	 * mac_stop()'s store-release.
+	 */
+	smp_store_release(&priv->closing, false);
+
 	if (priv->phy)
 		phy_start(priv->phy);
 
@@ -754,13 +773,42 @@ static int hip04_mac_open(struct net_device *ndev)
 	return 0;
 }
 
-static int hip04_mac_stop(struct net_device *ndev)
+static int hip04_mac_open(struct net_device *ndev)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+	int ret;
+
+	mutex_lock(&priv->state_lock);
+	ret = __hip04_mac_open(ndev);
+	mutex_unlock(&priv->state_lock);
+	return ret;
+}
+
+static int __hip04_mac_stop(struct net_device *ndev)
 {
 	struct hip04_priv *priv = netdev_priv(ndev);
 	int i;
 
+	/* Stop new timer arms before draining: set the closing flag (checked
+	 * at the single arming site), wait for the NAPI poll and any in-flight
+	 * TX to finish, then cancel the timer.
+	 *
+	 * netif_tx_disable() (not netif_stop_queue()) is required because this
+	 * function is also called directly from hip04_tx_timeout_task(), not
+	 * only via .ndo_stop where the core has already deactivated TX;
+	 * netif_tx_disable() waits for an in-flight hip04_mac_start_xmit(),
+	 * which arms the timer, to finish.
+	 *
+	 * Because hip04_rx_poll() arms the timer before napi_complete_done(),
+	 * napi_disable() returning means that arm has happened, so the
+	 * hrtimer_cancel() below cannot miss it.  The store-release pairs
+	 * with the load in hip04_start_tx_timer().
+	 */
+	smp_store_release(&priv->closing, true);
+
 	napi_disable(&priv->napi);
-	netif_stop_queue(ndev);
+	netif_tx_disable(ndev);
+	hrtimer_cancel(&priv->tx_coalesce_timer);
 	hip04_mac_disable(ndev);
 	hip04_tx_reclaim(ndev, true);
 	hip04_reset_ppe(priv);
@@ -779,6 +827,16 @@ static int hip04_mac_stop(struct net_device *ndev)
 	return 0;
 }
 
+static int hip04_mac_stop(struct net_device *ndev)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+
+	mutex_lock(&priv->state_lock);
+	__hip04_mac_stop(ndev);
+	mutex_unlock(&priv->state_lock);
+	return 0;
+}
+
 static void hip04_timeout(struct net_device *ndev, unsigned int txqueue)
 {
 	struct hip04_priv *priv = netdev_priv(ndev);
@@ -791,8 +849,23 @@ static void hip04_tx_timeout_task(struct work_struct *work)
 	struct hip04_priv *priv;
 
 	priv = container_of(work, struct hip04_priv, tx_timeout_task);
-	hip04_mac_stop(priv->ndev);
-	hip04_mac_open(priv->ndev);
+
+	/* Serialize the restart with .ndo_open/.ndo_stop, which take the
+	 * same lock.  If a close has completed or is in progress the
+	 * netif_running() check bails; if that check races the core's
+	 * running-state clear, .ndo_stop still runs under state_lock
+	 * afterwards and stops the device, so this worker cannot leave the
+	 * MAC re-enabled against a teardown.
+	 */
+	mutex_lock(&priv->state_lock);
+	if (!netif_running(priv->ndev))
+		goto out;
+
+	__hip04_mac_stop(priv->ndev);
+	if (__hip04_mac_open(priv->ndev))
+		netdev_err(priv->ndev, "restart after tx timeout failed\n");
+out:
+	mutex_unlock(&priv->state_lock);
 }
 
 static int hip04_get_coalesce(struct net_device *netdev,
@@ -983,6 +1056,7 @@ static int hip04_mac_probe(struct platform_device *pdev)
 	}
 
 	INIT_WORK(&priv->tx_timeout_task, hip04_tx_timeout_task);
+	mutex_init(&priv->state_lock);
 
 	ndev->netdev_ops = &hip04_netdev_ops;
 	ndev->ethtool_ops = &hip04_ethtool_ops;
@@ -1026,13 +1100,24 @@ static void hip04_remove(struct platform_device *pdev)
 	struct hip04_priv *priv = netdev_priv(ndev);
 	struct device *d = &pdev->dev;
 
+	unregister_netdev(ndev);
+
+	/* The IRQ is devm-managed and would otherwise be freed only after
+	 * this function returns.  Free it now, after unregister_netdev() has
+	 * run .ndo_stop to stop the device and mask its interrupt source, but
+	 * before the manual free_netdev() below, so that hip04_mac_interrupt()
+	 * (dev_id == ndev) cannot fire against freed memory.  free_irq() also
+	 * drains any in-flight handler.
+	 */
+	devm_free_irq(d, ndev->irq, ndev);
+	cancel_work_sync(&priv->tx_timeout_task);
+	hrtimer_cancel(&priv->tx_coalesce_timer);
+
 	if (priv->phy)
 		phy_disconnect(priv->phy);
 
 	hip04_free_ring(ndev, d);
-	unregister_netdev(ndev);
 	of_node_put(priv->phy_node);
-	cancel_work_sync(&priv->tx_timeout_task);
 	free_netdev(ndev);
 }

On Sat, 11 Jul 2026 05:29:22 +0000 Fan Wu wrote:
> This issue was found by an in-house static analysis tool.

Do you have the HW and were you able to test this patch?
If yes please add this to the commit message.
If no please leave this driver alone, it's old.
-- 
pw-bot: cr