Re: [PATCH v8 41/43] dyndbg: add +c flag to count advantage of classmaps for DRM
From: sashiko-bot@kernel.org
Date: 2026-09-05 18:29:26
Also in:
dri-devel
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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)[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c --- a/lib/dynamic_debug.c +++ b/lib/dynamic_debug.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
/* * 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)
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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