[3/4] net: korina: staging: octeon: Use DEV_STAT_INC()

Commit Message

zhangheng Nov. 9, 2024, 9:20 a.m. UTC

syzbot/KCSAN reported that races happen when multiple CPUs updating
dev->stats.tx_error concurrently. Adopt SMP safe DEV_STATS_INC() to
update the dev->stats fields.

Signed-off-by: zhangheng <[e-mail]>
---
 drivers/net/ethernet/korina.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

Comments

Simon Horman Nov. 11, 2024, 5:36 p.m. UTC | #1

On Sat, Nov 09, 2024 at 05:20:24PM +0800, zhangheng wrote:
> syzbot/KCSAN reported that races happen when multiple CPUs updating
> dev->stats.tx_error concurrently. Adopt SMP safe DEV_STATS_INC() to
> update the dev->stats fields.
> 
> Signed-off-by: zhangheng <[e-mail]>

Hi,

I'm unsure what happened to the other 3 patches of the series
(and the cover letter?), but only this patch seems to be known
to lore.

This path looks like it is a fix for Networking, and thus should
be targeted at the net tree.

	Subject: [PATCH net] ...

And, looking at git history, I think just "net: korina: " would be an
appropriate prefix for this patch. Along with a slightly more descriptive
subject. Maybe:

	Subject: [PATCH net] net: korina: Use DEV_STAT_INC() to avoid race

Next, is there a stack trace available? If so, it would be nice
to include it, succinctly, in the patch description.

And if there there is a public syzbot/KCSAN report it would be nice
to include a link to it.

As for the changes, you mention that the patch fixes a race wrt to
tx_error. But the patch does more than that. Are the other changes also
strictly necessary? If so, I think that should be mentioned in the patch
description. And I'd suggest that any changes that are not strictly
necessary as a bug-fix should be handled via a follow-up patch targeted
at net-next.

As a fix there should be a Fixes tag immediately above your Signed-off-by
line (no blank line in between). It should cite the commit where the bug
was introduced, typically the first commit where it manifests for users.

Lastly, information on the process for Networking patches can be
found here: https://docs.kernel.org/process/maintainer-netdev.html

...

Patch

diff --git a/drivers/net/ethernet/korina.c b/drivers/net/ethernet/korina.c
index 81cf3361a1e5..f08f6f115ce6 100644
--- a/drivers/net/ethernet/korina.c
+++ b/drivers/net/ethernet/korina.c
@@ -514,7 +514,7 @@  static netdev_tx_t korina_send_packet(struct sk_buff *skb,
 	return NETDEV_TX_OK;
 
 drop_packet:
-	dev->stats.tx_dropped++;
+	DEV_STATS_INC(dev, tx_dropped);
 	dev_kfree_skb_any(skb);
 	spin_unlock_irqrestore(&lp->lock, flags);
 
@@ -619,8 +619,8 @@  static int korina_rx(struct net_device *dev, int limit)
 
 		if (!(devcs & ETH_RX_ROK)) {
 			/* Update statistics counters */
-			dev->stats.rx_errors++;
-			dev->stats.rx_dropped++;
+			DEV_STATS_INC(dev, rx_errors);
+			DEV_STATS_INC(dev, rx_dropped);
 			if (devcs & ETH_RX_CRC)
 				dev->stats.rx_crc_errors++;
 			if (devcs & ETH_RX_LE)
@@ -657,12 +657,12 @@  static int korina_rx(struct net_device *dev, int limit)
 
 		/* Pass the packet to upper layers */
 		napi_gro_receive(&lp->napi, skb);
-		dev->stats.rx_packets++;
-		dev->stats.rx_bytes += pkt_len;
+		DEV_STATS_INC(dev, rx_packets);
+		DEV_STATS_ADD(dev, tx_bytes, pkt_len);
 
 		/* Update the mcast stats */
 		if (devcs & ETH_RX_MP)
-			dev->stats.multicast++;
+			DEV_STATS_INC(dev, multicast);
 
 		lp->rx_skb[lp->rx_next_done] = skb_new;
 		lp->rx_skb_dma[lp->rx_next_done] = ca;
@@ -780,39 +780,38 @@  static void korina_tx(struct net_device *dev)
 		devcs = lp->td_ring[lp->tx_next_done].devcs;
 		if ((devcs & (ETH_TX_FD | ETH_TX_LD)) !=
 				(ETH_TX_FD | ETH_TX_LD)) {
-			dev->stats.tx_errors++;
-			dev->stats.tx_dropped++;
+			DEV_STATS_INC(dev, tx_errors);
+			DEV_STATS_INC(dev, tx_dropped);
 
 			/* Should never happen */
 			printk(KERN_ERR "%s: split tx ignored\n",
 							dev->name);
 		} else if (devcs & ETH_TX_TOK) {
-			dev->stats.tx_packets++;
-			dev->stats.tx_bytes +=
-					lp->tx_skb[lp->tx_next_done]->len;
+			DEV_STATS_INC(dev, tx_packets);
+			DEV_STATS_ADD(dev, tx_bytes, lp->tx_skb[lp->tx_next_done]->len);
 		} else {
-			dev->stats.tx_errors++;
-			dev->stats.tx_dropped++;
+			DEV_STATS_INC(dev, tx_errors);
+			DEV_STATS_INC(dev, tx_dropped);
 
 			/* Underflow */
 			if (devcs & ETH_TX_UND)
-				dev->stats.tx_fifo_errors++;
+				DEV_STATS_INC(dev, tx_fifo_errors);
 
 			/* Oversized frame */
 			if (devcs & ETH_TX_OF)
-				dev->stats.tx_aborted_errors++;
+				DEV_STATS_INC(dev, tx_aborted_errors);
 
 			/* Excessive deferrals */
 			if (devcs & ETH_TX_ED)
-				dev->stats.tx_carrier_errors++;
+				DEV_STATS_INC(dev, tx_carrier_errors);
 
 			/* Collisions: medium busy */
 			if (devcs & ETH_TX_EC)
-				dev->stats.collisions++;
+				DEV_STATS_INC(dev, collisions);
 
 			/* Late collision */
 			if (devcs & ETH_TX_LC)
-				dev->stats.tx_window_errors++;
+				DEV_STATS_INC(dev, tx_window_errors);
 		}
 
 		/* We must always free the original skb */