Re: [PATCH v13 19/39] dyndbg,module: make proper substructs in _ddebug_info

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dyndbg: __UNIQUE_ID(_ddebug) prefix change breaks objtool klp diff
--

commit 317df06d413f02902ba4470f9dd1320a81d15cee
Author: Jim Cromie <jim.cromie@gmail.com>

dyndbg,module: make proper substructs in _ddebug_info

This commit refactors struct _ddebug_info to group descriptor and class map
vectors into struct _ddebug_descs and struct _ddebug_class_maps substructs,
embeds struct _ddebug_info into struct ddebug_table, and renames the linker
sections accordingly.

> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> index 36b7d341076db..b05dc843a1d8e 100644
> --- a/include/linux/dynamic_debug.h
> +++ b/include/linux/dynamic_debug.h

[ ... ]

> @@ -245,7 +257,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
>   * macro.
>   */
>  #define _dynamic_func_call_cls(cls, fmt, func, ...)			\
> -	__dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
> +	__dynamic_func_call_cls(__UNIQUE_ID(_ddebug), cls, fmt, func, ##__VA_ARGS__)
>  #define _dynamic_func_call(fmt, func, ...)				\
>  	_dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
>  
> @@ -255,7 +267,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
>   * with precisely the macro's varargs.
>   */
>  #define _dynamic_func_call_cls_no_desc(cls, fmt, func, ...)		\
> -	__dynamic_func_call_cls_no_desc(__UNIQUE_ID(ddebug), cls, fmt,	\
> +	__dynamic_func_call_cls_no_desc(__UNIQUE_ID(_ddebug), cls, fmt,	\
>  					func, ##__VA_ARGS__)

[Severity: Medium]
Does changing __UNIQUE_ID(ddebug) to __UNIQUE_ID(_ddebug) in
_dynamic_func_call_cls() and _dynamic_func_call_cls_no_desc() break livepatch
builds with objtool klp diff?

In include/linux/compiler.h, __UNIQUE_ID() already appends an underscore
after __UNIQUE_ID_:

    /* Format: __UNIQUE_ID_<name>_<__COUNTER__> */
    #define __UNIQUE_ID(name)					\
	    __PASTE(__UNIQUE_ID_,				\
	    __PASTE(name,					\
	    __PASTE(_, __COUNTER__)))

Passing _ddebug changes the generated static struct _ddebug descriptor symbol
names from __UNIQUE_ID_ddebug_<N> to __UNIQUE_ID__ddebug_<N> (with a double
underscore).

When CONFIG_JUMP_LABEL is enabled and a livepatched module function contains a
dynamic debug callsite, tools/objtool/klp-diff.c checks for the single
underscore prefix in validate_special_section_klp_reloc():

    if (strstr(reloc->sym->name, "__UNIQUE_ID_ddebug_")) {
	    WARN("%s: disabling unsupported pr_debug()",
		 code_sym);
	    ret = 1;
	    continue;
    }

    ERROR("%s+0x%lx: unsupported static branch key %s.  Use static_key_enabled() instead",
	  code_sym, code_offset, reloc->sym->name);
    return -1;

With the extra underscore in __UNIQUE_ID__ddebug_<N>, strstr() will no longer
match and validate_special_section_klp_reloc() will fail with an unsupported
static branch key error. Should this remain __UNIQUE_ID(ddebug), or should
validate_special_section_klp_reloc() in tools/objtool/klp-diff.c be updated to
match?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-dd-cmap-part2-clean-v13-0-9267923be612@gmail.com?part=19