Re: [PATCH v9 38/39] dyndbg: add +c flag to count advantage of classmaps for DRM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `+c` flag unintentionally triggers heavy logging and side-effects, and fails to increment the call counter for open-coded `DYNAMIC_DEBUG_BRANCH` callers.
- [Medium] The `reset_stats` command parsing in `ddebug_proc_write` silently drops batched queries, ignores leading whitespace, and falsely matches partial commands.
--

commit 02710e8ae9ac8d54e910e1db01d0bb84fea61ee7
Author: Jim Cromie <jim.cromie@gmail.com>

dyndbg: add +c flag to count advantage of classmaps for DRM

This commit introduces a +c flag to dynamic debug, allowing users to
increment a per-cpu counter when a flagged pr_debug() is called. It also
adds a reset_stats command to /proc/dynamic_debug/control.

> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> index 39de59c8bac2c..064df695b3a4c 100644
> --- a/include/linux/dynamic_debug.h
> +++ b/include/linux/dynamic_debug.h

[ ... ]

> @@ -403,14 +407,20 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
>  
>  #ifdef DEBUG
>  #define DYNAMIC_DEBUG_BRANCH(descriptor) \
> -	likely(descriptor.flags & _DPRINTK_FLAGS_PRINT)
> +	likely(descriptor.flags & _DPRINTK_FLAGS_ENABLED)
>  #else
>  #define DYNAMIC_DEBUG_BRANCH(descriptor) \
> -	unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT)
> +	unlikely(descriptor.flags & _DPRINTK_FLAGS_ENABLED)
>  #endif
>  
>  #endif /* CONFIG_JUMP_LABEL */

[Severity: High]
Does changing this macro to check for _DPRINTK_FLAGS_ENABLED (which includes
the new +c count flag) break open-coded callers of DYNAMIC_DEBUG_BRANCH?

For instance, in include/linux/printk.h, pr_debug_ratelimited() uses this:

	if (DYNAMIC_DEBUG_BRANCH(descriptor) &&
	    __ratelimit(&_rs))
		__dynamic_pr_debug(&descriptor, pr_fmt(fmt), ##__VA_ARGS__);

If a user enables only the +c flag, DYNAMIC_DEBUG_BRANCH evaluates to true,
but because it bypasses the DYNAMIC_DEBUG_COUNT() macro, it fails to
increment the counter. Furthermore, it falls through to __dynamic_pr_debug()
causing it to print anyway.

Other callers like alloc_contig_dump_pages() in mm/page_alloc.c or
sdhci_dbg_dumpregs() in drivers/mmc/host/sdhci.h will similarly execute full
stack and register dumps unintentionally if the +c flag is set, without
counting the calls.

[ ... ]

> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 77976e7b264ca..9fa1a6e3afe6c 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c

[ ... ]

> @@ -1104,6 +1122,11 @@ 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();
> +		kfree(tmpbuf);
> +		return len;
> +	}
>  	ret = ddebug_exec_queries(tmpbuf, NULL);
>  	kfree(tmpbuf);
>  	if (ret < 0)

[Severity: Medium]
Could this early return logic silently drop batched queries?

Since ddebug_exec_queries() handles parsing multiple commands separated by
newlines or semicolons, returning early here means a batched input like
"reset_stats\nfile foo.c +c" would reset the stats but completely ignore the
subsequent commands without returning an error.

Additionally, does this strncmp() check falsely match partial commands like
"reset_stats_and_more", while also failing to handle leading whitespace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-dd-cmap-part2-clean-v9-0-3a3f621f4a10@gmail.com?part=38