Re: [PATCH v10 23/38] dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP

On Wed, Sep 16, 2026 at 10:28 AM [off-list ref] wrote:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dyndbg: init_bits relies on _DPRINTK_FLAGS_DEFAULT evaluated in wrong translation unit
- [Medium] dyndbg: TOCTOU race in ddebug_sync_classbits() causes out-of-sync debug state
--

commit 3b29729057e8a4b4e0401fd7f3a74205f701c129
Author: Jim Cromie [off-list ref]

dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP

This commit replaces DECLARE_DYNDBG_CLASSMAP with an explicit client-server
model using DYNAMIC_DEBUG_CLASSMAP_DEFINE and DYNAMIC_DEBUG_CLASSMAP_USE.
At module load time, it resolves reference records and automatically applies
the initial bitmap to the module.

Cc: linux-doc@vger.kernel.org

quoted

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 03dc35af4faa1..566c3860d7bfb 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c

[ ... ]

quoted

+static void ddebug_sync_classbits(const struct kernel_param *kp, const char *modname)
+{
+     const struct ddebug_class_param *dcp = kp->arg;
+     u32 val, new_bits, init_bits;
+
+     if (!dcp || !dcp->map)
+             return;
+
+     init_bits = (_DPRINTK_FLAGS_DEFAULT & _DPRINTK_FLAGS_PRINT) ?
+             CLASSMAP_BITMASK(dcp->map->length) : 0;

[Severity: High]
Does this evaluate _DPRINTK_FLAGS_DEFAULT in the context of the dynamic
debug library rather than the target module's translation unit? Since
dynamic_debug.c is not compiled with -DDEBUG, init_bits will always be
evaluated as 0 here. Could this prevent the system from properly disabling
callsites for modules that are loaded and compiled with -DDEBUG?

yes. this code is misleading and has been removed.

what needs to be said ( Im looking for the place to say it once) is:

DEBUG is a local override  (1 file, or even 1 func,line )
to a param, which cannot possibly express its full range of behaviors

when DEBUG is used, it is honored.
In this case, to truly turn off the enabled sites, toggle them on,
then off again.

something like that..

quoted

+
+     switch (dcp->map->map_type) {
+     case DD_CLASS_TYPE_DISJOINT_BITS:
+             val = READ_ONCE(*dcp->bits);
+             new_bits = val;
+             v2pr_info("  %s: classbits: 0x%x\n", KP_NAME(kp), new_bits);
+             ddebug_apply_class_bitmap(dcp, &new_bits, init_bits, modname);
+             break;

[ ... ]

quoted

@@ -1238,12 +1410,17 @@ static int ddebug_add_module(struct _ddebug_info *di)

[ ... ]

quoted

      mutex_lock(&ddebug_lock);
      list_add_tail(&dt->link, &ddebug_tables);
      mutex_unlock(&ddebug_lock);

-     vpr_info("%3u debug prints in module %s\n", di->descs.len, di->mod_name);
+     if (dt->info.users.len)
+             ddebug_apply_class_users(&dt->info);

[Severity: Medium]
Could this introduce a time-of-check to time-of-use (TOCTOU) race condition
between module loading and concurrent sysfs updates?

Once ddebug_add_module() adds the module to ddebug_tables and drops the
ddebug_lock, it proceeds to call ddebug_apply_class_users() (which then
calls ddebug_sync_classbits()).

 In ddebug_add_module(), linking dt to the global ddebug_tables list
  and releasing ddebug_lock before calling ddebug_apply_class_maps() and
  ddebug_apply_class_users() is structurally required.

any changes to be made by apply-class-* only work cuz the module is on
the list scanned by ddebug-change() - which takes the lock, so we cannot.


Publishing dt on ddebug_tables is sufficient for both definers (maps)
  and users (users). The kernel module loader guarantees that definers
  load before users via ELF symbol dependencies on the exported classmap
  struct. Once linked, ddebug_find_valid_class() symmetrically resolves
  class_ids across di->maps and di->users, ddebug_apply_params()
  binds controlling_param and synchronizes initial bits, and the module
  is automatically included in all subsequent runtime sysfs writes and
  ">control" queries without requiring secondary registration.

In ddebug_sync_classbits(), the parameter value is read via
READ_ONCE(*dcp->bits) without holding the lock. If a concurrent sysfs write
updates the parameter and applies it to all modules in ddebug_tables
(including this newly added one), could the module load thread subsequently
resume and apply the stale value it just read, leaving this module's debug
state out-of-sync with the global parameter?

since the effect of a param change is to call ddebug_change() which
takes the lock.
taking it around the param ingestion also sounds like a bad idea.

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-dd-cmap-part2-clean-v10-0-af4cf4767707@gmail.com?part=23