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

[Stephen Boyd]

Quoting Petr Mladek (2021-04-13 08:16:20)
> On Tue 2021-04-13 13:56:31, Andy Shevchenko wrote:
> > On Mon, Apr 12, 2021 at 12:29:05PM -0700, Stephen Boyd wrote:
> > > Quoting Andy Shevchenko (2021-04-12 04:58:02)
> > > > 
> > > > First of all, why not static_assert() defined near to the actual macro?
> > > 
> > > Which macro? BUILD_ID_SIZE_MAX?
> > 
> > Yes.
> > 
> > > I tried static_assert() and it didn't
> > > work for me but maybe I missed something.
> 
> I guess that you wanted to use it inside macro definition:
> 
> #define VMCOREINFO_BUILD_ID(value) \
>         static_assert(ARRAY_SIZE(value) == BUILD_ID_SIZE_MAX); \
>         vmcoreinfo_append_str("BUILD-ID=%20phN\n", value)
> 
> Instead, you should do it outside the macro:
> 
> static_assert(ARRAY_SIZE(value) == BUILD_ID_SIZE_MAX);
> #define VMCOREINFO_BUILD_ID(value) \
>         vmcoreinfo_append_str("BUILD-ID=%20phN\n", value)

In this example "value" is not defined because it's an argument to the
macro. How can this work?

>From what I can tell static_assert() is for the case that you want to
assert something at the global scope level. BUILD_BUG_ON() can't be used
at global scope. I see the usage is usually to assert struct members and
alignment of those members. In turn, static_assert() can't be used at
function level scope. Each has a use and in this case I want to assert
at function level scope to be as close as possible to the place that
would need to change.

> 
> > Sounds weird. static_assert() is a good one. Check, for example, lib/vsprintf.c
> > on how to use it.
> > 
> > > Why is static_assert()
> > > preferred?
> 
> I guess that it is because it is enough and more efficient for
> checks of constant values (no computation of the value).
> 
> > Because it's cleaner way to achieve it and as a bonus it can be put outside of
> > the functions (be in the header or so).

Ok, but I'm still not sure what it would be enforcing. In this case we
need to have it near the sprintf line so that we know to fix the 20 in
there should it ever change to be larger. If it's defined next to the
BUILD_ID_SIZE_MAX macro then it does practically nothing to help future
developers know what to change.

> > 
> > > > > +             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);
> > > > 
> > > 
> > > Are you suggesting to use sprintf format here so that the size is part
> > > of the printf? Sounds good to me. Thanks.
> > 
> > I prefer %20phN when the size is carved in stone (for example by
> > specification), but if you are really expecting that it may be
> > changed in the future, use variadic approach as I showed above.
> 
> I would consider this written in stone (last famous words ;-) and use
> %20phN with the static_assert().
> 

Yes it is pretty much written in stone. The build ID can be an md5sum
instead of SHA1, and thus 16 bytes instead of 20 bytes for the 160-bit
SHA1 form. This is rare, and the code in buildid.c is padding it out
with zeroes in the case that the note is smaller than 20 bytes in
length. Within the kernel we can always assume the buffer is
BUILD_ID_SIZE_MAX. How about this patch?

----8<-----
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index 2174dab16ba9..042c9c034fba 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -39,7 +39,7 @@ phys_addr_t paddr_vmcoreinfo_note(void);
 #define VMCOREINFO_OSRELEASE(value) \
 	vmcoreinfo_append_str("OSRELEASE=%s\n", value)
 #define VMCOREINFO_BUILD_ID(value) \
-	BUILD_BUG_ON(ARRAY_SIZE(value) != BUILD_ID_SIZE_MAX); \
+	BUILD_BUG_ON(ARRAY_SIZE(value) != 20); \
 	vmcoreinfo_append_str("BUILD-ID=%20phN\n", value)
 #define VMCOREINFO_PAGESIZE(value) \
 	vmcoreinfo_append_str("PAGESIZE=%ld\n", value)
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index b835992e76c2..5d9c7ac80633 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -25,7 +25,10 @@
 #include <linux/filter.h>
 #include <linux/ftrace.h>
 #include <linux/kprobes.h>
+#include <linux/build_bug.h>
 #include <linux/compiler.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
 
 /*
  * These will be re-linked against their real values
@@ -394,7 +397,7 @@ static int __sprint_symbol(char *buffer, unsigned long address,
 	if (modname) {
 		len += sprintf(buffer + len, " [%s", modname);
 		/* build ID should match length of sprintf below */
-		BUILD_BUG_ON(BUILD_ID_SIZE_MAX != 20);
+		BUILD_BUG_ON(sizeof(typeof_member(struct module, build_id)) != 20);
 		if (IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) && add_buildid && buildid)
 			len += sprintf(buffer + len, " %20phN", buildid);
 		len += sprintf(buffer + len, "]");