!-------------------------------------------------------------------|
This Message Is From an External Sender
This message came from outside your organization.
|-------------------------------------------------------------------!
classmap-v1 code protected class'd pr_debugs from unintended
changes by unclassed/_DFLT queries:
# - to declutter examples:
alias ddcmd='echo $* > /proc/dynamic_debug/control'
# IOW, this should NOT alter drm.debug settings
ddcmd -p
# Instead, you must name the class to change it.
# Protective but tedious
ddcmd class DRM_UT_CORE +p
# Or do it the (old school) subsystem way
# This is ABI !!
echo 1 > /sys/module/drm/parameters/debug
Since the debug sysfs-node is ABI, if dyndbg is going to implement it,
it must also honor its settings; it must at least protect against
accidental changes to its classes from legacy queries.
The protection allows all previously conceived queries to work the way
they always have; ie select the same set of pr_debugs, despite the
inclusion of whole new classes of pr_debugs.
But that choice has 2 downsides:
1. "name the class to change it" makes a tedious long-winded
interface, needing many commands to set DRM_UT_* one at a time.
2. It makes the class keyword special in some sense; the other
keywords skip only on query mismatch, otherwise the code falls thru to
adjust the pr-debug site.
Jason Baron didn't like v1 on point 2.
Louis Chauvet didn't like recent rev on point 1 tedium.
But that said: /sys/ is ABI, so this must be reliable:
#> echo 0x1f > /sys/module/drm/parameters/debug
It 'just works' without dyndbg underneath; we must deliver that same
stability. Convenience is secondary.
The new resolution:
If ABI is the blocking issue, then no ABI means no blocking issue.
IOW, if the classmap has no presence under /sys/*, ie no PARAM, there
is no ABI to guard, and no reason to enforce a tedious interface.
In the future, if DRM wants to alter this protection, that is
practical, but I think default-on is the correct mode.
So atm classes without a PARAM are unprotected at >control, allowing
admins their shortcuts. I think this could satisfy all viewpoints.
That said, theres also a possibility of wildcard classes:
#> ddcmd class '*' +p
Currently, the query-class is exact-matched against each module's
classmaps.names. This gives precise behavior, a good basis.
But class wildcards are possible, they just did'nt appear useful for
DRM, whose classmap names are a flat DRM_UT_* namespace.
IOW, theres no useful selectivity there:
#> ddcmd class "DRM_*" +p # these enable every DRM_* class
#> ddcmd class "DRM_UT_*" +p
#> ddcmd class "DRM_UT_V*" +p # finally select just 1: DRM_UT_VBL
#> ddcmd class "DRM_UT_D*" +p # but this gets 3
#> ddcmd class "D*V*" +p # here be dragons
But there is debatable utility in the feature.
#> ddcmd class __DEFAULT__ -p # what about this ?
#> ddcmd -p # thats what this does.
automatically
Anyway, this patch does:
1. adds link field from _ddebug_class_map to the .controlling_param
2. sets it in ddebug_match_apply_kparam(), during modprobe/init,
when options like drm.debug=VAL are handled.
3. ddebug_class_has_param() now checks .controlling_param
4. ddebug_class_wants_protection() macro renames 3.
this frames it as a separable policy decision
5. ddebug_match_desc() gets the most attention:
a. move classmap consideration to the bottom
this insures all other constraints act 1st.
allows simpler 'final' decisions.
b. split class choices cleanly on query:
class FOO vs none, and class'd vs _DPRINTK_CLASS_DFLT site.
c. calls 4 when applying a class-less query to a class'd pr_debug
here we need a new fn to find the classmap with this .class_id
d. calls new ddebug_find_classmap_by_class_id().
when class-less query looks at a class'd pr_debug.
finds classmap, which can then decide, currently by PARAM existence.
NOTES:
protection is only against class-less queries, explicit "class FOO"
adjustments are allowed (that is the mechanism).
The drm.debug sysfs-node heavily under-specifies the class'd pr_debugs
it controls; none of the +mfls prefixing flags have any effect, and
each callsite remains individually controllable. drm.debug just
toggles the +p flag for all the modules' class'd pr_debugs.
Signed-off-by: Jim Cromie <[email protected]>
---
history
-v0 - original, before classmaps: no special case keywords
-v1 - "class DEFAULT" is assumed if not mentioned.
this protects classes from class-less queries
-v2.pre-this-patch - protection macro'd to false
-v2.with-this-patch - sysfs knob decides
-v2.speculative - module decides wrt classmap protection
seems unneeded now, TBD
v3 - new patch
v4
- drop fn-scope map var, with 2 local vars, renamed to purpose
- fix for NULL ptr case.
- Add loop-var to reduce many "&dt->info." exprs to "di->"
- add 1-liner postcondition comments
fixus
---
include/linux/dynamic_debug.h | 14 ++--
lib/dynamic_debug.c | 127 +++++++++++++++++++++++++++-------
2 files changed, 110 insertions(+), 31 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index b1d11d946780..b22da40e2583 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -75,6 +75,7 @@ enum ddebug_class_map_type {
* map @class_names 0..N to consecutive constants starting at @base.
*/
struct _ddebug_class_map {
+ struct _ddebug_class_param *controlling_param;
const struct module *mod; /* NULL for builtins */
const char *mod_name;
const char **class_names;
@@ -259,7 +260,12 @@ struct _ddebug_class_param {
*
* Creates a sysfs-param to control the classes defined by the
* exported classmap, with bits 0..N-1 mapped to the classes named.
- * This version keeps class-state in a private long int.
+ *
+ * Since sysfs-params are ABI, this also protects the classmap'd
+ * pr_debugs from un-class'd `echo -p > /proc/dynamic_debug/control`
+ * changes.
+ *
+ * This keeps class-state in a private long int.
*/
#define DYNAMIC_DEBUG_CLASSMAP_PARAM(_name, _var, _flags) \
static unsigned long _name##_bvec; \
@@ -272,10 +278,8 @@ struct _ddebug_class_param {
* @_var: name of the (exported) classmap var defining the classes/bits
* @_flags: flags to be toggled, typically just 'p'
*
- * Creates a sysfs-param to control the classes defined by the
- * exported clasmap, with bits 0..N-1 mapped to the classes named.
- * This version keeps class-state in user @_bits. This lets drm check
- * __drm_debug elsewhere too.
+ * Like DYNAMIC_DEBUG_CLASSMAP_PARAM, but maintains param-state in
+ * extern @_bits. This lets DRM check __drm_debug elsewhere too.
*/
#define DYNAMIC_DEBUG_CLASSMAP_PARAM_REF(_name, _bits, _var, _flags) \
__DYNAMIC_DEBUG_CLASSMAP_PARAM(_name, _bits, _var, _flags)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 636a6b5741f7..1082e0273f0e 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -206,6 +206,50 @@ ddebug_find_valid_class(struct _ddebug_info const *di,
const char *query_class,
return NULL;
}
+static bool ddebug_class_in_range(const int class_id, const struct _ddebug_class_map *map)
+{
+ return (class_id >= map->base &&
+ class_id < map->base + map->length);
+}
+
+static struct _ddebug_class_map *
+ddebug_find_map_by_class_id(struct _ddebug_info *di, int class_id)
+{
+ struct _ddebug_class_map *map;
+ struct _ddebug_class_user *cli;
+ int i;
+
+ for_subvec(i, map, di, maps)
+ if (ddebug_class_in_range(class_id, map))
+ return map;
+
+ for_subvec(i, cli, di, users)
+ if (ddebug_class_in_range(class_id, cli->map))
+ return cli->map;
+
+ return NULL;
+}
+
+/*
+ * classmaps-V1 protected classes from changes by legacy commands
+ * (those selecting _DPRINTK_CLASS_DFLT by omission). This had the
+ * downside that saying "class FOO" for every change can get tedious.
+ *
+ * V2 is smarter, it protects class-maps if the defining module also
+ * calls DYNAMIC_DEBUG_CLASSMAP_PARAM to create a sysfs parameter.
+ * Since the author wants the knob, we should assume they intend to
+ * use it (in preference to "class FOO +p" >control), and want to
+ * trust its settings. This gives protection when its useful, and not
+ * when its just tedious.
+ */
+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
@@ -214,11 +258,10 @@ ddebug_find_valid_class(struct _ddebug_info const *di,
const char *query_class,
*/
static bool ddebug_match_desc(const struct ddebug_query *query,
struct _ddebug *dp,
- int valid_class)
+ struct _ddebug_info *di,
+ int selected_class)
{
- /* match site against query-class */
- if (dp->class_id != valid_class)
- return false;
+ struct _ddebug_class_map *site_map;
/* match against the source filename */
if (query->filename &&
@@ -255,7 +298,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) {
+ /* _UNKNOWN_ class_id. XXX: Allow changes here ? */
+ return false;
+ }
