Re: [PATCH net-next 05/12] 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: pre-reset local snapshots overwrite zeroed stats in can_stat_update()
--

commit 569f80e5a195e52b5ac88d8f5cdb36714d69c5db
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
can_reset_pkg_stats() to reset atomic_long_t counters via atomic_long_set()
and scalar fields individually. It also uses atomic_long_xchg() in
can_stat_update() to fetch and clear the *_delta counters atomically.

Link: https://patch.msgid.link/20261016110905.422684-1-socketcan@hartkopp.net

> 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]
In net/can/proc.c:can_stat_update(), the counters and deltas are read into
local variables before the reset checks:

	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 a user reads /proc/net/can/reset_stats (setting user_reset = 1) or an
overflow condition triggers, can_init_stats(net) -> can_reset_pkg_stats()
zeroes all the counters and scalar fields in pkg_stats, and updates
pkg_stats->jiffies_init = jiffies.

However, because can_stat_update() does not return early or clear those
local snapshot variables after calling can_init_stats(net), it proceeds to
recompute the statistics using the stale pre-reset values:

	/* 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;
	...

Doesn't this immediately overwrite pkg_stats->total_rx_match_ratio,
current_rx_match_ratio, current_tx_rate, current_rx_rate, max_tx_rate,
max_rx_rate, and max_rx_match_ratio with pre-reset values right after
can_reset_pkg_stats() cleared them (with max_* and match ratios persisting
across subsequent timer ticks)?

Additionally, if jiffies advances between the initial snapshot of j at the
start of can_stat_update() and pkg_stats->jiffies_init = jiffies in
can_init_stats(), the subsequent check (j < pkg_stats->jiffies_init) will
evaluate to true, calling can_init_stats(net) a second time and passing
oldjif > newjif into calc_rate(). And if an overflow check on rx_frames,
tx_frames, or matches triggered the reset, the rate and ratio calculations
still run with the overflowing local values.

Should can_stat_update() re-arm the timer and return early when a reset
occurs, or perform the reset checks before snapshotting the counters?

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018164417.1540449-1-mkl@pengutronix.de?part=5