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

[Koichiro Den <den@valinux.co.jp>]

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.

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

Fixes: 24d9e73c7e00 ("net: ntb_netdev: Support ethtool channels for multi-queue")
Cc: stable@vger.kernel.org
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v3:
  - Use core-managed tstats and gate TX callback work under RCU (Jakub)
  - Fix reverse xmas tree ordering overlooked in v2

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

@Eric, thanks for reviewing v2. v3 moves tstats management back to the
core and adds RCU protection following Jakub's suggestion, so I did not
carry your R-b tag. Would appreciate another look when you have cycles.

v1: https://lore.kernel.org/r/20260824025720.3520288-1-den@valinux.co.jp/
v2: https://lore.kernel.org/r/20260828154122.2643578-1-den@valinux.co.jp/

Note: 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 already landed in net.
---
 drivers/net/ntb_netdev.c | 47 +++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index 9c171697e762..2c04be6d61a8 100644
--- a/drivers/net/ntb_netdev.c
+++ b/drivers/net/ntb_netdev.c
@@ -127,8 +127,10 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 {
 	struct ntb_netdev_queue *q = qp_data;
 	struct ntb_netdev *dev = q->ntdev;
+	struct pcpu_sw_netstats *tstats;
 	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);
 	}
 }
 
@@ -210,25 +215,39 @@ static void ntb_netdev_tx_handler(struct ntb_transport_qp *qp, void *qp_data,
 {
 	struct ntb_netdev_queue *q = qp_data;
 	struct ntb_netdev *dev = q->ntdev;
+	struct pcpu_sw_netstats *tstats;
 	struct net_device *ndev;
 	struct sk_buff *skb;
+	unsigned long flags;
+	bool registered;
 
 	ndev = dev->ndev;
 	skb = data;
 	if (!skb || !ndev)
 		return;
 
+	rcu_read_lock();
+	registered = READ_ONCE(ndev->reg_state) == NETREG_REGISTERED;
+	if (!registered)
+		goto free_skb;
+
 	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);
 	}
 
+free_skb:
 	dev_kfree_skb_any(skb);
 
-	if (ntb_transport_tx_free_entry(qp) >= tx_start) {
+	if (registered && ntb_transport_tx_free_entry(qp) >= tx_start) {
 		/* Make sure anybody stopping the queue after this sees the new
 		 * value of ntb_transport_tx_free_entry()
 		 */
@@ -237,6 +256,7 @@ static void ntb_netdev_tx_handler(struct ntb_transport_qp *qp, void *qp_data,
 		    ntb_transport_link_query(q->qp))
 			netif_wake_subqueue(ndev, q->qid);
 	}
+	rcu_read_unlock();
 }
 
 static const struct ntb_queue_handlers ntb_netdev_handlers = {
@@ -277,7 +297,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;
 }
 
@@ -647,6 +667,7 @@ static int ntb_netdev_probe(struct device *client_dev)
 	}
 
 	ndev->features = NETIF_F_HIGHDMA;
+	ndev->pcpu_stat_type = NETDEV_PCPU_STAT_TSTATS;
 
 	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
 

base-commit: 2188569e7e1b0bc3f3b557dc97ab7a02befc11c8
-- 
2.51.0