Re: [PATCH net] can: proc: reset pkg_stats atomics individually

[Marc Kleine-Budde]

On 2026-05-12 21:39 +0800, Runyu Xiao wrote:
> Commit 80b5f90158d1 ("can: statistics: use atomic access in hot path")
> converted several members of struct can_pkg_stats to atomic_long_t and
> updated the hot TX/RX and procfs read paths to use atomic_long_*()
> helpers. However, can_init_stats() still clears the whole struct with
> memset().
>
> can_init_stats() is reached from can_stat_update() in timer context and
> also from the procfs reset path. Those paths can run while the TX/RX hot
> paths are concurrently updating rx_frames, tx_frames, matches and their
> *_delta counters. Hitting the whole-struct memset() in that window
> performs plain writes to fields that otherwise follow an atomic_long_t
> access contract, which can lose or mix live statistics updates.
>
> This issue was found by source-level API-misuse analysis looking for
> whole-object resets left behind after atomic_long_t conversions, then
> manually audited on Linux v6.18.21.
>
> Replace the whole-struct memset() with a helper that resets the
> atomic_long_t counters via atomic_long_set() and clears the derived
> scalar statistics explicitly. This preserves the existing reset
> semantics for scalar fields while restoring atomic access discipline for
> the live counters.
>
> Build-tested by compiling net/can/proc.o on x86_64 netdev/main.
> Runtime-tested with a QEMU + vcan setup on Linux v6.18.21 by driving
> concurrent traffic and reset_stats reads, which reproduced inconsistent
> exported statistics before the fix.
>
> Fixes: 80b5f90158d1 ("can: statistics: use atomic access in hot path")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Runyu Xiao <runyu.xiao@xxxxxxxxxx>

The netdev bot has found some issues, can you take care of them?

regards,
Marc

--- Patch [4]: [PATCH net-next 04/37] can: proc: reset pkg_stats atomics individually ---
commit efa392826f228e08df66ba3c5bf707d70092f478
Author: Runyu Xiao <runyu.xiao@xxxxxxxxxx>

can: proc: reset pkg_stats atomics individually

This patch replaces the whole-struct memset() in can_init_stats() with a
new helper, can_reset_pkg_stats(). The helper resets the atomic_long_t
counters of struct can_pkg_stats with atomic_long_set() and clears the
scalar derived statistics with plain stores.

Link: https://patch.msgid.link/20260512133937.21957-1-runyu.xiao@xxxxxxxxxx

> diff --git a/net/can/proc.c b/net/can/proc.c
> index de4d05ae3459..64b3bdc2fa7e 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;

Discovered by: gpt-6-astra, opus-5-5, sonnet-5 · Missed by: sashiko-gemini · Called a false positive by: gpt-6-astra

This isn't a bug introduced by this patch, but these scalar fields are
still written with plain stores from timer softirq context. The writers
are can_reset_pkg_stats(), can_init_stats() (for jiffies_init) and
can_stat_update().

Process context reads the same fields with plain loads, in
can_stats_proc_show():

	seq_printf(m, " %8ld %% total match ratio (RXMR)\n",
			pkg_stats->total_rx_match_ratio);

and in can_reset_stats_proc_show():

	if (pkg_stats->jiffies_init != jiffies)

Without READ_ONCE()/WRITE_ONCE() or data_race(), will KCSAN keep
reporting data races on struct can_pkg_stats? The commit message limits
its goal to "the live counters", so this is only an observation.

> +
> +	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);
> +}

Discovered by: sashiko-gemini, opus-5-5 · Missed by: sonnet-5, gpt-6-astra · Confirmed by: opus-5-5 · Called a false positive by: gpt-6-astra
This is a pre-existing issue. The commit message says plain writes in
the reset path "can lose or mix live statistics updates", but the same
kind of loss remains in the regular timer path after this patch.

can_stat_update() runs once per second from the stats timer. It
snapshots the delta counters at the top of the function, for example:

	long rx_frames_delta = atomic_long_read(&pkg_stats->rx_frames_delta);

It then derives current_tx_rate, current_rx_rate and
current_rx_match_ratio from those locals. Only at the very end does it
clear the counters:

	atomic_long_set(&pkg_stats->rx_frames_delta, 0);

While this runs, can_send() and can_receive() keep calling
atomic_long_inc() on tx_frames_delta, rx_frames_delta and matches_delta
from other CPUs. An increment that lands between the read and the set
is lost. It is not in the snapshot used for this interval, and it is
wiped before the next interval starts. Under load, the current_* rates
and the current match ratio therefore undercount. The rx_frames,
tx_frames and matches totals still count the frame.

Switching can_reset_pkg_stats() from memset() to per-field
atomic_long_set() avoids plain stores to the atomics. It does not turn
the read and the clear in can_stat_update() into a single atomic step,
so this window is unchanged by the patch.

Would taking each snapshot with atomic_long_xchg() against 0 at the
top of can_stat_update(), and dropping the trailing atomic_long_set()
calls, close the window? Should that be folded into this fix, or sent
as a separate patch?

Discovered by: gpt-6-astra, opus-5-5 · Missed by: sonnet-5, sashiko-gemini · Called a false positive by: gpt-6-astra

This is a pre-existing issue, but can_stat_update() reads the *_delta
counters at the top and clears them at the bottom, after the
calculations and any can_init_stats() calls:

net/can/proc.c:can_stat_update() {
	...
	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);
	...
	/* clear values for 'current rate' calculation */
	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);
	...
}

Meanwhile can_send() and can_receive() call atomic_long_inc() on these
deltas from other CPUs, for example:

	atomic_long_inc(&pkg_stats->tx_frames_delta);

Any increment between the snapshot and the clear is overwritten. It
never shows up in current_tx_rate, current_rx_rate or
current_rx_match_ratio, though the totals still count it.

Would taking each snapshot with atomic_long_xchg(&delta, 0) close that
window?

> +
>  static void can_init_stats(struct net *net)
>  {

[ ... ]

> -	memset(pkg_stats, 0, sizeof(struct can_pkg_stats));
> +	can_reset_pkg_stats(pkg_stats);

Discovered by: gpt-6-astra, opus-5-5 · Missed by: sonnet-5, sashiko-gemini · Called a false positive by: gpt-6-astra

This is a pre-existing issue, but this patch has Fixes: 80b5f90158d1.
Should the reset in can_stat_update() be handled as well? That commit
also changed can_stat_update() to snapshot the counters into locals
before any of the can_init_stats() calls:

net/can/proc.c:can_stat_update() {
	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);
	...
	/* restart counting in timer context on user request */
	if (user_reset)
		can_init_stats(net);
	...
	/* matches overflow - very improbable */
	if (matches > (LONG_MAX / 100))
		can_init_stats(net);

	/* calc total values */
	if (rx_frames)
		pkg_stats->total_rx_match_ratio = (matches * 100) / rx_frames;
	...
}

After can_init_stats() returns, the pre-reset locals are still used for
total_rx_match_ratio, current_rx_match_ratio, current_tx_rate,
current_rx_rate and the max_* fields. So the values that
can_reset_pkg_stats() just cleared get refilled from pre-reset data.

With the stats timer enabled (the default), can_reset_stats_proc_show()
only sets user_reset = 1, so every user requested reset takes this path.
If no more traffic arrives, total_rx_match_ratio and
current_rx_match_ratio keep their pre-reset values indefinitely. They
are only recomputed under if (rx_frames) and if (rx_frames_delta).

The matches > (LONG_MAX / 100) guard is also defeated, because
(matches * 100) is still computed from the stale copy after the reset.
On 32-bit with HZ <= 100, rx_frames and matches can both pass
LONG_MAX / 100 in the same tick. Can this multiplication then overflow a
signed long and export a garbage ratio?

Before 80b5f90158d1, these expressions read pkg_stats->... after
can_init_stats() and so saw zeros. Given this, the commit message
statement "This preserves the existing reset semantics for scalar
fields" does not hold for the default timer-driven path.

>  	pkg_stats->jiffies_init = jiffies;
>
>  	rcv_lists_stats->stats_reset++;

Discovered by: gpt-6-astra, opus-5-5, sonnet-5 · Missed by: sashiko-gemini · Called a false positive by: gpt-6-astra

This isn't a bug introduced by this patch, but user_reset is a single
file-scope flag shared by every network namespace and accessed with
plain loads and stores:

static int user_reset;

can_pernet_init() arms a separate can_stat_update() timer for each
netns. A read of /proc/net/can/reset_stats in netns A sets the global
flag. Whichever netns timer runs next resets its own stats and clears
the flag here:

	if (user_reset) {
		user_reset = 0;
		rcv_lists_stats->user_reset++;
	}

reset_stats is created with mode 0644. Can an unprivileged user in one
netns therefore reset another namespace's statistics, including
init_net's, while the reset requested for netns A never happens?

Separately, with the stats timer disabled, two concurrent readers of
reset_stats can both pass the check in can_reset_stats_proc_show():

	if (pkg_stats->jiffies_init != jiffies)
		can_init_stats(net);

Could they then race on the plain stats_reset++ and user_reset++
increments and on the user_reset test-and-clear?

pw-bot: cr

-- 
Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>