Re: [PATCH v13 35/39] dyndbg: add +c flag to count pr_debug calls without printing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dyndbg: race between reset_ddebug_call_count() and ddebug_increment_call_count()
--

commit 9bd6a53ec548939d0c4d27704bc80cf4e68b28ef
Author: Jim Cromie <jim.cromie@gmail.com>

dyndbg: add +c flag to count pr_debug calls without printing

This commit adds a +c flag to dynamic debug to count enabled pr_debug()
invocations via a per-CPU counter without printing messages to the syslog.
It also supports resetting the counter via the reset_stats command and
reports the total in the /proc/dynamic_debug/control epilogue.

> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index efc7a741008ed..027f7c31fe226 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c

[ ... ]

> @@ -71,6 +73,21 @@ struct flag_settings {
>  	unsigned int mask;
>  };
>  
> +static DEFINE_PER_CPU(unsigned long, ddebug_call_count);
> +void ddebug_increment_call_count(void)
> +{
> +	this_cpu_inc(ddebug_call_count);
> +}
> +EXPORT_SYMBOL(ddebug_increment_call_count);
> +
> +static void reset_ddebug_call_count(void)
> +{
> +	int cpu;
> +
> +	for_each_possible_cpu(cpu)
> +		per_cpu(ddebug_call_count, cpu) = 0;
> +}

[Severity: Medium]
Can reset_ddebug_call_count() race with concurrent calls to
ddebug_increment_call_count() on remote CPUs and lose the reset?

When userspace writes reset_stats to /proc/dynamic_debug/control,
ddebug_proc_write() -> ddebug_exec_queries() -> reset_ddebug_call_count()
directly stores 0 into each CPU's ddebug_call_count from the resetting
CPU. Meanwhile, other CPUs executing +c callsites invoke
DYNAMIC_DEBUG_BRANCH() -> DYNAMIC_DEBUG_COUNT() ->
ddebug_increment_call_count(), which uses this_cpu_inc(ddebug_call_count).

Because this_cpu_inc() only guarantees atomicity with respect to the local
CPU rather than cross-CPU stores, a remote CPU performing a read-modify-write
can load its old counter value N just before reset_ddebug_call_count() writes
0, and then store N + 1 afterward. That causes a data race and leaves the
remote CPU's entire previous count intact instead of resetting it to zero.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-dd-cmap-part2-clean-v13-0-9267923be612@gmail.com?part=35