Re: [PATCH v2 2/2] can: bcm: use atomic access in receive statistics

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Converting `frames_filtered` to `atomic_long_t` introduces a signed integer overflow on 32-bit systems during procfs statistics calculation, producing bogus corrupted percentages.

Pre-existing issues:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in `bcm_proc_show()` can cause a division by zero and kernel panic due to multiple lockless reads of `op->frames_abs`.
--

commit 6749d4fcb2595ae8bc232f3dd041a194ebd9a9f4
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: use atomic access in receive statistics

This commit modifies the CAN broadcast manager (bcm) protocol to use atomic
operations for rx statistics. This prevents a data race detected by KCSAN when
multiple CAN frames are processed simultaneously by different CPUs.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index dbdf5d4f4ed7b..8a2cb766603dc 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -228,7 +229,7 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>  		unsigned long reduction;
>  
>  		/* print only active entries & prevent division by zero */
> -		if (!op->frames_abs)
> +		if (!atomic_long_read(&op->frames_abs))
>  			continue;
>  
>  		seq_printf(m, "rx_op: %03X %-5s ", op->can_id,

[ ... ]

> @@ -250,9 +251,12 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>  				   (long long)ktime_to_us(op->kt_ival2));
>  
>  		seq_printf(m, "# recv %ld (%ld) => reduction: ",
> -			   op->frames_filtered, op->frames_abs);
> +			   atomic_long_read(&op->frames_filtered),
> +			   atomic_long_read(&op->frames_abs));
>  
> -		reduction = 100 - (op->frames_filtered * 100) / op->frames_abs;
> +		reduction = 100 -
> +			(atomic_long_read(&op->frames_filtered) * 100) /
> +			atomic_long_read(&op->frames_abs);

[Severity: High]
This isn't a bug introduced by this patch, but does reading op->frames_abs
multiple times introduce a Time-of-Check to Time-of-Use race here?

bcm_proc_show() in net/can/bcm.c first ensures op->frames_abs is not zero.
However, concurrently, bcm_rx_changed() can hit the overflow threshold and
reset it:

net/can/bcm.c:bcm_rx_changed() {
    ...
	if (atomic_long_read(&op->frames_filtered) > ULONG_MAX / 100) {
		atomic_long_set(&op->frames_filtered, 0);
		atomic_long_set(&op->frames_abs, 0);
	}
    ...
}

If this reset occurs precisely between the initial non-zero check and the
division above, could the denominator evaluate to 0 and cause a divide-by-zero
kernel panic?

Would it be safer to read the atomic values exactly once into local variables
at the beginning of the iteration?

[Severity: Medium]
Does changing the statistics variables to atomic_long_t introduce a signed
integer overflow on 32-bit systems?

atomic_long_read() returns a signed long. The expression
atomic_long_read(&op->frames_filtered) * 100 in bcm_proc_show() now evaluates
using signed 32-bit integer arithmetic on 32-bit systems.

While the overflow prevention in bcm_rx_changed() caps frames_filtered at
ULONG_MAX / 100, multiplying by 100 when frames_filtered exceeds LONG_MAX / 100
will overflow the maximum positive value for a signed 32-bit integer.

Dividing this negative intermediate result by frames_abs yields a negative
number, which corrupts the final reduction percentage.

Should the reads be cast to unsigned long before the calculation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522180758.51128-1-socketcan@hartkopp.net?part=2