Re: [PATCH v8 41/43] dyndbg: add +c flag to count advantage of classmaps for DRM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Memory leak of the user-provided buffer `tmpbuf` in `ddebug_proc_write()` when handling the `reset_stats` command.
- [High] The `+c` (call counting) flag fails to operate when `CONFIG_JUMP_LABEL` is disabled, unless the `+p` flag is also set.
- [Medium] Call counts from offlined CPUs are dropped from the total, resulting in inaccurate statistics.
- [Low] Data race when resetting per-CPU call counts from another CPU locklessly.
--

commit 5f4a75e3669c0fca15e2071c6b6911a58fe6a5ab
Author: Jim Cromie <jim.cromie@gmail.com>
Subject: dyndbg: add +c flag to count advantage of classmaps for DRM

This commit introduces a +c flag to dynamic debug to count per-cpu hits
for flagged pr_debug() callsites. The counts can be reset via a
reset_stats command and read from the control file.

> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> --- a/include/linux/dynamic_debug.h
> +++ b/include/linux/dynamic_debug.h
[ ... ]
> @@ -420,11 +430,15 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
>   * (|_cls):	adds in _DPRINT_CLASS_DFLT as needed
>   * (|_no_desc):	former gets callsite descriptor as 1st arg (for prdbgs)
>   */
> +
>  #define __dynamic_func_call_cls(id, cls, fmt, func, ...) do {	\
>  	DEFINE_DYNAMIC_DEBUG_METADATA_CLS(id, cls, fmt);	\
>  	if (DYNAMIC_DEBUG_BRANCH(id)) {				\

[Severity: High]
Will the +c (call counting) flag operate correctly when CONFIG_JUMP_LABEL
is disabled?

If CONFIG_JUMP_LABEL is not set, the fallback DYNAMIC_DEBUG_BRANCH(descriptor)
macros check only for _DPRINTK_FLAGS_PRINT.

When a user enables counting with +c but leaves printing disabled (-p),
the branch evaluates to false, and DYNAMIC_DEBUG_COUNT() is bypassed
completely.

> -		func(&id, ##__VA_ARGS__);			\
> -		__dynamic_dump_stack(id);			\
> +		DYNAMIC_DEBUG_COUNT(id);			\
> +		if (id.flags & _DPRINTK_FLAGS_ACTIVE) {		\
> +			func(&id, ##__VA_ARGS__);		\
> +			__dynamic_dump_stack(id);		\
> +		}						\
>  	}							\
>  } while (0)

[ ... ]
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
[ ... ]
> @@ -1083,6 +1093,14 @@ static __init int dyndbg_setup(char *str)
>  
>  __setup("dyndbg=", dyndbg_setup);
>  
> +static void reset_ddebug_call_count(void)
> +{
> +	int cpu;
> +
> +	for_each_possible_cpu(cpu)
> +		per_cpu(ddebug_call_count, cpu) = 0;
> +}
> +

[Severity: Low]
Can this lockless reset cause a data race?

This logic locklessly writes 0 to the per-CPU variable of all possible
CPUs. If another CPU is concurrently executing this_cpu_inc(), which is
atomic only with respect to local interrupts, a data race occurs.

This can result in a lost reset or a lost increment in the call count
statistics.

>  /*
>   * File_ops->write method for <debugfs>/dynamic_debug/control.  Gathers the
>   * command text from userspace, parses and executes it.
> @@ -1105,6 +1123,10 @@ static ssize_t ddebug_proc_write(struct file *file, const char __user *ubuf,
>  		return PTR_ERR(tmpbuf);
>  	v2pr_info("read %zu bytes from userspace\n", len);
>  
> +	if (len >= 11 && !strncmp(tmpbuf, "reset_stats", 11)) {
> +		reset_ddebug_call_count();
> +		return len;
> +	}

[Severity: High]
Does this code leak tmpbuf?

When handling the reset_stats command, this early return path bypasses
the standard cleanup and skips the kfree(tmpbuf) call required to free the
dynamically allocated buffer.

This allows a privileged user to leak memory (up to PAGE_SIZE per write)
on every successful reset_stats command.

>  	ret = ddebug_exec_queries(tmpbuf, NULL);
>  	kfree(tmpbuf);
>  	if (ret < 0)
[ ... ]
> @@ -1238,6 +1260,16 @@ static const char *ddebug_class_name(struct _ddebug_info *di, struct _ddebug *dp
>  	return NULL;
>  }
>  
> +static unsigned long get_ddebug_call_count(void)
> +{
> +	unsigned long total = 0;
> +	int cpu;
> +
> +	for_each_online_cpu(cpu)
> +		total += per_cpu(ddebug_call_count, cpu);
> +	return total;
> +}

[Severity: Medium]
Is it intentional to sum only the online CPUs here?

The reset logic in reset_ddebug_call_count() expects all possible CPUs
to be managed, but this read logic drops accumulated counts from CPUs
that are currently offline.

If a CPU was online, accumulated hits, and is subsequently offlined, its
counts are excluded from the total, making the metric fluctuate and
become unreliable across CPU hotplug events.

> +
>  /*
>   * Seq_ops show method.  Called several times within a read()
>   * call from userspace, with ddebug_lock held.  Formats the

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905-dd-cmap-part2-clean-v8-0-a4cc0674f6fd@gmail.com?part=41