Re: [PATCH v8 36/43] dyndbg: resolve "protection" of class'd pr_debug
From: sashiko-bot@kernel.org
Date: 2026-09-05 18:33:16
Also in:
dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The 'controlling_param' field in 'struct ddebug_class_map' is never assigned and its initialization path is disabled, completely breaking the intended protection mechanism for class'd pr_debugs. -- commit 6f4d594fc681c54264d43b5caa68a2316a8b157d Author: Jim Cromie [off-list ref] dyndbg: resolve "protection" of class'd pr_debug This commit aims to protect class'd pr_debugs from unintended changes by unclassed legacy queries (like `echo -p > /proc/dynamic_debug/control`). It introduces a link from `ddebug_class_map` to `controlling_param` and intends to check it to decide whether to protect the class map.
quoted hunk ↗ jump to hunk
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c index 93a5a481c8b8c..a5d813ad323a1 100644 --- a/lib/dynamic_debug.c +++ b/lib/dynamic_debug.c
[ ... ]
+static inline bool ddebug_class_has_param(const struct ddebug_class_map *map)
+{
+ return !!(map->controlling_param);
+}
+
+/* re-framed as a policy choice */
+#define ddebug_class_wants_protection(map) (ddebug_class_has_param(map))
+
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching[ ... ]
quoted hunk ↗ jump to hunk
@@ -256,7 +299,28 @@ static bool ddebug_match_desc(const struct ddebug_query *query, dp->lineno > query->last_lineno) return false; - return true; + /* + * above are all satisfied, so we can make final decisions: + * 1- class FOO or implied class __DEFAULT__ + * 2- site.is_classed or not + */ + if (query->class_string) { + /* class FOO given, exact match required */ + return (dp->class_id == selected_class); + } + /* query class __DEFAULT__ by omission. */ + if (dp->class_id == _DPRINTK_CLASS_DFLT) { + /* un-classed site */ + return true; + } + /* site is class'd */ + site_map = ddebug_find_map_by_class_id(di, dp->class_id); + if (!site_map) { + WARN_ONCE(1, "unknown class_id %d, check %s's CLASSMAP definitions", dp->class_id, di->mod_name); + return false; + } + /* module(-param) decides protection */ + return !ddebug_class_wants_protection(site_map); }
[Severity: Medium] Will `ddebug_class_wants_protection()` always evaluate to false? Because `map->controlling_param` appears to never be initialized, it seems this will always return false, which causes the function to return true and bypass the intended protection for class'd pr_debugs against unclassed legacy queries. [ ... ]
quoted hunk ↗ jump to hunk
@@ -1298,16 +1359,25 @@ static void ddebug_sync_classbits(const struct kernel_param *kp, const char *mod } } -static void ddebug_match_apply_kparam(const struct kernel_param *kp, - const struct ddebug_class_map *map, - const char *mod_name) +static struct ddebug_class_param * +ddebug_get_classmap_kparam(const struct kernel_param *kp, + const struct ddebug_class_map *map) { struct ddebug_class_param *dcp; if (kp->ops != ¶m_ops_dyndbg_classes) - return; + return NULL; dcp = (struct ddebug_class_param *)kp->arg; + return (map == dcp->map) + ? dcp : (struct ddebug_class_param *)NULL; +} + +static void ddebug_match_apply_kparam(const struct kernel_param *kp, + struct ddebug_class_map *map, + const char *mod_name) +{ + struct ddebug_class_param *dcp = ddebug_get_classmap_kparam(kp, map); if (dcp && dcp->map == map) { v2pr_info(" kp:%s.%s =0x%x", mod_name, kp->name, *dcp->bits); vpr_cm_info(map, " %s maps ", mod_name); ddebug_sync_classbits(kp, mod_name); } }
[Severity: Medium] Was the assignment `map->controlling_param = dcp;` intended to be added here? The commit message states that the patch "sets it in ddebug_match_apply_kparam()", but the assignment seems to be missing. Adding the assignment locklessly could also introduce a data race here. [ ... ]
quoted hunk ↗ jump to hunk
@@ -1339,6 +1409,26 @@ static void ddebug_apply_params(const struct ddebug_class_map *cm, const char *m } } +#if 0 +/* + * called from add_module, ie early. it can find controlling kparams, + * which can/does? enable protection of this classmap from class-less + * queries, on the grounds that the user created the kparam, means to + * use it, and expects it to reflect reality. We should oblige him, + * and protect those classmaps from classless "-p" changes. + */ +static void ddebug_apply_class_maps(const struct _ddebug_info *di) +{ + struct ddebug_class_map *cm; + int i; + + for_subvec(i, cm, di, maps) + ddebug_apply_params(cm, cm->mod_name); + + v2pr_di_info(di, "attached %d class-maps to ", i); +} +#endif + static void ddebug_apply_class_users(const struct _ddebug_info *di) {
[Severity: Medium] Does the `#if 0` block around `ddebug_apply_class_maps()` prevent `ddebug_match_apply_kparam()` from ever being called for the defining module? If this function is disabled, the parameter initialization appears unreachable for the defining module's map, even if the assignment was added to `ddebug_match_apply_kparam()`. -- Sashiko AI review · https://sashiko.dev/#/patchset/20260905-dd-cmap-part2-clean-v8-0-a4cc0674f6fd@gmail.com?part=36