2021-04-08 18:03:01

by Quentin Perret

[permalink] [raw]
Subject: [PATCH] export: Make CRCs robust to symbol trimming

The CRC calculation done by genksyms is triggered when the parser hits
EXPORT_SYMBOL*() macros. At this point, genksyms recursively expands the
types, and uses that as the input for the CRC calculation. In the case
of forward-declared structs, the type expands to 'UNKNOWN'. Next, the
result of the expansion of each type is cached, and is re-used when/if
the same type is seen again for another exported symbol in the file.

Unfortunately, this can cause CRC 'stability' issues when a struct
definition becomes visible in the middle of a C file. For example, let's
assume code with the following pattern:

struct foo;

int bar(struct foo *arg)
{
/* Do work ... */
}
EXPORT_SYMBOL_GPL(bar);

/* This contains struct foo's definition */
#include "foo.h"

int baz(struct foo *arg)
{
/* Do more work ... */
}
EXPORT_SYMBOL_GPL(baz);

Here, baz's CRC will be computed using the expansion of struct foo that
was cached after bar's CRC calculation ('UNKOWN' here). But if
EXPORT_SYMBOL_GPL(bar) is removed from the file (because of e.g. symbol
trimming using CONFIG_TRIM_UNUSED_KSYMS), struct foo will be expanded
late, during baz's CRC calculation, which now has visibility over the
full struct definition, hence resulting in a different CRC for baz.

This can cause annoying issues for distro kernel (such as the Android
Generic Kernel Image) which use CONFIG_UNUSED_KSYMS_WHITELIST. Indeed,
as per the above, adding a symbol to the whitelist can change the CRC of
symbols that are already kept exported. As such, modules built against a
kernel with a trimmed ABI may not load against the same kernel built
with an extended whitelist, even though they are still strictly binary
compatible. While rebuilding the modules would obviously solve the
issue, I believe this classifies as an odd genksyms corner case, and it
gets in the way of kernel updates in the GKI context.

To work around the issue, make sure to keep issuing the
__GENKSYMS_EXPORT_SYMBOL macros for all trimmed symbols, hence making
the genksyms parsing insensitive to symbol trimming.

Signed-off-by: Quentin Perret <[email protected]>
---
include/linux/export.h | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/include/linux/export.h b/include/linux/export.h
index 6271a5d9c988..27d848712b90 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -140,7 +140,12 @@ struct kernel_symbol {
#define ___cond_export_sym(sym, sec, ns, enabled) \
__cond_export_sym_##enabled(sym, sec, ns)
#define __cond_export_sym_1(sym, sec, ns) ___EXPORT_SYMBOL(sym, sec, ns)
+
+#ifdef __GENKSYMS__
+#define __cond_export_sym_0(sym, sec, ns) __GENKSYMS_EXPORT_SYMBOL(sym)
+#else
#define __cond_export_sym_0(sym, sec, ns) /* nothing */
+#endif

#else

--
2.31.0.208.g409f899ff0-goog


2021-04-08 18:16:45

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] export: Make CRCs robust to symbol trimming

On Thu, Apr 08, 2021 at 06:01:05PM +0000, Quentin Perret wrote:
> The CRC calculation done by genksyms is triggered when the parser hits
> EXPORT_SYMBOL*() macros. At this point, genksyms recursively expands the
> types, and uses that as the input for the CRC calculation. In the case
> of forward-declared structs, the type expands to 'UNKNOWN'. Next, the
> result of the expansion of each type is cached, and is re-used when/if
> the same type is seen again for another exported symbol in the file.
>
> Unfortunately, this can cause CRC 'stability' issues when a struct
> definition becomes visible in the middle of a C file. For example, let's
> assume code with the following pattern:
>
> struct foo;
>
> int bar(struct foo *arg)
> {
> /* Do work ... */
> }
> EXPORT_SYMBOL_GPL(bar);
>
> /* This contains struct foo's definition */
> #include "foo.h"
>
> int baz(struct foo *arg)
> {
> /* Do more work ... */
> }
> EXPORT_SYMBOL_GPL(baz);
>
> Here, baz's CRC will be computed using the expansion of struct foo that
> was cached after bar's CRC calculation ('UNKOWN' here). But if
> EXPORT_SYMBOL_GPL(bar) is removed from the file (because of e.g. symbol
> trimming using CONFIG_TRIM_UNUSED_KSYMS), struct foo will be expanded
> late, during baz's CRC calculation, which now has visibility over the
> full struct definition, hence resulting in a different CRC for baz.
>
> This can cause annoying issues for distro kernel (such as the Android
> Generic Kernel Image) which use CONFIG_UNUSED_KSYMS_WHITELIST. Indeed,
> as per the above, adding a symbol to the whitelist can change the CRC of
> symbols that are already kept exported. As such, modules built against a
> kernel with a trimmed ABI may not load against the same kernel built
> with an extended whitelist, even though they are still strictly binary
> compatible. While rebuilding the modules would obviously solve the
> issue, I believe this classifies as an odd genksyms corner case, and it
> gets in the way of kernel updates in the GKI context.
>
> To work around the issue, make sure to keep issuing the
> __GENKSYMS_EXPORT_SYMBOL macros for all trimmed symbols, hence making
> the genksyms parsing insensitive to symbol trimming.
>
> Signed-off-by: Quentin Perret <[email protected]>
> ---
> include/linux/export.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/export.h b/include/linux/export.h
> index 6271a5d9c988..27d848712b90 100644
> --- a/include/linux/export.h
> +++ b/include/linux/export.h
> @@ -140,7 +140,12 @@ struct kernel_symbol {
> #define ___cond_export_sym(sym, sec, ns, enabled) \
> __cond_export_sym_##enabled(sym, sec, ns)
> #define __cond_export_sym_1(sym, sec, ns) ___EXPORT_SYMBOL(sym, sec, ns)
> +
> +#ifdef __GENKSYMS__
> +#define __cond_export_sym_0(sym, sec, ns) __GENKSYMS_EXPORT_SYMBOL(sym)
> +#else
> #define __cond_export_sym_0(sym, sec, ns) /* nothing */
> +#endif
>
> #else
>

Anything to help make these symbol values more "stable" is good, they
drive me crazy...

Acked-by: Greg Kroah-Hartman <[email protected]>