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
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]>
Hi,
On Thursday 08 Apr 2021 at 18:01:05 (+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]>
Gentle ping.
Is there anything else I should do in this one?
Thanks,
Quentin
On Fri, 21 May 2021, Greg Kroah-Hartman wrote:
> On Fri, May 21, 2021 at 10:57:44AM +0100, Lee Jones wrote:
> > On Mon, 10 May 2021 at 11:25, Quentin Perret <[email protected]> wrote:
> >
> > > Hi,
> > >
> > > On Thursday 08 Apr 2021 at 18:01:05 (+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]>
> > >
> > > Gentle ping.
> > >
> > > Is there anything else I should do in this one?
> > >
> >
> > With Greg's Ack and ~6 weeks on the list, you're probably golden.
> >
> > I *could* pick this up, but seems wrong somehow.
> >
> > Greg, is this something you're prepared to merge? If not, who's the
> > g{uy,al}?
>
> What does get_maintainer.pl show?
It doesn't [0], which is why I commented in this way. :)
[0]:
Jessica Yu <[email protected]> (commit_signer:2/5=40%)
Emil Velikov <[email protected]> (commit_signer:2/5=40%)
Miroslav Benes <[email protected]> (commit_signer:2/5=40%)
Nick Desaulniers <[email protected]> (commit_signer:2/5=40%,authored:1/5=20%,added_lines:1/7=14%,removed_lines:1/11=9%)
Greg Kroah-Hartman <[email protected]> (commit_signer:1/5=20%)
Quentin Perret <[email protected]> (authored:1/5=20%,added_lines:5/7=71%)
Joe Perches <[email protected]> (authored:1/5=20%,added_lines:1/7=14%,removed_lines:1/11=9%)
[email protected] (open list)
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, May 21, 2021 at 12:02:06PM +0100, Lee Jones wrote:
> On Fri, 21 May 2021, Greg Kroah-Hartman wrote:
>
> > On Fri, May 21, 2021 at 10:57:44AM +0100, Lee Jones wrote:
> > > On Mon, 10 May 2021 at 11:25, Quentin Perret <[email protected]> wrote:
> > >
> > > > Hi,
> > > >
> > > > On Thursday 08 Apr 2021 at 18:01:05 (+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]>
> > > >
> > > > Gentle ping.
> > > >
> > > > Is there anything else I should do in this one?
> > > >
> > >
> > > With Greg's Ack and ~6 weeks on the list, you're probably golden.
> > >
> > > I *could* pick this up, but seems wrong somehow.
> > >
> > > Greg, is this something you're prepared to merge? If not, who's the
> > > g{uy,al}?
> >
> > What does get_maintainer.pl show?
>
> It doesn't [0], which is why I commented in this way. :)
>
> [0]:
>
> Jessica Yu <[email protected]> (commit_signer:2/5=40%)
> Emil Velikov <[email protected]> (commit_signer:2/5=40%)
> Miroslav Benes <[email protected]> (commit_signer:2/5=40%)
> Nick Desaulniers <[email protected]> (commit_signer:2/5=40%,authored:1/5=20%,added_lines:1/7=14%,removed_lines:1/11=9%)
> Greg Kroah-Hartman <[email protected]> (commit_signer:1/5=20%)
> Quentin Perret <[email protected]> (authored:1/5=20%,added_lines:5/7=71%)
> Joe Perches <[email protected]> (authored:1/5=20%,added_lines:1/7=14%,removed_lines:1/11=9%)
> [email protected] (open list)
Bah, ok, fine, I'll scoop it up now :)
greg k-h
On Fri, May 21, 2021 at 10:57:44AM +0100, Lee Jones wrote:
> On Mon, 10 May 2021 at 11:25, Quentin Perret <[email protected]> wrote:
>
> > Hi,
> >
> > On Thursday 08 Apr 2021 at 18:01:05 (+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]>
> >
> > Gentle ping.
> >
> > Is there anything else I should do in this one?
> >
>
> With Greg's Ack and ~6 weeks on the list, you're probably golden.
>
> I *could* pick this up, but seems wrong somehow.
>
> Greg, is this something you're prepared to merge? If not, who's the
> g{uy,al}?
What does get_maintainer.pl show?