Re: [PATCH can-next v3 1/4] can: proc: reset pkg_stats atomics individually

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] can: proc: stale counter snapshots in can_stat_update() overwrite reset stats
--

commit 947871fd730bb7740551ee6c8b42c7ac5ed61308
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

can: proc: reset pkg_stats atomics individually

This patch replaces the whole-struct memset() in can_init_stats() with
individual atomic_long_set() and scalar field resets in can_reset_pkg_stats(),
and switches can_stat_update() to use atomic_long_xchg() when reading and
clearing the delta counters.

> diff --git a/net/can/proc.c b/net/can/proc.c
> index de4d05ae34597..70104f13077e0 100644
> --- a/net/can/proc.c
> +++ b/net/can/proc.c
> @@ -76,16 +76,39 @@ static const char rx_list_name[][8] = {
>   * af_can statistics stuff
>   */
>  
> +static void can_reset_pkg_stats(struct can_pkg_stats *pkg_stats)
> +{
> +	atomic_long_set(&pkg_stats->rx_frames, 0);
> +	atomic_long_set(&pkg_stats->tx_frames, 0);
> +	atomic_long_set(&pkg_stats->matches, 0);
> +
> +	pkg_stats->total_rx_rate = 0;
> +	pkg_stats->total_tx_rate = 0;
> +	pkg_stats->total_rx_match_ratio = 0;
> +
> +	pkg_stats->current_rx_rate = 0;
> +	pkg_stats->current_tx_rate = 0;
> +	pkg_stats->current_rx_match_ratio = 0;
> +
> +	pkg_stats->max_rx_rate = 0;
> +	pkg_stats->max_tx_rate = 0;
> +	pkg_stats->max_rx_match_ratio = 0;
> +
> +	atomic_long_set(&pkg_stats->rx_frames_delta, 0);
> +	atomic_long_set(&pkg_stats->tx_frames_delta, 0);
> +	atomic_long_set(&pkg_stats->matches_delta, 0);
> +}
> +
>  static void can_init_stats(struct net *net)
>  {
>  	struct can_pkg_stats *pkg_stats = net->can.pkg_stats;
>  	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
>  	/*
> -	 * This memset function is called from a timer context (when
> +	 * This stats reset is called from a timer context (when
>  	 * can_stattimer is active which is the default) OR in a process
>  	 * context (reading the proc_fs when can_stattimer is disabled).
>  	 */
> -	memset(pkg_stats, 0, sizeof(struct can_pkg_stats));
> +	can_reset_pkg_stats(pkg_stats);
>  	pkg_stats->jiffies_init = jiffies;
>  
>  	rcv_lists_stats->stats_reset++;
> @@ -121,9 +144,10 @@ void can_stat_update(struct timer_list *t)
>  	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);
> +	/* fetch and clear values for 'current rate' calculation */
> +	long rx_frames_delta = atomic_long_xchg(&pkg_stats->rx_frames_delta, 0);
> +	long tx_frames_delta = atomic_long_xchg(&pkg_stats->tx_frames_delta, 0);
> +	long matches_delta = atomic_long_xchg(&pkg_stats->matches_delta, 0);
>  
>  	/* restart counting in timer context on user request */
>  	if (user_reset)

[Severity: Medium]
Does can_stat_update() overwrite the freshly reset scalar statistics in
pkg_stats with stale pre-reset counter snapshots after calling
can_init_stats()?

At the beginning of can_stat_update() in net/can/proc.c, the counters are
snapshotted into local variables before checking for a reset or overflow:

	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);
	/* fetch and clear values for 'current rate' calculation */
	long rx_frames_delta = atomic_long_xchg(&pkg_stats->rx_frames_delta, 0);
	long tx_frames_delta = atomic_long_xchg(&pkg_stats->tx_frames_delta, 0);
	long matches_delta = atomic_long_xchg(&pkg_stats->matches_delta, 0);

	/* 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 (rx_frames > (LONG_MAX / HZ))
		can_init_stats(net);
	...

When user_reset is set (for example, after reading /proc/net/can/reset_stats)
or when one of the overflow conditions triggers, can_stat_update() calls
can_init_stats() -> can_reset_pkg_stats(), which zeroes the atomic counters
and the derived scalar fields (total_*, current_*, max_*).

However, the local variables rx_frames, tx_frames, matches, rx_frames_delta,
tx_frames_delta, and matches_delta still hold the pre-reset values when
can_stat_update() continues immediately below:

	/* calc total values */
	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,
					    tx_frames);
	pkg_stats->total_rx_rate = calc_rate(pkg_stats->jiffies_init, j,
					    rx_frames);

	/* calc current values */
	if (rx_frames_delta)
		pkg_stats->current_rx_match_ratio =
			(matches_delta * 100) /	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;
	...

As a result, those pre-reset local variables overwrite total_rx_match_ratio,
current_*, and max_* right after can_reset_pkg_stats() cleared them (and if
the reset was triggered by the rx_frames, tx_frames, or matches overflow
checks, the overflowing values are still passed into the ratio and calc_rate()
calculations).

Additionally, because can_init_stats() sets pkg_stats->jiffies_init = jiffies
by reading live jiffies rather than using the snapshot j, if jiffies ticks
between the snapshot and can_init_stats(net) during a user_reset, the
subsequent check (j < pkg_stats->jiffies_init) will also evaluate to true,
calling can_init_stats(net) a second time and then passing jiffies_init > j
along with the stale non-zero tx_frames and rx_frames into calc_rate().

Should can_stat_update() re-arm the timer and return early (or zero the local
snapshot variables) when can_init_stats() is called?

[ ... ]

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