[PATCH] net: hip04: quiesce tx coalesce timer before teardown

[PATCH] net: hip04: quiesce tx coalesce timer before teardown

[Fan Wu]

hip04_start_tx_timer() arms priv->tx_coalesce_timer from both the
TX path and the NAPI poll path. The timer callback, tx_done(), derives
priv from the timer and touches priv->napi, priv->reg_inten and
priv->base before scheduling NAPI.

The remove path currently frees the TX/RX rings before unregistering the
netdev. If the interface is still up, unregister_netdev() will run
.ndo_stop only after those rings have already been freed, while a pending
TX coalesce timer or NAPI instance can still reach the ring state. The
timer is also never cancelled before free_netdev() releases the netdev
private area.

Cancel the timer from hip04_mac_stop() after NAPI and the TX queue have
been disabled. In hip04_remove(), unregister the netdev first, drain the
timeout work and timer, and only then free the rings.

hip04_tx_timeout_task() can also restart the device by calling
hip04_mac_stop() followed by hip04_mac_open(). Serialize that restart with
rtnl_lock(), matching the netdev core's .ndo_stop locking, and skip it if
the device is no longer running.

Fixes: a41ea46a9a12 ("net: hisilicon: new hip04 ethernet driver")
Cc: stable@vger.kernel.org
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index 18376bc..cb9b01c 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/rtnetlink.h>
 
 #define SC_PPE_RESET_DREQ		0x026C
 
@@ -761,6 +762,13 @@ static int hip04_mac_stop(struct net_device *ndev)
 
 	napi_disable(&priv->napi);
 	netif_stop_queue(ndev);
+
+	/* Cancel the TX-coalesce timer after the arming paths (xmit via the
+	 * queue, rx poll via NAPI) are disabled, so a pending tx_done()
+	 * (which dereferences priv) is drained before the device is freed.
+	 */
+	hrtimer_cancel(&priv->tx_coalesce_timer);
+
 	hip04_mac_disable(ndev);
 	hip04_tx_reclaim(ndev, true);
 	hip04_reset_ppe(priv);
@@ -791,8 +799,15 @@ static void hip04_tx_timeout_task(struct work_struct *work)
 	struct hip04_priv *priv;
 
 	priv = container_of(work, struct hip04_priv, tx_timeout_task);
+
+	rtnl_lock();
+	if (!netif_running(priv->ndev))
+		goto out;
+
 	hip04_mac_stop(priv->ndev);
 	hip04_mac_open(priv->ndev);
+out:
+	rtnl_unlock();
 }
 
 static int hip04_get_coalesce(struct net_device *netdev,
@@ -1029,10 +1044,15 @@ static void hip04_remove(struct platform_device *pdev)
 	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);
+	hrtimer_cancel(&priv->tx_coalesce_timer);
+	/* Free the rings only after the interface is stopped (.ndo_stop via
+	 * unregister_netdev) and the work/timer are drained; the TX/NAPI
+	 * paths touch them while the device is up.
+	 */
+	hip04_free_ring(ndev, d);
+	of_node_put(priv->phy_node);
 	free_netdev(ndev);
 }
 

Re: [PATCH] net: hip04: quiesce tx coalesce timer before teardown

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: hip04: quiesce tx coalesce timer before teardown

This patch fixes race conditions with the tx_coalesce_timer by properly
canceling it during device stop and remove. It also serializes device
restarts in the tx timeout handler using rtnl_lock().

> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
> index 18376bcc718a2..cb9b01c79ee6e 100644
> --- a/drivers/net/ethernet/hisilicon/hip04_eth.c
> +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c

[ ... ]

> @@ -761,6 +762,13 @@ static int hip04_mac_stop(struct net_device *ndev)
>  
>  	napi_disable(&priv->napi);
>  	netif_stop_queue(ndev);

[Severity: High]
This is a pre-existing issue, but does netif_stop_queue() adequately protect
against concurrent transmission here?

Since netif_stop_queue() only changes the queue state flag without waiting for
an active ndo_start_xmit callback to finish (unlike netif_tx_disable()), could
this allow the subsequent hip04_tx_reclaim() call to corrupt the TX ring or
cause invalid DMA mappings if a transmission is actively running?

> +
> +	/* Cancel the TX-coalesce timer after the arming paths (xmit via the
> +	 * queue, rx poll via NAPI) are disabled, so a pending tx_done()
> +	 * (which dereferences priv) is drained before the device is freed.
> +	 */
> +	hrtimer_cancel(&priv->tx_coalesce_timer);

[Severity: High]
This is a pre-existing issue, but is it possible for the softirq to re-arm
the timer after hrtimer_cancel() is called?

In hip04_rx_poll(), the NAPI ownership is released via napi_complete_done()
before the timer is armed:

hip04_rx_poll() {
    ...
    napi_complete_done(napi, rx);
done:
    if (rx < budget && tx_remaining)
        hip04_start_tx_timer(priv);
}

If the teardown thread cancels the timer and frees the private structure
while the softirq is preempted just after napi_complete_done(), could this
result in a use-after-free when hip04_start_tx_timer() is eventually executed?

> +
>  	hip04_mac_disable(ndev);
>  	hip04_tx_reclaim(ndev, true);

[Severity: High]
This is a pre-existing issue, but is there missing hardware synchronization
between disabling the MAC and reclaiming the TX ring?

hip04_mac_disable() disables the MAC using a relaxed MMIO write without
polling for an idle state.

Since the very next call is hip04_tx_reclaim() which unmaps buffers and
modifies descriptor memory, could this cause silent data corruption or IOMMU
page faults if the MAC is still actively processing packets in the background?

[ ... ]

> @@ -791,8 +799,15 @@ static void hip04_tx_timeout_task(struct work_struct *work)
>  	struct hip04_priv *priv;
>  
>  	priv = container_of(work, struct hip04_priv, tx_timeout_task);
> +
> +	rtnl_lock();
> +	if (!netif_running(priv->ndev))
> +		goto out;
> +
>  	hip04_mac_stop(priv->ndev);
>  	hip04_mac_open(priv->ndev);

[Severity: High]
This is a pre-existing issue, but what happens if hip04_mac_open() fails
during this restart?

If dma_map_single() fails inside hip04_mac_open(), it returns -EIO without
enabling NAPI. Because this return value is ignored here, the device remains
broken and NAPI is left disabled (with the SCHED bit set).

Will this cause a hard lockup later when hip04_mac_stop() spins forever in
napi_disable() waiting for the SCHED bit to clear?

> +out:
> +	rtnl_unlock();
>  }
>  

[ ... ]

> @@ -1029,10 +1044,15 @@ static void hip04_remove(struct platform_device *pdev)
>  	if (priv->phy)
>  		phy_disconnect(priv->phy);
>  
> -	hip04_free_ring(ndev, d);
>  	unregister_netdev(ndev);

[Severity: High]
This is a pre-existing issue, but does calling phy_disconnect() before
unregister_netdev() cause a use-after-free and WARN?

phy_disconnect() halts and detaches the PHY, dropping its reference count.
The subsequent call to unregister_netdev() invokes hip04_mac_stop(), which
unconditionally calls phy_stop().

Could calling phy_stop() on a detached and potentially freed PHY device
pointer cause a crash?

> -	of_node_put(priv->phy_node);
>  	cancel_work_sync(&priv->tx_timeout_task);
> +	hrtimer_cancel(&priv->tx_coalesce_timer);
> +	/* Free the rings only after the interface is stopped (.ndo_stop via
> +	 * unregister_netdev) and the work/timer are drained; the TX/NAPI
> +	 * paths touch them while the device is up.
> +	 */
> +	hip04_free_ring(ndev, d);

[Severity: High]
This is a pre-existing issue, but does hip04_free_ring() safely handle
allocation failures?

During probe, if allocating the TX descriptor ring fails, priv->tx_desc
remains NULL, and the error path calls hip04_free_ring().

Since hip04_free_ring() unconditionally calls dma_free_coherent() on
priv->tx_desc, could this cause a kernel panic on architectures that trap
virtual-to-physical translations on NULL?

> +	of_node_put(priv->phy_node);
>  	free_netdev(ndev);

[Severity: High]
This is a pre-existing issue, but does explicitly freeing the netdev here
create a use-after-free race with the interrupt handler?

The driver requests its interrupt in hip04_mac_probe() using
devm_request_irq(), meaning the IRQ is only freed during devres cleanup after
hip04_remove() completely finishes.

If an interrupt fires between free_netdev() and the devres cleanup,
hip04_mac_interrupt() will cast dev_id to the freed ndev pointer, which seems
like it would lead to a use-after-free crash.

>  }
>  

[ ... ]

Regarding the RX polling path in hip04_rx_poll():

[Severity: High]
This is a pre-existing issue, but does a failure in build_skb() cause a memory
and DMA mapping leak?

Looking at hip04_rx_poll():

    if (unlikely(!skb)) {
        net_dbg_ratelimited("build_skb failed\n");
        goto refill;
    }
    ...
refill:
    ...
    priv->rx_buf[priv->rx_head] = buf;
    priv->rx_phys[priv->rx_head] = phys;

If build_skb() fails, the code jumps directly to the refill label, completely
bypassing the dma_unmap_single() call for the original buffer. It then
allocates and maps a new buffer, overwriting the array entries.

Could this permanently leak the original buffer and its DMA mapping,
eventually leading to an OOM crash under memory pressure?

Re: [PATCH] net: hip04: quiesce tx coalesce timer before teardown

[Fan Wu]

Hi Simon,

Thanks for the v1 review. The updated patch fixes the PHY teardown ordering
and IRQ lifetime issues: hip04_remove() unregisters the netdev, running
.ndo_stop and phy_stop(), before phy_disconnect(), and frees the devm-managed
IRQ before free_netdev().

The remaining NULL tx_desc cleanup in hip04_free_ring() and RX refill
failure paths are independent pre-existing error-path issues. I will address them 
in separate patches rather than expanding this teardown fix. In particular, the
RX fix must preserve the old descriptor mapping until a replacement buffer is
successfully allocated and mapped.

The latest revision is v3:
https://lore.kernel.org/netdev/20260711052922.1634837-1-fanwu01@zju.edu.cn

Thanks,
Fan

> On Jul 9, 2026, at 20:43, Simon Horman <horms@kernel.org> wrote:
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> net: hip04: quiesce tx coalesce timer before teardown
> 
> This patch fixes race conditions with the tx_coalesce_timer by properly
> canceling it during device stop and remove. It also serializes device
> restarts in the tx timeout handler using rtnl_lock().
> 
>> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
>> index 18376bcc718a2..cb9b01c79ee6e 100644
>> --- a/drivers/net/ethernet/hisilicon/hip04_eth.c
>> +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
> 
> [ ... ]
> 
>> @@ -761,6 +762,13 @@ static int hip04_mac_stop(struct net_device *ndev)
>> 
>> napi_disable(&priv->napi);
>> netif_stop_queue(ndev);
> 
> [Severity: High]
> This is a pre-existing issue, but does netif_stop_queue() adequately protect
> against concurrent transmission here?
> 
> Since netif_stop_queue() only changes the queue state flag without waiting for
> an active ndo_start_xmit callback to finish (unlike netif_tx_disable()), could
> this allow the subsequent hip04_tx_reclaim() call to corrupt the TX ring or
> cause invalid DMA mappings if a transmission is actively running?
> 
>> +
>> + /* Cancel the TX-coalesce timer after the arming paths (xmit via the
>> + * queue, rx poll via NAPI) are disabled, so a pending tx_done()
>> + * (which dereferences priv) is drained before the device is freed.
>> + */
>> + hrtimer_cancel(&priv->tx_coalesce_timer);
> 
> [Severity: High]
> This is a pre-existing issue, but is it possible for the softirq to re-arm
> the timer after hrtimer_cancel() is called?
> 
> In hip04_rx_poll(), the NAPI ownership is released via napi_complete_done()
> before the timer is armed:
> 
> hip04_rx_poll() {
>    ...
>    napi_complete_done(napi, rx);
> done:
>    if (rx < budget && tx_remaining)
>        hip04_start_tx_timer(priv);
> }
> 
> If the teardown thread cancels the timer and frees the private structure
> while the softirq is preempted just after napi_complete_done(), could this
> result in a use-after-free when hip04_start_tx_timer() is eventually executed?
> 
>> +
>> hip04_mac_disable(ndev);
>> hip04_tx_reclaim(ndev, true);
> 
> [Severity: High]
> This is a pre-existing issue, but is there missing hardware synchronization
> between disabling the MAC and reclaiming the TX ring?
> 
> hip04_mac_disable() disables the MAC using a relaxed MMIO write without
> polling for an idle state.
> 
> Since the very next call is hip04_tx_reclaim() which unmaps buffers and
> modifies descriptor memory, could this cause silent data corruption or IOMMU
> page faults if the MAC is still actively processing packets in the background?
> 
> [ ... ]
> 
>> @@ -791,8 +799,15 @@ static void hip04_tx_timeout_task(struct work_struct *work)
>> struct hip04_priv *priv;
>> 
>> priv = container_of(work, struct hip04_priv, tx_timeout_task);
>> +
>> + rtnl_lock();
>> + if (!netif_running(priv->ndev))
>> + goto out;
>> +
>> hip04_mac_stop(priv->ndev);
>> hip04_mac_open(priv->ndev);
> 
> [Severity: High]
> This is a pre-existing issue, but what happens if hip04_mac_open() fails
> during this restart?
> 
> If dma_map_single() fails inside hip04_mac_open(), it returns -EIO without
> enabling NAPI. Because this return value is ignored here, the device remains
> broken and NAPI is left disabled (with the SCHED bit set).
> 
> Will this cause a hard lockup later when hip04_mac_stop() spins forever in
> napi_disable() waiting for the SCHED bit to clear?
> 
>> +out:
>> + rtnl_unlock();
>> }
>> 
> 
> [ ... ]
> 
>> @@ -1029,10 +1044,15 @@ static void hip04_remove(struct platform_device *pdev)
>> if (priv->phy)
>> phy_disconnect(priv->phy);
>> 
>> - hip04_free_ring(ndev, d);
>> unregister_netdev(ndev);
> 
> [Severity: High]
> This is a pre-existing issue, but does calling phy_disconnect() before
> unregister_netdev() cause a use-after-free and WARN?
> 
> phy_disconnect() halts and detaches the PHY, dropping its reference count.
> The subsequent call to unregister_netdev() invokes hip04_mac_stop(), which
> unconditionally calls phy_stop().
> 
> Could calling phy_stop() on a detached and potentially freed PHY device
> pointer cause a crash?
> 
>> - of_node_put(priv->phy_node);
>> cancel_work_sync(&priv->tx_timeout_task);
>> + hrtimer_cancel(&priv->tx_coalesce_timer);
>> + /* Free the rings only after the interface is stopped (.ndo_stop via
>> + * unregister_netdev) and the work/timer are drained; the TX/NAPI
>> + * paths touch them while the device is up.
>> + */
>> + hip04_free_ring(ndev, d);
> 
> [Severity: High]
> This is a pre-existing issue, but does hip04_free_ring() safely handle
> allocation failures?
> 
> During probe, if allocating the TX descriptor ring fails, priv->tx_desc
> remains NULL, and the error path calls hip04_free_ring().
> 
> Since hip04_free_ring() unconditionally calls dma_free_coherent() on
> priv->tx_desc, could this cause a kernel panic on architectures that trap
> virtual-to-physical translations on NULL?
> 
>> + of_node_put(priv->phy_node);
>> free_netdev(ndev);
> 
> [Severity: High]
> This is a pre-existing issue, but does explicitly freeing the netdev here
> create a use-after-free race with the interrupt handler?
> 
> The driver requests its interrupt in hip04_mac_probe() using
> devm_request_irq(), meaning the IRQ is only freed during devres cleanup after
> hip04_remove() completely finishes.
> 
> If an interrupt fires between free_netdev() and the devres cleanup,
> hip04_mac_interrupt() will cast dev_id to the freed ndev pointer, which seems
> like it would lead to a use-after-free crash.
> 
>> }
>> 
> 
> [ ... ]
> 
> Regarding the RX polling path in hip04_rx_poll():
> 
> [Severity: High]
> This is a pre-existing issue, but does a failure in build_skb() cause a memory
> and DMA mapping leak?
> 
> Looking at hip04_rx_poll():
> 
>    if (unlikely(!skb)) {
>        net_dbg_ratelimited("build_skb failed\n");
>        goto refill;
>    }
>    ...
> refill:
>    ...
>    priv->rx_buf[priv->rx_head] = buf;
>    priv->rx_phys[priv->rx_head] = phys;
> 
> If build_skb() fails, the code jumps directly to the refill label, completely
> bypassing the dma_unmap_single() call for the original buffer. It then
> allocates and maps a new buffer, overwriting the array entries.
> 
> Could this permanently leak the original buffer and its DMA mapping,
> eventually leading to an OOM crash under memory pressure?