linux-kernel - Re: [PATCH v4 05/13] module: Add printk formats to add module build ID to stacktraces

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YHQ1yqVkweZeN5+1@smile.fi.intel.com>
Date:   Mon, 12 Apr 2021 14:58:02 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, Jiri Olsa <jolsa@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Jessica Yu <jeyu@...nel.org>,
        Evan Green <evgreen@...omium.org>,
        Hsin-Yi Wang <hsinyi@...omium.org>,
        Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        linux-doc@...r.kernel.org, Matthew Wilcox <willy@...radead.org>
Subject: Re: [PATCH v4 05/13] module: Add printk formats to add module build
 ID to stacktraces

On Fri, Apr 09, 2021 at 06:52:52PM -0700, Stephen Boyd wrote:
> Let's make kernel stacktraces easier to identify by including the build
> ID[1] of a module if the stacktrace is printing a symbol from a module.
> This makes it simpler for developers to locate a kernel module's full
> debuginfo for a particular stacktrace. Combined with
> scripts/decode_stracktrace.sh, a developer can download the matching
> debuginfo from a debuginfod[2] server and find the exact file and line
> number for the functions plus offsets in a stacktrace that match the
> module. This is especially useful for pstore crash debugging where the
> kernel crashes are recorded in something like console-ramoops and the
> recovery kernel/modules are different or the debuginfo doesn't exist on
> the device due to space concerns (the debuginfo can be too large for
> space limited devices).
> 
> Originally, I put this on the %pS format, but that was quickly rejected
> given that %pS is used in other places such as ftrace where build IDs
> aren't meaningful. There was some discussions on the list to put every
> module build ID into the "Modules linked in:" section of the stacktrace
> message but that quickly becomes very hard to read once you have more
> than three or four modules linked in. It also provides too much
> information when we don't expect each module to be traversed in a
> stacktrace. Having the build ID for modules that aren't important just
> makes things messy. Splitting it to multiple lines for each module
> quickly explodes the number of lines printed in an oops too, possibly
> wrapping the warning off the console. And finally, trying to stash away
> each module used in a callstack to provide the ID of each symbol printed
> is cumbersome and would require changes to each architecture to stash
> away modules and return their build IDs once unwinding has completed.
> 
> Instead, we opt for the simpler approach of introducing new printk
> formats '%pS[R]b' for "pointer symbolic backtrace with module build ID"
> and '%pBb' for "pointer backtrace with module build ID" and then
> updating the few places in the architecture layer where the stacktrace
> is printed to use this new format.
> 
> Example:

Can you trim a bit the example, so we will see only important lines.
In such case you may provide "before" and "after" variants.

...

> -	if (modname)
> -		len += sprintf(buffer + len, " [%s]", modname);
> +	if (modname) {
> +		len += sprintf(buffer + len, " [%s", modname);

> +		/* build ID should match length of sprintf below */
> +		BUILD_BUG_ON(BUILD_ID_SIZE_MAX != 20);

First of all, why not static_assert() defined near to the actual macro?

> +		if (IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) && add_buildid && buildid)
> +			len += sprintf(buffer + len, " %20phN", buildid);

			len += sprintf(buffer + len, " %*phN", BUILD_ID_SIZE_MAX, buildid);

?

-- 
With Best Regards,
Andy Shevchenko