[PATCH net v2] net: ntb_netdev: Fix statistics races

[PATCH net v2] net: ntb_netdev: Fix statistics races

[Koichiro Den]

ntb_netdev updates shared net_device stats from per-QP RX and TX
callbacks. Once multiple queues are enabled, concurrent updates can be
lost.

Use per-CPU tstats for packet and byte counters and DEV_STATS_INC() for
less frequent drop and error counters. Callbacks can run synchronously
in the xmit path or asynchronously from a tasklet or the memcpy kthread.
Pin TX updates against migration in the kthread path. Use the IRQ-safe
u64_stats helpers because netpoll can invoke the synchronous path with
IRQs disabled.

Note that transport callbacks may still complete after
unregister_netdev(), so tie the tstats lifetime to the client device.

Fixes: 24d9e73c7e00 ("net: ntb_netdev: Support ethtool channels for multi-queue")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v2:
  - Make packet and byte updates IRQ-safe in netpoll context (Simon, Sashiko)
  - Use device-managed tstats and restore the original teardown order

This is the follow-up mentioned here:
https://lore.kernel.org/r/20260819172539.1450821-1-den@valinux.co.jp/
The related TX and RX fixes have now landed in net.
---
 drivers/net/ntb_netdev.c | 44 +++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index 9c171697e762..ccbafad8f650 100644
--- a/drivers/net/ntb_netdev.c
+++ b/drivers/net/ntb_netdev.c
@@ -125,10 +125,12 @@ static void ntb_netdev_event_handler(void *data, int link_is_up)
 static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 				  void *data, int len)
 {
+	struct pcpu_sw_netstats *tstats;
 	struct ntb_netdev_queue *q = qp_data;
 	struct ntb_netdev *dev = q->ntdev;
 	struct sk_buff *skb, *new_skb;
 	struct net_device *ndev;
+	unsigned long flags;
 	int rc;
 
 	ndev = dev->ndev;
@@ -139,17 +141,20 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 	netdev_dbg(ndev, "%s: %d byte payload received\n", __func__, len);
 
 	if (len < 0) {
-		ndev->stats.rx_errors++;
-		ndev->stats.rx_length_errors++;
+		DEV_STATS_INC(ndev, rx_errors);
+		DEV_STATS_INC(ndev, rx_length_errors);
 		goto enqueue_again;
 	}
 
-	ndev->stats.rx_packets++;
-	ndev->stats.rx_bytes += len;
+	tstats = this_cpu_ptr(ndev->tstats);
+	flags = u64_stats_update_begin_irqsave(&tstats->syncp);
+	u64_stats_inc(&tstats->rx_packets);
+	u64_stats_add(&tstats->rx_bytes, len);
+	u64_stats_update_end_irqrestore(&tstats->syncp, flags);
 
 	new_skb = netdev_alloc_skb(ndev, ndev->mtu + ETH_HLEN);
 	if (!new_skb) {
-		ndev->stats.rx_dropped++;
+		DEV_STATS_INC(ndev, rx_dropped);
 		goto enqueue_again;
 	}
 
@@ -166,8 +171,8 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 	rc = ntb_transport_rx_enqueue(qp, skb, skb->data, ndev->mtu + ETH_HLEN);
 	if (rc) {
 		dev_kfree_skb_any(skb);
-		ndev->stats.rx_errors++;
-		ndev->stats.rx_fifo_errors++;
+		DEV_STATS_INC(ndev, rx_errors);
+		DEV_STATS_INC(ndev, rx_fifo_errors);
 	}
 }
 
@@ -208,10 +213,12 @@ static int ntb_netdev_maybe_stop_tx(struct net_device *ndev,
 static void ntb_netdev_tx_handler(struct ntb_transport_qp *qp, void *qp_data,
 				  void *data, int len)
 {
+	struct pcpu_sw_netstats *tstats;
 	struct ntb_netdev_queue *q = qp_data;
 	struct ntb_netdev *dev = q->ntdev;
 	struct net_device *ndev;
 	struct sk_buff *skb;
+	unsigned long flags;
 
 	ndev = dev->ndev;
 	skb = data;
@@ -219,11 +226,16 @@ static void ntb_netdev_tx_handler(struct ntb_transport_qp *qp, void *qp_data,
 		return;
 
 	if (len > 0) {
-		ndev->stats.tx_packets++;
-		ndev->stats.tx_bytes += skb->len;
+		/* The memcpy kthread can migrate, so pin the per-CPU update. */
+		tstats = get_cpu_ptr(ndev->tstats);
+		flags = u64_stats_update_begin_irqsave(&tstats->syncp);
+		u64_stats_inc(&tstats->tx_packets);
+		u64_stats_add(&tstats->tx_bytes, skb->len);
+		u64_stats_update_end_irqrestore(&tstats->syncp, flags);
+		put_cpu_ptr(ndev->tstats);
 	} else {
-		ndev->stats.tx_errors++;
-		ndev->stats.tx_aborted_errors++;
+		DEV_STATS_INC(ndev, tx_errors);
+		DEV_STATS_INC(ndev, tx_aborted_errors);
 	}
 
 	dev_kfree_skb_any(skb);
@@ -277,7 +289,7 @@ static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
 
 drop:
 	dev_kfree_skb_any(skb);
-	ndev->stats.tx_dropped++;
+	DEV_STATS_INC(ndev, tx_dropped);
 	return NETDEV_TX_OK;
 }
 
@@ -433,6 +445,7 @@ static const struct net_device_ops ntb_netdev_ops = {
 	.ndo_start_xmit = ntb_netdev_start_xmit,
 	.ndo_change_mtu = ntb_netdev_change_mtu,
 	.ndo_set_mac_address = eth_mac_addr,
+	.ndo_get_stats64 = dev_get_tstats64,
 };
 
 static void ntb_get_drvinfo(struct net_device *ndev,
@@ -646,6 +659,13 @@ static int ntb_netdev_probe(struct device *client_dev)
 		goto err_free_netdev;
 	}
 
+	ndev->tstats = devm_netdev_alloc_pcpu_stats(client_dev,
+						    struct pcpu_sw_netstats);
+	if (!ndev->tstats) {
+		rc = -ENOMEM;
+		goto err_free_queues;
+	}
+
 	ndev->features = NETIF_F_HIGHDMA;
 
 	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;

base-commit: 1b78070aaef63512688aebfbc82365ef9d6660f1
-- 
2.51.0

Re: [PATCH net v2] net: ntb_netdev: Fix statistics races

[Eric Dumazet]

On Fri, Aug 28, 2026 at 5:41 PM Koichiro Den <den@valinux.co.jp> wrote:
>
> ntb_netdev updates shared net_device stats from per-QP RX and TX
> callbacks. Once multiple queues are enabled, concurrent updates can be
> lost.
>
> Use per-CPU tstats for packet and byte counters and DEV_STATS_INC() for
> less frequent drop and error counters. Callbacks can run synchronously
> in the xmit path or asynchronously from a tasklet or the memcpy kthread.
> Pin TX updates against migration in the kthread path. Use the IRQ-safe
> u64_stats helpers because netpoll can invoke the synchronous path with
> IRQs disabled.
>
> Note that transport callbacks may still complete after
> unregister_netdev(), so tie the tstats lifetime to the client device.
>
> Fixes: 24d9e73c7e00 ("net: ntb_netdev: Support ethtool channels for multi-queue")
> Cc: stable@vger.kernel.org
> Signed-off-by: Koichiro Den <den@valinux.co.jp>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net v2] net: ntb_netdev: Fix statistics races

[Jakub Kicinski]

On Sat, 29 Aug 2026 00:41:22 +0900 Koichiro Den wrote:
> Note that transport callbacks may still complete after
> unregister_netdev(), so tie the tstats lifetime to the client device.

We shouldn't really call devm_netdev_alloc_pcpu_stats() for types
of stats declared by netdev core. We should set the pointer type
like you did in v1.

Could you instead fix the fact that the Tx callback runs on 
an unregistered device? Maybe just wrap the whole body of
ntb_netdev_tx_handler() in RCU and check 

	READ_ONCE(ndev->reg_state) == NETREG_REGISTERED

?

Re: [PATCH net v2] net: ntb_netdev: Fix statistics races

[Jakub Kicinski]

On Fri, 28 Aug 2026 13:49:57 -0700 Jakub Kicinski wrote:
> On Sat, 29 Aug 2026 00:41:22 +0900 Koichiro Den wrote:
> > Note that transport callbacks may still complete after
> > unregister_netdev(), so tie the tstats lifetime to the client device.  
> 
> We shouldn't really call devm_netdev_alloc_pcpu_stats() for types
> of stats declared by netdev core. We should set the pointer type
> like you did in v1.
> 
> Could you instead fix the fact that the Tx callback runs on 
> an unregistered device? Maybe just wrap the whole body of
> ntb_netdev_tx_handler() in RCU and check 
> 
> 	READ_ONCE(ndev->reg_state) == NETREG_REGISTERED
> 
> ?

Maybe it's over-complicating. IDK.

Re: [PATCH net v2] net: ntb_netdev: Fix statistics races

[Koichiro Den]

On Fri, Aug 28, 2026 at 02:10:48PM -0700, Jakub Kicinski wrote:
> On Fri, 28 Aug 2026 13:49:57 -0700 Jakub Kicinski wrote:
> > On Sat, 29 Aug 2026 00:41:22 +0900 Koichiro Den wrote:
> > > Note that transport callbacks may still complete after
> > > unregister_netdev(), so tie the tstats lifetime to the client device.  
> > 
> > We shouldn't really call devm_netdev_alloc_pcpu_stats() for types
> > of stats declared by netdev core. We should set the pointer type
> > like you did in v1.

Thanks for pointing that out, I wasn't aware of that.

> > 
> > Could you instead fix the fact that the Tx callback runs on 
> > an unregistered device? Maybe just wrap the whole body of
> > ntb_netdev_tx_handler() in RCU and check 
> > 
> > 	READ_ONCE(ndev->reg_state) == NETREG_REGISTERED
> > 
> > ?
> 
> Maybe it's over-complicating. IDK.

I think the RCU approach is clean enough. RCU plus the NETREG_REGISTERED
check should address the stats lifetime issue without moving transport
teardown under RTNL.

I'll respin and let the core manage tstats by setting
NETDEV_PCPU_STAT_TSTATS, as v1 did for dstats. Late completions will still
consume the skb, but skip accounting and queue wake.

Also, looking again at the commit message paragraph you quoted, I realized
it was poorly written and confusing. What I meant was:

    Tie tstats to the client device so they remain alive until transport
    queue teardown. Moving that teardown into .ndo_uninit() would put
    potentially long DMA waits under RTNL, so leave it after
    unregister_netdev(). TX completions may therefore still update stats in
    between.

After reworking it as you suggested, the last paragraph in v3 will say:

    Let the core manage tstats while keeping transport teardown after
    unregister_netdev(), outside RTNL. RCU lets unregister wait for a
    completion already updating stats, while later completions only consume
    the skb and skip accounting and queue wake.

Best regards,
Koichiro