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 <[email protected]> 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/[email protected]?part=41
