[PATCH] can: statistics: use atomic access in hot path

[PATCH] can: statistics: use atomic access in hot path

[Oliver Hartkopp]

In can_send() and can_receive() CAN messages and CAN filter matches are
counted to be visible in the CAN procfs files.

KCSAN detected a data race within can_send() when two CAN frames have
been generated by a timer event writing to the same CAN netdevice at the
same time. Use atomic operations to access the statistics in the hot path
to fix the KCSAN complaint.

Reported-by: syzbot+78ce4489b812515d5e4d@syzkaller.appspotmail.com
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/af_can.c | 12 ++++++------
 net/can/af_can.h | 12 ++++++------
 net/can/proc.c   | 46 +++++++++++++++++++++++++++-------------------
 3 files changed, 39 insertions(+), 31 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 01f3fbb3b67d..65230e81fa08 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -285,12 +285,12 @@ int can_send(struct sk_buff *skb, int loop)
 
 	if (newskb)
 		netif_rx(newskb);
 
 	/* update statistics */
-	pkg_stats->tx_frames++;
-	pkg_stats->tx_frames_delta++;
+	atomic_long_inc(&pkg_stats->tx_frames);
+	atomic_long_inc(&pkg_stats->tx_frames_delta);
 
 	return 0;
 
 inval_skb:
 	kfree_skb(skb);
@@ -645,12 +645,12 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
 	struct net *net = dev_net(dev);
 	struct can_pkg_stats *pkg_stats = net->can.pkg_stats;
 	int matches;
 
 	/* update statistics */
-	pkg_stats->rx_frames++;
-	pkg_stats->rx_frames_delta++;
+	atomic_long_inc(&pkg_stats->rx_frames);
+	atomic_long_inc(&pkg_stats->rx_frames_delta);
 
 	/* create non-zero unique skb identifier together with *skb */
 	while (!(can_skb_prv(skb)->skbcnt))
 		can_skb_prv(skb)->skbcnt = atomic_inc_return(&skbcounter);
 
@@ -667,12 +667,12 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
 
 	/* consume the skbuff allocated by the netdevice driver */
 	consume_skb(skb);
 
 	if (matches > 0) {
-		pkg_stats->matches++;
-		pkg_stats->matches_delta++;
+		atomic_long_inc(&pkg_stats->matches);
+		atomic_long_inc(&pkg_stats->matches_delta);
 	}
 }
 
 static int can_rcv(struct sk_buff *skb, struct net_device *dev,
 		   struct packet_type *pt, struct net_device *orig_dev)
diff --git a/net/can/af_can.h b/net/can/af_can.h
index 7c2d9161e224..22f3352c77fe 100644
--- a/net/can/af_can.h
+++ b/net/can/af_can.h
@@ -64,13 +64,13 @@ struct receiver {
 
 /* can be reset e.g. by can_init_stats() */
 struct can_pkg_stats {
 	unsigned long jiffies_init;
 
-	unsigned long rx_frames;
-	unsigned long tx_frames;
-	unsigned long matches;
+	atomic_long_t rx_frames;
+	atomic_long_t tx_frames;
+	atomic_long_t matches;
 
 	unsigned long total_rx_rate;
 	unsigned long total_tx_rate;
 	unsigned long total_rx_match_ratio;
 
@@ -80,13 +80,13 @@ struct can_pkg_stats {
 
 	unsigned long max_rx_rate;
 	unsigned long max_tx_rate;
 	unsigned long max_rx_match_ratio;
 
-	unsigned long rx_frames_delta;
-	unsigned long tx_frames_delta;
-	unsigned long matches_delta;
+	atomic_long_t rx_frames_delta;
+	atomic_long_t tx_frames_delta;
+	atomic_long_t matches_delta;
 };
 
 /* persistent statistics */
 struct can_rcv_lists_stats {
 	unsigned long stats_reset;
diff --git a/net/can/proc.c b/net/can/proc.c
index bbce97825f13..25fdf060e30d 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -116,48 +116,53 @@ void can_stat_update(struct timer_list *t)
 {
 	struct net *net = from_timer(net, t, can.stattimer);
 	struct can_pkg_stats *pkg_stats = net->can.pkg_stats;
 	unsigned long j = jiffies; /* snapshot */
 
+	long rx_frames = atomic_long_read(&pkg_stats->rx_frames);
+	long tx_frames = atomic_long_read(&pkg_stats->tx_frames);
+	long matches = atomic_long_read(&pkg_stats->matches);
+	long rx_frames_delta = atomic_long_read(&pkg_stats->rx_frames_delta);
+	long tx_frames_delta = atomic_long_read(&pkg_stats->tx_frames_delta);
+	long matches_delta = atomic_long_read(&pkg_stats->matches_delta);
+
 	/* restart counting in timer context on user request */
 	if (user_reset)
 		can_init_stats(net);
 
 	/* restart counting on jiffies overflow */
 	if (j < pkg_stats->jiffies_init)
 		can_init_stats(net);
 
 	/* prevent overflow in calc_rate() */
-	if (pkg_stats->rx_frames > (ULONG_MAX / HZ))
+	if (rx_frames > (LONG_MAX / HZ))
 		can_init_stats(net);
 
 	/* prevent overflow in calc_rate() */
-	if (pkg_stats->tx_frames > (ULONG_MAX / HZ))
+	if (tx_frames > (LONG_MAX / HZ))
 		can_init_stats(net);
 
 	/* matches overflow - very improbable */
-	if (pkg_stats->matches > (ULONG_MAX / 100))
+	if (matches > (LONG_MAX / 100))
 		can_init_stats(net);
 
 	/* calc total values */
-	if (pkg_stats->rx_frames)
-		pkg_stats->total_rx_match_ratio = (pkg_stats->matches * 100) /
-			pkg_stats->rx_frames;
+	if (rx_frames)
+		pkg_stats->total_rx_match_ratio = (matches * 100) / rx_frames;
 
 	pkg_stats->total_tx_rate = calc_rate(pkg_stats->jiffies_init, j,
-					    pkg_stats->tx_frames);
+					    tx_frames);
 	pkg_stats->total_rx_rate = calc_rate(pkg_stats->jiffies_init, j,
-					    pkg_stats->rx_frames);
+					    rx_frames);
 
 	/* calc current values */
-	if (pkg_stats->rx_frames_delta)
+	if (rx_frames_delta)
 		pkg_stats->current_rx_match_ratio =
-			(pkg_stats->matches_delta * 100) /
-			pkg_stats->rx_frames_delta;
+			(matches_delta * 100) /	rx_frames_delta;
 
-	pkg_stats->current_tx_rate = calc_rate(0, HZ, pkg_stats->tx_frames_delta);
-	pkg_stats->current_rx_rate = calc_rate(0, HZ, pkg_stats->rx_frames_delta);
+	pkg_stats->current_tx_rate = calc_rate(0, HZ, tx_frames_delta);
+	pkg_stats->current_rx_rate = calc_rate(0, HZ, rx_frames_delta);
 
 	/* check / update maximum values */
 	if (pkg_stats->max_tx_rate < pkg_stats->current_tx_rate)
 		pkg_stats->max_tx_rate = pkg_stats->current_tx_rate;
 
@@ -166,13 +171,13 @@ void can_stat_update(struct timer_list *t)
 
 	if (pkg_stats->max_rx_match_ratio < pkg_stats->current_rx_match_ratio)
 		pkg_stats->max_rx_match_ratio = pkg_stats->current_rx_match_ratio;
 
 	/* clear values for 'current rate' calculation */
-	pkg_stats->tx_frames_delta = 0;
-	pkg_stats->rx_frames_delta = 0;
-	pkg_stats->matches_delta   = 0;
+	atomic_long_set(&pkg_stats->tx_frames_delta, 0);
+	atomic_long_set(&pkg_stats->rx_frames_delta, 0);
+	atomic_long_set(&pkg_stats->matches_delta, 0);
 
 	/* restart timer (one second) */
 	mod_timer(&net->can.stattimer, round_jiffies(jiffies + HZ));
 }
 
@@ -212,13 +217,16 @@ static int can_stats_proc_show(struct seq_file *m, void *v)
 	struct net *net = m->private;
 	struct can_pkg_stats *pkg_stats = net->can.pkg_stats;
 	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
 
 	seq_putc(m, '\n');
-	seq_printf(m, " %8ld transmitted frames (TXF)\n", pkg_stats->tx_frames);
-	seq_printf(m, " %8ld received frames (RXF)\n", pkg_stats->rx_frames);
-	seq_printf(m, " %8ld matched frames (RXMF)\n", pkg_stats->matches);
+	seq_printf(m, " %8ld transmitted frames (TXF)\n",
+		   atomic_long_read(&pkg_stats->tx_frames));
+	seq_printf(m, " %8ld received frames (RXF)\n",
+		   atomic_long_read(&pkg_stats->rx_frames));
+	seq_printf(m, " %8ld matched frames (RXMF)\n",
+		   atomic_long_read(&pkg_stats->matches));
 
 	seq_putc(m, '\n');
 
 	if (net->can.stattimer.function == can_stat_update) {
 		seq_printf(m, " %8ld %% total match ratio (RXMR)\n",
-- 
2.47.2

Re: [PATCH] can: statistics: use atomic access in hot path

[Vincent Mailhol]

On Mon. 10 Mar 2025 à 23:34, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> In can_send() and can_receive() CAN messages and CAN filter matches are
> counted to be visible in the CAN procfs files.
>
> KCSAN detected a data race within can_send() when two CAN frames have
> been generated by a timer event writing to the same CAN netdevice at the
> same time. Use atomic operations to access the statistics in the hot path
> to fix the KCSAN complaint.
>
> Reported-by: syzbot+78ce4489b812515d5e4d@syzkaller.appspotmail.com

Maybe add a Fixes: tag?

> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

Notwithstanding of above remark:

Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>


Yours sincerely,
Vincent Mailhol

Re: [PATCH] can: statistics: use atomic access in hot path

[Oliver Hartkopp]

On 10.03.25 16:04, Vincent Mailhol wrote:
> On Mon. 10 Mar 2025 à 23:34, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> In can_send() and can_receive() CAN messages and CAN filter matches are
>> counted to be visible in the CAN procfs files.
>>
>> KCSAN detected a data race within can_send() when two CAN frames have
>> been generated by a timer event writing to the same CAN netdevice at the
>> same time. Use atomic operations to access the statistics in the hot path
>> to fix the KCSAN complaint.
>>
>> Reported-by: syzbot+78ce4489b812515d5e4d@syzkaller.appspotmail.com
> 
> Maybe add a Fixes: tag?

I'm not sure about if we should provide a Fixes tag here.
The code is from the initial commit in 2008:

https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/net/can/af_can.c?id=0d66548a10cbbe0ef256852d63d30603f0f73f9b

But it never created any problems and it is also not that relevant if we 
have a packet counter which is +/- 1 correct just to be displayed in the 
procfs for users that accidentally know about this feature.

If we add a Fixes tag the patch is backported to many stable kernels.
Not sure if it's worth it.

For future KCSAN tests we are safe then.

Best regards,
Oliver

> 
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> Notwithstanding of above remark:
> 
> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 
> 
> Yours sincerely,
> Vincent Mailhol

Re: [PATCH] can: statistics: use atomic access in hot path

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 878 bytes --]

On 10.03.2025 15:33:53, Oliver Hartkopp wrote:
> In can_send() and can_receive() CAN messages and CAN filter matches are
> counted to be visible in the CAN procfs files.
> 
> KCSAN detected a data race within can_send() when two CAN frames have
> been generated by a timer event writing to the same CAN netdevice at the
> same time. Use atomic operations to access the statistics in the hot path
> to fix the KCSAN complaint.
> 
> Reported-by: syzbot+78ce4489b812515d5e4d@syzkaller.appspotmail.com
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

Applied to linux-can.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]