| Message ID | 20171227085033.22389-3-ard.biesheuvel@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add support for relative references in special sections | expand |
On Wed, Dec 27, 2017 at 12:50 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > index 52e611ab9a6c..fe752d365334 100644 > --- a/include/linux/compiler.h > +++ b/include/linux/compiler.h > @@ -327,4 +327,15 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s > compiletime_assert(__native_word(t), \ > "Need native word sized stores/loads for atomicity.") > > +/* > + * Force the compiler to emit 'sym' as a symbol, so that we can reference > + * it from inline assembler. Necessary in case 'sym' could be inlined > + * otherwise, or eliminated entirely due to lack of references that are > + * visibile to the compiler. > + */ > +#define __ADDRESSABLE(sym) \ > + static void *__attribute__((section(".discard.text"), used)) \ > + __PASTE(__discard_##sym, __LINE__)(void) \ > + { return (void *)&sym; } \ > + > #endif /* __LINUX_COMPILER_H */ Isn't this logically the point where you should add the arm64 vmlinux.lds.S change, and explain how ".discard.text" turns into ".init.discard.text" for some odd arm64 reason? Linus
On 27 December 2017 at 20:07, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Wed, Dec 27, 2017 at 12:50 AM, Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: >> diff --git a/include/linux/compiler.h b/include/linux/compiler.h >> index 52e611ab9a6c..fe752d365334 100644 >> --- a/include/linux/compiler.h >> +++ b/include/linux/compiler.h >> @@ -327,4 +327,15 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s >> compiletime_assert(__native_word(t), \ >> "Need native word sized stores/loads for atomicity.") >> >> +/* >> + * Force the compiler to emit 'sym' as a symbol, so that we can reference >> + * it from inline assembler. Necessary in case 'sym' could be inlined >> + * otherwise, or eliminated entirely due to lack of references that are >> + * visibile to the compiler. >> + */ >> +#define __ADDRESSABLE(sym) \ >> + static void *__attribute__((section(".discard.text"), used)) \ >> + __PASTE(__discard_##sym, __LINE__)(void) \ >> + { return (void *)&sym; } \ >> + >> #endif /* __LINUX_COMPILER_H */ > > Isn't this logically the point where you should add the arm64 > vmlinux.lds.S change, and explain how ".discard.text" turns into > ".init.discard.text" for some odd arm64 reason? > I tried to keep the generic patches generic, so perhaps I should just put the arm64 vmlinux.lds.S change in a patch on its own?
On Wed, Dec 27, 2017 at 12:11 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > I tried to keep the generic patches generic, so perhaps I should just > put the arm64 vmlinux.lds.S change in a patch on its own? I guess it doesn't matter, but regardless of where it gets introduced I would like to see the explanation for where the heck that magical ".init.discard.text" comes from. It's definitely not obvious from the patches, and is presumably some odd arm64 special case. Linus
On 27 December 2017 at 20:13, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Wed, Dec 27, 2017 at 12:11 PM, Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: >> >> I tried to keep the generic patches generic, so perhaps I should just >> put the arm64 vmlinux.lds.S change in a patch on its own? > > I guess it doesn't matter, but regardless of where it gets introduced > I would like to see the explanation for where the heck that magical > ".init.discard.text" comes from. It's definitely not obvious from the > patches, and is presumably some odd arm64 special case. > This has to do with the EFI stub. x86 and ARM link it into the decompressor, and so the code and data are not annotated as __init (and doing so would involve modifying a lot of code). arm64 does not have a decompressor, and so the EFI stub is linked into the kernel proper. To make sure the code ends up in the .init segment, all sections are prepended with .init at the object level, using objcopy. Annoyingly, we need this because there is a single instance of a special section that ends up in the EFI stub code: we build lib/sort.c again as a EFI libstub object, and given that sort() is exported, we end up with a ksymtab section in the EFI stub. The sort() thing has caused issues before [0], so perhaps I should just clone sort.c into drivers/firmware/efi/libstub and get rid of that hack. [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=29f9007b3182ab3f328a31da13e6b1c9072f7a95
* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > Annoyingly, we need this because there is a single instance of a > special section that ends up in the EFI stub code: we build lib/sort.c > again as a EFI libstub object, and given that sort() is exported, we > end up with a ksymtab section in the EFI stub. The sort() thing has > caused issues before [0], so perhaps I should just clone sort.c into > drivers/firmware/efi/libstub and get rid of that hack. > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=29f9007b3182ab3f328a31da13e6b1c9072f7a95 If the root problem is early bootstrap code randomly using generic facility that isn't __init, then we should definitely improve tooling to at least detect these problems. As bootstrap code gets improved (KASLR, more complex decompression, etc. etc.) we keep using new bits of generic facilities... So this should definitely not be hidden by open coding that function (which has various other disadvantages as well), but should be turned from silent breakage either into non-breakage (and do so not only for sort() but for other generic functions as well), or should be turned into a build failure. Thanks, Ingo
On 28 December 2017 at 12:05, Ingo Molnar <mingo@kernel.org> wrote: > > * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> Annoyingly, we need this because there is a single instance of a >> special section that ends up in the EFI stub code: we build lib/sort.c >> again as a EFI libstub object, and given that sort() is exported, we >> end up with a ksymtab section in the EFI stub. The sort() thing has >> caused issues before [0], so perhaps I should just clone sort.c into >> drivers/firmware/efi/libstub and get rid of that hack. >> >> [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=29f9007b3182ab3f328a31da13e6b1c9072f7a95 > > If the root problem is early bootstrap code randomly using generic facility that > isn't __init, then we should definitely improve tooling to at least detect these > problems. > > As bootstrap code gets improved (KASLR, more complex decompression, etc. etc.) we > keep using new bits of generic facilities... > > So this should definitely not be hidden by open coding that function (which has > various other disadvantages as well), but should be turned from silent breakage > either into non-breakage (and do so not only for sort() but for other generic > functions as well), or should be turned into a build failure. > We already have safeguards in place to ensure that the arm64 EFI stub (which is essentially the same executable as the kernel proper) only pulls in code that has been made available to it explicitly. That is why sort.c is recompiled for the EFI stub, as well as all other C code that is shared between the stub and the kernel. We also have a build time check to ensure that the resulting code does not rely on absolute symbol references, which will be invalid in the UEFI execution context. So the only problem is that unneeded ksymtab/kcrctab sections, which affected ARM for obscure reasons; typically, they just take up some space. On x86, the kaslr code deals with a similar issue by #define'ing _LINUX_EXPORT_H before including linux/export.h, which also gets rid of these sections, but I was a bit reluctant to copy that pattern. Perhaps we should enhance linux/export.h for reasons such as these by adding a macro that nops out EXPORT_SYMBOL() declarations?
Hi Ard,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.15-rc5 next-20171222]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ard-Biesheuvel/add-support-for-relative-references-in-special-sections/20171228-171634
config: s390-gcov_defconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=s390
All errors (new ones prefixed by >>):
>> arch/s390/kernel/ebcdic.o:(.data+0x118): undefined reference to `__gcov_merge_add'
arch/s390/kernel/ebcdic.o: In function `_GLOBAL__sub_I_00100_0__ascebc':
>> ebcdic.c:(.text.startup+0xe): undefined reference to `__gcov_init'
arch/s390/kernel/ebcdic.o: In function `_GLOBAL__sub_D_00100_1__ascebc':
>> ebcdic.c:(.text.exit+0x8): undefined reference to `__gcov_exit'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/arch/x86/include/asm/Kbuild b/arch/x86/include/asm/Kbuild index 5d6a53fd7521..3e8a88dcaa1d 100644 --- a/arch/x86/include/asm/Kbuild +++ b/arch/x86/include/asm/Kbuild @@ -9,5 +9,6 @@ generated-y += xen-hypercalls.h generic-y += clkdev.h generic-y += dma-contiguous.h generic-y += early_ioremap.h +generic-y += export.h generic-y += mcs_spinlock.h generic-y += mm-arch-hooks.h diff --git a/arch/x86/include/asm/export.h b/arch/x86/include/asm/export.h deleted file mode 100644 index 2a51d66689c5..000000000000 --- a/arch/x86/include/asm/export.h +++ /dev/null @@ -1,5 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0 */ -#ifdef CONFIG_64BIT -#define KSYM_ALIGN 16 -#endif -#include <asm-generic/export.h> diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h index 719db1968d81..97ce606459ae 100644 --- a/include/asm-generic/export.h +++ b/include/asm-generic/export.h @@ -5,12 +5,10 @@ #define KSYM_FUNC(x) x #endif #ifdef CONFIG_64BIT -#define __put .quad #ifndef KSYM_ALIGN #define KSYM_ALIGN 8 #endif #else -#define __put .long #ifndef KSYM_ALIGN #define KSYM_ALIGN 4 #endif @@ -25,6 +23,16 @@ #define KSYM(name) name #endif +.macro __put, val, name +#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS + .long \val - ., \name - . +#elif defined(CONFIG_64BIT) + .quad \val, \name +#else + .long \val, \name +#endif +.endm + /* * note on .section use: @progbits vs %progbits nastiness doesn't matter, * since we immediately emit into those sections anyway. diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 52e611ab9a6c..fe752d365334 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -327,4 +327,15 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s compiletime_assert(__native_word(t), \ "Need native word sized stores/loads for atomicity.") +/* + * Force the compiler to emit 'sym' as a symbol, so that we can reference + * it from inline assembler. Necessary in case 'sym' could be inlined + * otherwise, or eliminated entirely due to lack of references that are + * visibile to the compiler. + */ +#define __ADDRESSABLE(sym) \ + static void *__attribute__((section(".discard.text"), used)) \ + __PASTE(__discard_##sym, __LINE__)(void) \ + { return (void *)&sym; } \ + #endif /* __LINUX_COMPILER_H */ diff --git a/include/linux/export.h b/include/linux/export.h index 1a1dfdb2a5c6..5112d0c41512 100644 --- a/include/linux/export.h +++ b/include/linux/export.h @@ -24,12 +24,6 @@ #define VMLINUX_SYMBOL_STR(x) __VMLINUX_SYMBOL_STR(x) #ifndef __ASSEMBLY__ -struct kernel_symbol -{ - unsigned long value; - const char *name; -}; - #ifdef MODULE extern struct module __this_module; #define THIS_MODULE (&__this_module) @@ -60,17 +54,47 @@ extern struct module __this_module; #define __CRC_SYMBOL(sym, sec) #endif +#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS +#include <linux/compiler.h> +/* + * Emit the ksymtab entry as a pair of relative references: this reduces + * the size by half on 64-bit architectures, and eliminates the need for + * absolute relocations that require runtime processing on relocatable + * kernels. + */ +#define __KSYMTAB_ENTRY(sym, sec) \ + __ADDRESSABLE(sym) \ + asm(" .section \"___ksymtab" sec "+" #sym "\", \"a\" \n" \ + " .balign 8 \n" \ + VMLINUX_SYMBOL_STR(__ksymtab_##sym) ": \n" \ + " .long " VMLINUX_SYMBOL_STR(sym) "- . \n" \ + " .long " VMLINUX_SYMBOL_STR(__kstrtab_##sym) "- .\n" \ + " .previous \n") + +struct kernel_symbol { + signed int value_offset; + signed int name_offset; +}; +#else +#define __KSYMTAB_ENTRY(sym, sec) \ + static const struct kernel_symbol __ksymtab_##sym \ + __attribute__((section("___ksymtab" sec "+" #sym), used)) \ + = { (unsigned long)&sym, __kstrtab_##sym } + +struct kernel_symbol { + unsigned long value; + const char *name; +}; +#endif + /* For every exported symbol, place a struct in the __ksymtab section */ #define ___EXPORT_SYMBOL(sym, sec) \ extern typeof(sym) sym; \ __CRC_SYMBOL(sym, sec) \ static const char __kstrtab_##sym[] \ - __attribute__((section("__ksymtab_strings"), aligned(1))) \ + __attribute__((section("__ksymtab_strings"), used, aligned(1))) \ = VMLINUX_SYMBOL_STR(sym); \ - static const struct kernel_symbol __ksymtab_##sym \ - __used \ - __attribute__((section("___ksymtab" sec "+" #sym), used)) \ - = { (unsigned long)&sym, __kstrtab_##sym } + __KSYMTAB_ENTRY(sym, sec) #if defined(__KSYM_DEPS__) diff --git a/kernel/module.c b/kernel/module.c index dea01ac9cb74..d3a908ffc42c 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -549,12 +549,31 @@ static bool check_symbol(const struct symsearch *syms, return true; } +static unsigned long kernel_symbol_value(const struct kernel_symbol *sym) +{ +#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS + return (unsigned long)&sym->value_offset + sym->value_offset; +#else + return sym->value; +#endif +} + +static const char *kernel_symbol_name(const struct kernel_symbol *sym) +{ +#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS + return (const char *)((unsigned long)&sym->name_offset + + sym->name_offset); +#else + return sym->name; +#endif +} + static int cmp_name(const void *va, const void *vb) { const char *a; const struct kernel_symbol *b; a = va; b = vb; - return strcmp(a, b->name); + return strcmp(a, kernel_symbol_name(b)); } static bool find_symbol_in_section(const struct symsearch *syms, @@ -2198,7 +2217,7 @@ void *__symbol_get(const char *symbol) sym = NULL; preempt_enable(); - return sym ? (void *)sym->value : NULL; + return sym ? (void *)kernel_symbol_value(sym) : NULL; } EXPORT_SYMBOL_GPL(__symbol_get); @@ -2228,10 +2247,12 @@ static int verify_export_symbols(struct module *mod) for (i = 0; i < ARRAY_SIZE(arr); i++) { for (s = arr[i].sym; s < arr[i].sym + arr[i].num; s++) { - if (find_symbol(s->name, &owner, NULL, true, false)) { + if (find_symbol(kernel_symbol_name(s), &owner, NULL, + true, false)) { pr_err("%s: exports duplicate symbol %s" " (owned by %s)\n", - mod->name, s->name, module_name(owner)); + mod->name, kernel_symbol_name(s), + module_name(owner)); return -ENOEXEC; } } @@ -2280,7 +2301,7 @@ static int simplify_symbols(struct module *mod, const struct load_info *info) ksym = resolve_symbol_wait(mod, info, name); /* Ok if resolved. */ if (ksym && !IS_ERR(ksym)) { - sym[i].st_value = ksym->value; + sym[i].st_value = kernel_symbol_value(ksym); break; } @@ -2540,7 +2561,7 @@ static int is_exported(const char *name, unsigned long value, ks = lookup_symbol(name, __start___ksymtab, __stop___ksymtab); else ks = lookup_symbol(name, mod->syms, mod->syms + mod->num_syms); - return ks != NULL && ks->value == value; + return ks != NULL && kernel_symbol_value(ks) == value; } /* As per nm */