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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 12.05.26 15:39, 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@vger.kernel.org
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>

Makes perfect sense!
Many thanks!

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

> ---
>   net/can/proc.c | 27 +++++++++++++++++++++++++--
>   1 file changed, 25 insertions(+), 2 deletions(-)
> 
> 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;
> +
> +	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++;