The symbol CRCs are emitted as ELF symbols, which allows us to easily
populate the kcrctab sections by relying on the linker to associate
each kcrctab slot with the correct value.
This has two downsides:
- given that the CRCs are treated as pointers, we waste 4 bytes for
each CRC on 64 bit architectures,
- on architectures that support runtime relocation, a relocation entry is
emitted for each CRC value, which may take up 24 bytes of __init space
(on ELF64 systems)
This comes down to a x8 overhead in [uncompressed] kernel size. In addition,
each relocation has to be reverted before the CRC value can be used.
Switching to explicit 32 bit values on 64 bit architectures fixes both
issues, since 32 bit values are not treated as relocatable quantities on
ELF64 systems, even if the value ultimately resolves to a linker supplied
value.
So redefine all CRC fields and variables as u32, and redefine the
__CRC_SYMBOL() macro for 64 bit builds to emit the CRC reference using
inline assembler (which is necessary since 64-bit C code cannot use
32-bit types to hold memory addresses, even if they are ultimately
resolved using values that do no exceed 0xffffffff).
Also remove the special handling for PPC64, this should no longer be
required.
Signed-off-by: Ard Biesheuvel <[email protected]>
---
v2: drop the change to struct modversion_info: it affects the layout of the
__versions section, which is consumed by userland tools as well, so it is
effectively ABI
On an arm64 defconfig build with CONFIG_RELOCATABLE=y, this patch reduces
the CRC footprint by 24 KB for .rodata, and by 217 KB for .init
Before:
[ 9] __kcrctab PROGBITS ffff000008b992a8 00b292a8
0000000000009440 0000000000000000 A 0 0 8
[10] __kcrctab_gpl PROGBITS ffff000008ba26e8 00b326e8
0000000000008d40 0000000000000000 A 0 0 8
...
[22] .rela RELA ffff000008c96e20 00c26e20
00000000001cc758 0000000000000018 A 0 0 8
After:
[ 9] __kcrctab PROGBITS ffff000008b728a8 00b028a8
0000000000004a20 0000000000000000 A 0 0 1
[10] __kcrctab_gpl PROGBITS ffff000008b772c8 00b072c8
00000000000046a0 0000000000000000 A 0 0 1
...
[22] .rela RELA ffff000008c66e20 00bf6e20
00000000001962d8 0000000000000018 A 0 0 8
arch/powerpc/include/asm/module.h | 4 --
arch/powerpc/kernel/module_64.c | 8 ----
include/linux/export.h | 8 ++++
include/linux/module.h | 14 +++----
kernel/module.c | 39 +++++++-------------
5 files changed, 29 insertions(+), 44 deletions(-)
diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
index cd4ffd86765f..94a7f7aa3ae8 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -94,9 +94,5 @@ struct exception_table_entry;
void sort_ex_table(struct exception_table_entry *start,
struct exception_table_entry *finish);
-#if defined(CONFIG_MODVERSIONS) && defined(CONFIG_PPC64)
-#define ARCH_RELOCATES_KCRCTAB
-#define reloc_start PHYSICAL_START
-#endif
#endif /* __KERNEL__ */
#endif /* _ASM_POWERPC_MODULE_H */
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 183368e008cf..be9b2d5ff846 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -286,14 +286,6 @@ static void dedotify_versions(struct modversion_info *vers,
for (end = (void *)vers + size; vers < end; vers++)
if (vers->name[0] == '.') {
memmove(vers->name, vers->name+1, strlen(vers->name));
-#ifdef ARCH_RELOCATES_KCRCTAB
- /* The TOC symbol has no CRC computed. To avoid CRC
- * check failing, we must force it to the expected
- * value (see CRC check in module.c).
- */
- if (!strcmp(vers->name, "TOC."))
- vers->crc = -(unsigned long)reloc_start;
-#endif
}
}
diff --git a/include/linux/export.h b/include/linux/export.h
index 2a0f61fbc731..fa51ab2ad190 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -41,6 +41,7 @@ extern struct module __this_module;
#if defined(__KERNEL__) && !defined(__GENKSYMS__)
#ifdef CONFIG_MODVERSIONS
+#ifndef CONFIG_64BIT
/* Mark the CRC weak since genksyms apparently decides not to
* generate a checksums for some symbols */
#define __CRC_SYMBOL(sym, sec) \
@@ -50,6 +51,13 @@ extern struct module __this_module;
__attribute__((section("___kcrctab" sec "+" #sym), used)) \
= (unsigned long) &__crc_##sym;
#else
+#define __CRC_SYMBOL(sym, sec) \
+ asm(" .section \"___kcrctab" sec "+" #sym "\", \"a\" \n" \
+ " .weak " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
+ " .word " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
+ " .previous \n");
+#endif
+#else
#define __CRC_SYMBOL(sym, sec)
#endif
diff --git a/include/linux/module.h b/include/linux/module.h
index 0c3207d26ac0..e0067673f5e5 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -346,7 +346,7 @@ struct module {
/* Exported symbols */
const struct kernel_symbol *syms;
- const unsigned long *crcs;
+ const u32 *crcs;
unsigned int num_syms;
/* Kernel parameters. */
@@ -359,18 +359,18 @@ struct module {
/* GPL-only exported symbols. */
unsigned int num_gpl_syms;
const struct kernel_symbol *gpl_syms;
- const unsigned long *gpl_crcs;
+ const u32 *gpl_crcs;
#ifdef CONFIG_UNUSED_SYMBOLS
/* unused exported symbols. */
const struct kernel_symbol *unused_syms;
- const unsigned long *unused_crcs;
+ const u32 *unused_crcs;
unsigned int num_unused_syms;
/* GPL-only, unused exported symbols. */
unsigned int num_unused_gpl_syms;
const struct kernel_symbol *unused_gpl_syms;
- const unsigned long *unused_gpl_crcs;
+ const u32 *unused_gpl_crcs;
#endif
#ifdef CONFIG_MODULE_SIG
@@ -382,7 +382,7 @@ struct module {
/* symbols that will be GPL-only in the near future. */
const struct kernel_symbol *gpl_future_syms;
- const unsigned long *gpl_future_crcs;
+ const u32 *gpl_future_crcs;
unsigned int num_gpl_future_syms;
/* Exception table */
@@ -523,7 +523,7 @@ struct module *find_module(const char *name);
struct symsearch {
const struct kernel_symbol *start, *stop;
- const unsigned long *crcs;
+ const u32 *crcs;
enum {
NOT_GPL_ONLY,
GPL_ONLY,
@@ -539,7 +539,7 @@ struct symsearch {
*/
const struct kernel_symbol *find_symbol(const char *name,
struct module **owner,
- const unsigned long **crc,
+ const u32 **crc,
bool gplok,
bool warn);
diff --git a/kernel/module.c b/kernel/module.c
index f57dd63186e6..90ecdad07e1a 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -386,16 +386,16 @@ extern const struct kernel_symbol __start___ksymtab_gpl[];
extern const struct kernel_symbol __stop___ksymtab_gpl[];
extern const struct kernel_symbol __start___ksymtab_gpl_future[];
extern const struct kernel_symbol __stop___ksymtab_gpl_future[];
-extern const unsigned long __start___kcrctab[];
-extern const unsigned long __start___kcrctab_gpl[];
-extern const unsigned long __start___kcrctab_gpl_future[];
+extern const u32 __start___kcrctab[];
+extern const u32 __start___kcrctab_gpl[];
+extern const u32 __start___kcrctab_gpl_future[];
#ifdef CONFIG_UNUSED_SYMBOLS
extern const struct kernel_symbol __start___ksymtab_unused[];
extern const struct kernel_symbol __stop___ksymtab_unused[];
extern const struct kernel_symbol __start___ksymtab_unused_gpl[];
extern const struct kernel_symbol __stop___ksymtab_unused_gpl[];
-extern const unsigned long __start___kcrctab_unused[];
-extern const unsigned long __start___kcrctab_unused_gpl[];
+extern const u32 __start___kcrctab_unused[];
+extern const u32 __start___kcrctab_unused_gpl[];
#endif
#ifndef CONFIG_MODVERSIONS
@@ -494,7 +494,7 @@ struct find_symbol_arg {
/* Output */
struct module *owner;
- const unsigned long *crc;
+ const u32 *crc;
const struct kernel_symbol *sym;
};
@@ -560,7 +560,7 @@ static bool find_symbol_in_section(const struct symsearch *syms,
* (optional) module which owns it. Needs preempt disabled or module_mutex. */
const struct kernel_symbol *find_symbol(const char *name,
struct module **owner,
- const unsigned long **crc,
+ const u32 **crc,
bool gplok,
bool warn)
{
@@ -1257,22 +1257,11 @@ static int try_to_force_load(struct module *mod, const char *reason)
}
#ifdef CONFIG_MODVERSIONS
-/* If the arch applies (non-zero) relocations to kernel kcrctab, unapply it. */
-static unsigned long maybe_relocated(unsigned long crc,
- const struct module *crc_owner)
-{
-#ifdef ARCH_RELOCATES_KCRCTAB
- if (crc_owner == NULL)
- return crc - (unsigned long)reloc_start;
-#endif
- return crc;
-}
-
static int check_version(Elf_Shdr *sechdrs,
unsigned int versindex,
const char *symname,
struct module *mod,
- const unsigned long *crc,
+ const u32 *crc,
const struct module *crc_owner)
{
unsigned int i, num_versions;
@@ -1294,10 +1283,10 @@ static int check_version(Elf_Shdr *sechdrs,
if (strcmp(versions[i].name, symname) != 0)
continue;
- if (versions[i].crc == maybe_relocated(*crc, crc_owner))
+ if (versions[i].crc == *crc)
return 1;
- pr_debug("Found checksum %lX vs module %lX\n",
- maybe_relocated(*crc, crc_owner), versions[i].crc);
+ pr_debug("Found checksum %X vs module %lX\n",
+ *crc, versions[i].crc);
goto bad_version;
}
@@ -1314,7 +1303,7 @@ static inline int check_modstruct_version(Elf_Shdr *sechdrs,
unsigned int versindex,
struct module *mod)
{
- const unsigned long *crc;
+ const u32 *crc;
/*
* Since this should be found in kernel (which can't be removed), no
@@ -1347,7 +1336,7 @@ static inline int check_version(Elf_Shdr *sechdrs,
unsigned int versindex,
const char *symname,
struct module *mod,
- const unsigned long *crc,
+ const u32 *crc,
const struct module *crc_owner)
{
return 1;
@@ -1375,7 +1364,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
{
struct module *owner;
const struct kernel_symbol *sym;
- const unsigned long *crc;
+ const u32 *crc;
int err;
/*
--
2.7.4
Ard Biesheuvel <[email protected]> writes:
> The symbol CRCs are emitted as ELF symbols, which allows us to easily
> populate the kcrctab sections by relying on the linker to associate
> each kcrctab slot with the correct value.
>
> This has two downsides:
> - given that the CRCs are treated as pointers, we waste 4 bytes for
> each CRC on 64 bit architectures,
> - on architectures that support runtime relocation, a relocation entry is
> emitted for each CRC value, which may take up 24 bytes of __init space
> (on ELF64 systems)
>
> This comes down to a x8 overhead in [uncompressed] kernel size. In addition,
> each relocation has to be reverted before the CRC value can be used.
>
> Switching to explicit 32 bit values on 64 bit architectures fixes both
> issues, since 32 bit values are not treated as relocatable quantities on
> ELF64 systems, even if the value ultimately resolves to a linker supplied
> value.
>
> So redefine all CRC fields and variables as u32, and redefine the
> __CRC_SYMBOL() macro for 64 bit builds to emit the CRC reference using
> inline assembler (which is necessary since 64-bit C code cannot use
> 32-bit types to hold memory addresses, even if they are ultimately
> resolved using values that do no exceed 0xffffffff).
>
> Also remove the special handling for PPC64, this should no longer be
> required.
>
> Signed-off-by: Ard Biesheuvel <[email protected]>
This looks good! Thanks for this, it fixes a nasty wart with the
relocation of crcs.
If the ppc and arm maintainers are happy, I'm happy for Jessica to take
it into her module tree.
Acked-by: Rusty Russell <[email protected]>
Thanks,
Rusty.
> ---
> v2: drop the change to struct modversion_info: it affects the layout of the
> __versions section, which is consumed by userland tools as well, so it is
> effectively ABI
>
> On an arm64 defconfig build with CONFIG_RELOCATABLE=y, this patch reduces
> the CRC footprint by 24 KB for .rodata, and by 217 KB for .init
>
> Before:
> [ 9] __kcrctab PROGBITS ffff000008b992a8 00b292a8
> 0000000000009440 0000000000000000 A 0 0 8
> [10] __kcrctab_gpl PROGBITS ffff000008ba26e8 00b326e8
> 0000000000008d40 0000000000000000 A 0 0 8
> ...
> [22] .rela RELA ffff000008c96e20 00c26e20
> 00000000001cc758 0000000000000018 A 0 0 8
>
> After:
> [ 9] __kcrctab PROGBITS ffff000008b728a8 00b028a8
> 0000000000004a20 0000000000000000 A 0 0 1
> [10] __kcrctab_gpl PROGBITS ffff000008b772c8 00b072c8
> 00000000000046a0 0000000000000000 A 0 0 1
> ...
> [22] .rela RELA ffff000008c66e20 00bf6e20
> 00000000001962d8 0000000000000018 A 0 0 8
>
> arch/powerpc/include/asm/module.h | 4 --
> arch/powerpc/kernel/module_64.c | 8 ----
> include/linux/export.h | 8 ++++
> include/linux/module.h | 14 +++----
> kernel/module.c | 39 +++++++-------------
> 5 files changed, 29 insertions(+), 44 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
> index cd4ffd86765f..94a7f7aa3ae8 100644
> --- a/arch/powerpc/include/asm/module.h
> +++ b/arch/powerpc/include/asm/module.h
> @@ -94,9 +94,5 @@ struct exception_table_entry;
> void sort_ex_table(struct exception_table_entry *start,
> struct exception_table_entry *finish);
>
> -#if defined(CONFIG_MODVERSIONS) && defined(CONFIG_PPC64)
> -#define ARCH_RELOCATES_KCRCTAB
> -#define reloc_start PHYSICAL_START
> -#endif
> #endif /* __KERNEL__ */
> #endif /* _ASM_POWERPC_MODULE_H */
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 183368e008cf..be9b2d5ff846 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -286,14 +286,6 @@ static void dedotify_versions(struct modversion_info *vers,
> for (end = (void *)vers + size; vers < end; vers++)
> if (vers->name[0] == '.') {
> memmove(vers->name, vers->name+1, strlen(vers->name));
> -#ifdef ARCH_RELOCATES_KCRCTAB
> - /* The TOC symbol has no CRC computed. To avoid CRC
> - * check failing, we must force it to the expected
> - * value (see CRC check in module.c).
> - */
> - if (!strcmp(vers->name, "TOC."))
> - vers->crc = -(unsigned long)reloc_start;
> -#endif
> }
> }
>
> diff --git a/include/linux/export.h b/include/linux/export.h
> index 2a0f61fbc731..fa51ab2ad190 100644
> --- a/include/linux/export.h
> +++ b/include/linux/export.h
> @@ -41,6 +41,7 @@ extern struct module __this_module;
>
> #if defined(__KERNEL__) && !defined(__GENKSYMS__)
> #ifdef CONFIG_MODVERSIONS
> +#ifndef CONFIG_64BIT
> /* Mark the CRC weak since genksyms apparently decides not to
> * generate a checksums for some symbols */
> #define __CRC_SYMBOL(sym, sec) \
> @@ -50,6 +51,13 @@ extern struct module __this_module;
> __attribute__((section("___kcrctab" sec "+" #sym), used)) \
> = (unsigned long) &__crc_##sym;
> #else
> +#define __CRC_SYMBOL(sym, sec) \
> + asm(" .section \"___kcrctab" sec "+" #sym "\", \"a\" \n" \
> + " .weak " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
> + " .word " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
> + " .previous \n");
> +#endif
> +#else
> #define __CRC_SYMBOL(sym, sec)
> #endif
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 0c3207d26ac0..e0067673f5e5 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -346,7 +346,7 @@ struct module {
>
> /* Exported symbols */
> const struct kernel_symbol *syms;
> - const unsigned long *crcs;
> + const u32 *crcs;
> unsigned int num_syms;
>
> /* Kernel parameters. */
> @@ -359,18 +359,18 @@ struct module {
> /* GPL-only exported symbols. */
> unsigned int num_gpl_syms;
> const struct kernel_symbol *gpl_syms;
> - const unsigned long *gpl_crcs;
> + const u32 *gpl_crcs;
>
> #ifdef CONFIG_UNUSED_SYMBOLS
> /* unused exported symbols. */
> const struct kernel_symbol *unused_syms;
> - const unsigned long *unused_crcs;
> + const u32 *unused_crcs;
> unsigned int num_unused_syms;
>
> /* GPL-only, unused exported symbols. */
> unsigned int num_unused_gpl_syms;
> const struct kernel_symbol *unused_gpl_syms;
> - const unsigned long *unused_gpl_crcs;
> + const u32 *unused_gpl_crcs;
> #endif
>
> #ifdef CONFIG_MODULE_SIG
> @@ -382,7 +382,7 @@ struct module {
>
> /* symbols that will be GPL-only in the near future. */
> const struct kernel_symbol *gpl_future_syms;
> - const unsigned long *gpl_future_crcs;
> + const u32 *gpl_future_crcs;
> unsigned int num_gpl_future_syms;
>
> /* Exception table */
> @@ -523,7 +523,7 @@ struct module *find_module(const char *name);
>
> struct symsearch {
> const struct kernel_symbol *start, *stop;
> - const unsigned long *crcs;
> + const u32 *crcs;
> enum {
> NOT_GPL_ONLY,
> GPL_ONLY,
> @@ -539,7 +539,7 @@ struct symsearch {
> */
> const struct kernel_symbol *find_symbol(const char *name,
> struct module **owner,
> - const unsigned long **crc,
> + const u32 **crc,
> bool gplok,
> bool warn);
>
> diff --git a/kernel/module.c b/kernel/module.c
> index f57dd63186e6..90ecdad07e1a 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -386,16 +386,16 @@ extern const struct kernel_symbol __start___ksymtab_gpl[];
> extern const struct kernel_symbol __stop___ksymtab_gpl[];
> extern const struct kernel_symbol __start___ksymtab_gpl_future[];
> extern const struct kernel_symbol __stop___ksymtab_gpl_future[];
> -extern const unsigned long __start___kcrctab[];
> -extern const unsigned long __start___kcrctab_gpl[];
> -extern const unsigned long __start___kcrctab_gpl_future[];
> +extern const u32 __start___kcrctab[];
> +extern const u32 __start___kcrctab_gpl[];
> +extern const u32 __start___kcrctab_gpl_future[];
> #ifdef CONFIG_UNUSED_SYMBOLS
> extern const struct kernel_symbol __start___ksymtab_unused[];
> extern const struct kernel_symbol __stop___ksymtab_unused[];
> extern const struct kernel_symbol __start___ksymtab_unused_gpl[];
> extern const struct kernel_symbol __stop___ksymtab_unused_gpl[];
> -extern const unsigned long __start___kcrctab_unused[];
> -extern const unsigned long __start___kcrctab_unused_gpl[];
> +extern const u32 __start___kcrctab_unused[];
> +extern const u32 __start___kcrctab_unused_gpl[];
> #endif
>
> #ifndef CONFIG_MODVERSIONS
> @@ -494,7 +494,7 @@ struct find_symbol_arg {
>
> /* Output */
> struct module *owner;
> - const unsigned long *crc;
> + const u32 *crc;
> const struct kernel_symbol *sym;
> };
>
> @@ -560,7 +560,7 @@ static bool find_symbol_in_section(const struct symsearch *syms,
> * (optional) module which owns it. Needs preempt disabled or module_mutex. */
> const struct kernel_symbol *find_symbol(const char *name,
> struct module **owner,
> - const unsigned long **crc,
> + const u32 **crc,
> bool gplok,
> bool warn)
> {
> @@ -1257,22 +1257,11 @@ static int try_to_force_load(struct module *mod, const char *reason)
> }
>
> #ifdef CONFIG_MODVERSIONS
> -/* If the arch applies (non-zero) relocations to kernel kcrctab, unapply it. */
> -static unsigned long maybe_relocated(unsigned long crc,
> - const struct module *crc_owner)
> -{
> -#ifdef ARCH_RELOCATES_KCRCTAB
> - if (crc_owner == NULL)
> - return crc - (unsigned long)reloc_start;
> -#endif
> - return crc;
> -}
> -
> static int check_version(Elf_Shdr *sechdrs,
> unsigned int versindex,
> const char *symname,
> struct module *mod,
> - const unsigned long *crc,
> + const u32 *crc,
> const struct module *crc_owner)
> {
> unsigned int i, num_versions;
> @@ -1294,10 +1283,10 @@ static int check_version(Elf_Shdr *sechdrs,
> if (strcmp(versions[i].name, symname) != 0)
> continue;
>
> - if (versions[i].crc == maybe_relocated(*crc, crc_owner))
> + if (versions[i].crc == *crc)
> return 1;
> - pr_debug("Found checksum %lX vs module %lX\n",
> - maybe_relocated(*crc, crc_owner), versions[i].crc);
> + pr_debug("Found checksum %X vs module %lX\n",
> + *crc, versions[i].crc);
> goto bad_version;
> }
>
> @@ -1314,7 +1303,7 @@ static inline int check_modstruct_version(Elf_Shdr *sechdrs,
> unsigned int versindex,
> struct module *mod)
> {
> - const unsigned long *crc;
> + const u32 *crc;
>
> /*
> * Since this should be found in kernel (which can't be removed), no
> @@ -1347,7 +1336,7 @@ static inline int check_version(Elf_Shdr *sechdrs,
> unsigned int versindex,
> const char *symname,
> struct module *mod,
> - const unsigned long *crc,
> + const u32 *crc,
> const struct module *crc_owner)
> {
> return 1;
> @@ -1375,7 +1364,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
> {
> struct module *owner;
> const struct kernel_symbol *sym;
> - const unsigned long *crc;
> + const u32 *crc;
> int err;
>
> /*
> --
> 2.7.4
Hi Ard,
I like the concept, but ...
Ard Biesheuvel <[email protected]> writes:
> The symbol CRCs are emitted as ELF symbols, which allows us to easily
> populate the kcrctab sections by relying on the linker to associate
> each kcrctab slot with the correct value.
>
> This has two downsides:
> - given that the CRCs are treated as pointers, we waste 4 bytes for
> each CRC on 64 bit architectures,
> - on architectures that support runtime relocation, a relocation entry is
> emitted for each CRC value, which may take up 24 bytes of __init space
> (on ELF64 systems)
>
> This comes down to a x8 overhead in [uncompressed] kernel size. In addition,
> each relocation has to be reverted before the CRC value can be used.
>
> Switching to explicit 32 bit values on 64 bit architectures fixes both
> issues, since 32 bit values are not treated as relocatable quantities on
> ELF64 systems, even if the value ultimately resolves to a linker supplied
> value.
Are we sure that part is true? ("not treated as relocatable")
A quick test build on powerpc gives me:
WARNING: 6829 bad relocations
c000000000ca3748 R_PPC64_ADDR16 *ABS*+0x0000000013f53da6
c000000000ca374a R_PPC64_ADDR16 *ABS*+0x00000000f7272059
c000000000ca374c R_PPC64_ADDR16 *ABS*+0x0000000002013d36
c000000000ca374e R_PPC64_ADDR16 *ABS*+0x00000000a59dffc8
...
Which is coming from our relocs_check.sh script, which checks that the
generated relocations are ones we know how to handle.
And when I try to boot it I get:
virtio: disagrees about version of symbol module_layout
virtio: disagrees about version of symbol module_layout
scsi_mod: disagrees about version of symbol module_layout
And it can't find my root file system (unsurprisingly as it's on scsi).
Will try and investigate more tomorrow.
cheers
On 26 October 2016 at 11:07, Michael Ellerman <[email protected]> wrote:
> Hi Ard,
>
> I like the concept, but ...
>
> Ard Biesheuvel <[email protected]> writes:
>> The symbol CRCs are emitted as ELF symbols, which allows us to easily
>> populate the kcrctab sections by relying on the linker to associate
>> each kcrctab slot with the correct value.
>>
>> This has two downsides:
>> - given that the CRCs are treated as pointers, we waste 4 bytes for
>> each CRC on 64 bit architectures,
>> - on architectures that support runtime relocation, a relocation entry is
>> emitted for each CRC value, which may take up 24 bytes of __init space
>> (on ELF64 systems)
>>
>> This comes down to a x8 overhead in [uncompressed] kernel size. In addition,
>> each relocation has to be reverted before the CRC value can be used.
>>
>> Switching to explicit 32 bit values on 64 bit architectures fixes both
>> issues, since 32 bit values are not treated as relocatable quantities on
>> ELF64 systems, even if the value ultimately resolves to a linker supplied
>> value.
>
> Are we sure that part is true? ("not treated as relocatable")
>
Thanks for testing this.
> A quick test build on powerpc gives me:
>
> WARNING: 6829 bad relocations
> c000000000ca3748 R_PPC64_ADDR16 *ABS*+0x0000000013f53da6
> c000000000ca374a R_PPC64_ADDR16 *ABS*+0x00000000f7272059
> c000000000ca374c R_PPC64_ADDR16 *ABS*+0x0000000002013d36
> c000000000ca374e R_PPC64_ADDR16 *ABS*+0x00000000a59dffc8
> ...
>
> Which is coming from our relocs_check.sh script, which checks that the
> generated relocations are ones we know how to handle.
>
OK, first of all, it appears the ppc64 assembler interprets .word
differently than the arm64 one, so I will need to fold this in
"""
diff --git a/include/linux/export.h b/include/linux/export.h
index fa51ab2ad190..a000d421526d 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -54,7 +54,7 @@ extern struct module __this_module;
#define __CRC_SYMBOL(sym, sec) \
asm(" .section \"___kcrctab" sec "+" #sym "\", \"a\" \n" \
" .weak " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
- " .word " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
+ " .long " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
" .previous \n");
#endif
#else
"""
With that change, the CRCs are actually emitted as
WARNING: 7525 bad relocations
c000000000ce7f50 R_PPC64_ADDR32 *ABS*+0x0000000013f53da6
c000000000ce7f54 R_PPC64_ADDR32 *ABS*+0x0000000004f7c64e
c000000000ce7f58 R_PPC64_ADDR32 *ABS*+0x0000000092be8a3e
which is still a bit disappointing, given that we still have 7525 RELA
entries to process.
I tried several thing, i.e., adding -Bsymbolic to the linker command
line, emitting the reference above as .hidden or emit the definition
from the linker script as HIDDEN(), but nothing seems to make any
difference. (On arm64, -Bsymbolic eliminates *all* runtime relocations
except R_<arch>_RELATIVE ones)
> And when I try to boot it I get:
>
> virtio: disagrees about version of symbol module_layout
> virtio: disagrees about version of symbol module_layout
> scsi_mod: disagrees about version of symbol module_layout
>
> And it can't find my root file system (unsurprisingly as it's on scsi).
>
Something like the below should fix it, I hope.
"""
diff --git a/arch/powerpc/kernel/reloc_64.S b/arch/powerpc/kernel/reloc_64.S
index d88736fbece6..99cdf2311ab5 100644
--- a/arch/powerpc/kernel/reloc_64.S
+++ b/arch/powerpc/kernel/reloc_64.S
@@ -14,6 +14,7 @@
RELA = 7
RELACOUNT = 0x6ffffff9
R_PPC64_RELATIVE = 22
+R_PPC64_ADDR32 = 1
/*
* r3 = desired final address of kernel
@@ -77,9 +78,22 @@ _GLOBAL(relocate)
add r0,r0,r3
stdx r0,r7,r6
addi r9,r9,24
- bdnz 5b
+ b 7f
+
+ /*
+ * CRCs of exported symbols are emitted as 32-bit relocations against
+ * the *ABS* section with the CRC value recorded in the addend.
+ */
+6: cmpdi r0,R_PPC64_ADDR32
+ bne 7f
+ ld r6,0(r9) /* reloc->r_offset */
+ ld r0,16(r9) /* reloc->r_addend */
+ stwx r0,r7,r6
+ addi r9,r9,24
+
+7: bdnz 5b
+ blr
-6: blr
.balign 8
p_dyn: .llong __dynamic_start - 0b
"""
Note that the loop no longer terminates at the first
non-R_PPC64_RELATIVE relocation, but that seems safer to me in any
case. It simply stores the value of r_addend at r_offset, which is the
correct thing to do for R_PPC64_ADDR32 relocations against the *ABS*
section, regardless of whether we are dealing with CRCs or something
else. Note that the comparison above will fail for R_PPC64_ADDR32
relocations against named symbols, since we compare the entire r_info
field and not just the type (as the comment a few lines higher up
suggests)
Also a fix for relocs_check.sh:
"""
diff --git a/arch/powerpc/relocs_check.sh b/arch/powerpc/relocs_check.sh
index ec2d5c835170..2f510fbc87da 100755
--- a/arch/powerpc/relocs_check.sh
+++ b/arch/powerpc/relocs_check.sh
@@ -43,7 +43,8 @@ R_PPC_ADDR16_HA
R_PPC_RELATIVE
R_PPC_NONE' |
grep -E -v '\<R_PPC64_ADDR64[[:space:]]+mach_' |
- grep -E -v '\<R_PPC64_ADDR64[[:space:]]+__crc_'
+ grep -E -v '\<R_PPC64_ADDR64[[:space:]]+__crc_' |
+ grep -E -v '\<R_PPC64_ADDR32[[:space:]]+\*ABS\*'
)
if [ -z "$bad_relocs" ]; then
"""
If these changes work for PPC, I think we should fold them in.
Hopefully, GNU ld for PPC will gain that ability to resolve absolute
relocations at build time (like other architectures), and then the
R_PPC64_ADDR32 handling will simply become dead code. In any case, it
is an improvement over the mangling of CRC values to undo the runtime
relocation, imo.
Regards,
Ard.
On 26 October 2016 at 14:04, Ard Biesheuvel <[email protected]> wrote:
> On 26 October 2016 at 11:07, Michael Ellerman <[email protected]> wrote:
>> Hi Ard,
>>
>> I like the concept, but ...
>>
>> Ard Biesheuvel <[email protected]> writes:
>>> The symbol CRCs are emitted as ELF symbols, which allows us to easily
>>> populate the kcrctab sections by relying on the linker to associate
>>> each kcrctab slot with the correct value.
>>>
>>> This has two downsides:
>>> - given that the CRCs are treated as pointers, we waste 4 bytes for
>>> each CRC on 64 bit architectures,
>>> - on architectures that support runtime relocation, a relocation entry is
>>> emitted for each CRC value, which may take up 24 bytes of __init space
>>> (on ELF64 systems)
>>>
>>> This comes down to a x8 overhead in [uncompressed] kernel size. In addition,
>>> each relocation has to be reverted before the CRC value can be used.
>>>
>>> Switching to explicit 32 bit values on 64 bit architectures fixes both
>>> issues, since 32 bit values are not treated as relocatable quantities on
>>> ELF64 systems, even if the value ultimately resolves to a linker supplied
>>> value.
>>
>> Are we sure that part is true? ("not treated as relocatable")
>>
>
> Thanks for testing this.
>
>> A quick test build on powerpc gives me:
>>
>> WARNING: 6829 bad relocations
>> c000000000ca3748 R_PPC64_ADDR16 *ABS*+0x0000000013f53da6
>> c000000000ca374a R_PPC64_ADDR16 *ABS*+0x00000000f7272059
>> c000000000ca374c R_PPC64_ADDR16 *ABS*+0x0000000002013d36
>> c000000000ca374e R_PPC64_ADDR16 *ABS*+0x00000000a59dffc8
>> ...
>>
>> Which is coming from our relocs_check.sh script, which checks that the
>> generated relocations are ones we know how to handle.
>>
>
> OK, first of all, it appears the ppc64 assembler interprets .word
> differently than the arm64 one, so I will need to fold this in
>
> """
> diff --git a/include/linux/export.h b/include/linux/export.h
> index fa51ab2ad190..a000d421526d 100644
> --- a/include/linux/export.h
> +++ b/include/linux/export.h
> @@ -54,7 +54,7 @@ extern struct module __this_module;
> #define __CRC_SYMBOL(sym, sec) \
> asm(" .section \"___kcrctab" sec "+" #sym "\", \"a\" \n" \
> " .weak " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
> - " .word " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
> + " .long " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
> " .previous \n");
> #endif
> #else
> """
>
> With that change, the CRCs are actually emitted as
>
> WARNING: 7525 bad relocations
> c000000000ce7f50 R_PPC64_ADDR32 *ABS*+0x0000000013f53da6
> c000000000ce7f54 R_PPC64_ADDR32 *ABS*+0x0000000004f7c64e
> c000000000ce7f58 R_PPC64_ADDR32 *ABS*+0x0000000092be8a3e
>
> which is still a bit disappointing, given that we still have 7525 RELA
> entries to process.
> I tried several thing, i.e., adding -Bsymbolic to the linker command
> line, emitting the reference above as .hidden or emit the definition
> from the linker script as HIDDEN(), but nothing seems to make any
> difference. (On arm64, -Bsymbolic eliminates *all* runtime relocations
> except R_<arch>_RELATIVE ones)
>
>> And when I try to boot it I get:
>>
>> virtio: disagrees about version of symbol module_layout
>> virtio: disagrees about version of symbol module_layout
>> scsi_mod: disagrees about version of symbol module_layout
>>
>> And it can't find my root file system (unsurprisingly as it's on scsi).
>>
>
> Something like the below should fix it, I hope.
>
> """
> diff --git a/arch/powerpc/kernel/reloc_64.S b/arch/powerpc/kernel/reloc_64.S
> index d88736fbece6..99cdf2311ab5 100644
> --- a/arch/powerpc/kernel/reloc_64.S
> +++ b/arch/powerpc/kernel/reloc_64.S
> @@ -14,6 +14,7 @@
> RELA = 7
> RELACOUNT = 0x6ffffff9
> R_PPC64_RELATIVE = 22
> +R_PPC64_ADDR32 = 1
>
> /*
> * r3 = desired final address of kernel
> @@ -77,9 +78,22 @@ _GLOBAL(relocate)
> add r0,r0,r3
> stdx r0,r7,r6
> addi r9,r9,24
> - bdnz 5b
> + b 7f
> +
> + /*
> + * CRCs of exported symbols are emitted as 32-bit relocations against
> + * the *ABS* section with the CRC value recorded in the addend.
> + */
> +6: cmpdi r0,R_PPC64_ADDR32
> + bne 7f
> + ld r6,0(r9) /* reloc->r_offset */
> + ld r0,16(r9) /* reloc->r_addend */
> + stwx r0,r7,r6
> + addi r9,r9,24
> +
> +7: bdnz 5b
> + blr
>
> -6: blr
>
> .balign 8
> p_dyn: .llong __dynamic_start - 0b
> """
>
> Note that the loop no longer terminates at the first
> non-R_PPC64_RELATIVE relocation, but that seems safer to me in any
> case. It simply stores the value of r_addend at r_offset, which is the
> correct thing to do for R_PPC64_ADDR32 relocations against the *ABS*
> section, regardless of whether we are dealing with CRCs or something
> else. Note that the comparison above will fail for R_PPC64_ADDR32
> relocations against named symbols, since we compare the entire r_info
> field and not just the type (as the comment a few lines higher up
> suggests)
>
> Also a fix for relocs_check.sh:
>
> """
> diff --git a/arch/powerpc/relocs_check.sh b/arch/powerpc/relocs_check.sh
> index ec2d5c835170..2f510fbc87da 100755
> --- a/arch/powerpc/relocs_check.sh
> +++ b/arch/powerpc/relocs_check.sh
> @@ -43,7 +43,8 @@ R_PPC_ADDR16_HA
> R_PPC_RELATIVE
> R_PPC_NONE' |
> grep -E -v '\<R_PPC64_ADDR64[[:space:]]+mach_' |
> - grep -E -v '\<R_PPC64_ADDR64[[:space:]]+__crc_'
> + grep -E -v '\<R_PPC64_ADDR64[[:space:]]+__crc_' |
> + grep -E -v '\<R_PPC64_ADDR32[[:space:]]+\*ABS\*'
> )
>
> if [ -z "$bad_relocs" ]; then
> """
>
> If these changes work for PPC, I think we should fold them in.
> Hopefully, GNU ld for PPC will gain that ability to resolve absolute
> relocations at build time (like other architectures), and then the
> R_PPC64_ADDR32 handling will simply become dead code. In any case, it
> is an improvement over the mangling of CRC values to undo the runtime
> relocation, imo.
>
I have spent some more time looking into this, and it seems impossible
to coerce the powerpc linker into resolving relocations against build
time absolute constants at build time.
Interestingly, the CONFIG_MODVERSIONS + CONFIG_RELOCATABLE issue
exists on PPC32 as well, i.e., commit 0e0ed6406e61 ("powerpc/modules:
Module CRC relocation fix causes perf issues") reintroduced it on
PPC32 by making the fix PPC64 specific.
I think the fact that relocations against *ABS* symbols are converted
into R_PPC[64]_RELATIVE relocations is a linker bug. I spotted
something similar on arm64 a while ago
https://lists.gnu.org/archive/html/bug-binutils/2016-07/msg00087.html
but this has not been fixed yet either.
Having to deal with toolchain bugs when working on issues like these
is always a bit annoying, so I wonder what your take is on the PPC32
issue. If nobody cares about CONFIG_RELOCATABLE on PPC32, perhaps it
could be made mutually exclusive with CONFIG_MODVERSIONS on 32-bit
PPC.