* Re: [PATCH] module: use hidden visibility for weak symbol references
[not found] <20201027151132.14066-1-ardb@kernel.org>
@ 2020-10-27 18:27 ` Nick Desaulniers
2020-10-27 22:18 ` Fangrui Song
0 siblings, 1 reply; 2+ messages in thread
From: Nick Desaulniers @ 2020-10-27 18:27 UTC (permalink / raw)
To: Ard Biesheuvel, Fangrui Song
Cc: LKML, Linux ARM, Will Deacon, Catalin Marinas, Jessica Yu,
Kees Cook, Geert Uytterhoeven, clang-built-linux,
linux-toolchains
+ Fangrui
On Tue, Oct 27, 2020 at 8:11 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Geert reports that commit be2881824ae9eb92 ("arm64/build: Assert for
> unwanted sections") results in build errors on arm64 for configurations
> that have CONFIG_MODULES disabled.
>
> The commit in question added ASSERT()s to the arm64 linker script to
> ensure that linker generated sections such as .got, .plt etc are empty,
> but as it turns out, there are corner cases where the linker does emit
> content into those sections. More specifically, weak references to
> function symbols (which can remain unsatisfied, and can therefore not
> be emitted as relative references) will be emitted as GOT and PLT
> entries when linking the kernel in PIE mode (which is the case when
> CONFIG_RELOCATABLE is enabled, which is on by default).
>
> What happens is that code such as
>
> struct device *(*fn)(struct device *dev);
> struct device *iommu_device;
>
> fn = symbol_get(mdev_get_iommu_device);
> if (fn) {
> iommu_device = fn(dev);
>
> essentially gets converted into the following when CONFIG_MODULES is off:
>
> struct device *iommu_device;
>
> if (&mdev_get_iommu_device) {
> iommu_device = mdev_get_iommu_device(dev);
>
> where mdev_get_iommu_device is emitted as a weak symbol reference into
> the object file. The first reference is decorated with an ordinary
> ABS64 data relocation (which yields 0x0 if the reference remains
> unsatisfied). However, the indirect call is turned into a direct call
> covered by a R_AARCH64_CALL26 relocation, which is converted into a
> call via a PLT entry taking the target address from the associated
> GOT entry.
>
> Given that such GOT and PLT entries are unnecessary for fully linked
> binaries such as the kernel, let's give these weak symbol references
> hidden visibility, so that the linker knows that the weak reference
> via R_AARCH64_CALL26 can simply remain unsatisfied.
>
> Cc: Jessica Yu <jeyu@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> include/linux/module.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 7ccdf87f376f..6264617bab4d 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -740,7 +740,7 @@ static inline bool within_module(unsigned long addr, const struct module *mod)
> }
>
> /* Get/put a kernel symbol (calls should be symmetric) */
> -#define symbol_get(x) ({ extern typeof(x) x __attribute__((weak)); &(x); })
> +#define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); })
> #define symbol_put(x) do { } while (0)
> #define symbol_put_addr(x) do { } while (0)
>
> --
> 2.17.1
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] module: use hidden visibility for weak symbol references
2020-10-27 18:27 ` [PATCH] module: use hidden visibility for weak symbol references Nick Desaulniers
@ 2020-10-27 22:18 ` Fangrui Song
0 siblings, 0 replies; 2+ messages in thread
From: Fangrui Song @ 2020-10-27 22:18 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Nick Desaulniers, LKML, Linux ARM, Will Deacon, Catalin Marinas,
Jessica Yu, Kees Cook, Geert Uytterhoeven, clang-built-linux,
linux-toolchains
One nit about ".got" in the message:
Reviewed-by: Fangrui Song <maskray@google.com>
On 2020-10-27, Nick Desaulniers wrote:
>+ Fangrui
>
>On Tue, Oct 27, 2020 at 8:11 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> Geert reports that commit be2881824ae9eb92 ("arm64/build: Assert for
>> unwanted sections") results in build errors on arm64 for configurations
>> that have CONFIG_MODULES disabled.
>>
>> The commit in question added ASSERT()s to the arm64 linker script to
>> ensure that linker generated sections such as .got, .plt etc are empty,
.got -> .got.plt
be2881824ae9eb92 does not ASSERT on .got (it can).
Strangely *(.got) is placed in .text in arch/arm64/kernel/vmlinux.lds.S
I think that line can be removed. On x86, aarch64 and many other archs,
the start of .got.plt is the GOT base. .got is not needed (ppc/arm/riscv
use .got instead of .got.plt as the GOT base anchor).
>> but as it turns out, there are corner cases where the linker does emit
>> content into those sections. More specifically, weak references to
>> function symbols (which can remain unsatisfied, and can therefore not
>> be emitted as relative references) will be emitted as GOT and PLT
>> entries when linking the kernel in PIE mode (which is the case when
>> CONFIG_RELOCATABLE is enabled, which is on by default).
Confirmed.
>> What happens is that code such as
>>
>> struct device *(*fn)(struct device *dev);
>> struct device *iommu_device;
>>
>> fn = symbol_get(mdev_get_iommu_device);
>> if (fn) {
>> iommu_device = fn(dev);
>>
>> essentially gets converted into the following when CONFIG_MODULES is off:
>>
>> struct device *iommu_device;
>>
>> if (&mdev_get_iommu_device) {
>> iommu_device = mdev_get_iommu_device(dev);
>>
>> where mdev_get_iommu_device is emitted as a weak symbol reference into
>> the object file. The first reference is decorated with an ordinary
>> ABS64 data relocation (which yields 0x0 if the reference remains
>> unsatisfied). However, the indirect call is turned into a direct call
>> covered by a R_AARCH64_CALL26 relocation, which is converted into a
>> call via a PLT entry taking the target address from the associated
>> GOT entry.
Yes, the R_AARCH64_CALL26 relocation referencing an undefined weak
symbol causes one .plt entry and one .got.plt entry.
>> Given that such GOT and PLT entries are unnecessary for fully linked
>> binaries such as the kernel, let's give these weak symbol references
>> hidden visibility, so that the linker knows that the weak reference
>> via R_AARCH64_CALL26 can simply remain unsatisfied.
>>
>> Cc: Jessica Yu <jeyu@kernel.org>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>> Cc: Nick Desaulniers <ndesaulniers@google.com>
>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>> ---
>> include/linux/module.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index 7ccdf87f376f..6264617bab4d 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -740,7 +740,7 @@ static inline bool within_module(unsigned long addr, const struct module *mod)
>> }
>>
>> /* Get/put a kernel symbol (calls should be symmetric) */
>> -#define symbol_get(x) ({ extern typeof(x) x __attribute__((weak)); &(x); })
>> +#define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); })
>> #define symbol_put(x) do { } while (0)
>> #define symbol_put_addr(x) do { } while (0)
>>
>> --
>> 2.17.1
>>
>
>
>--
>Thanks,
>~Nick Desaulniers
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-10-27 22:18 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20201027151132.14066-1-ardb@kernel.org>
2020-10-27 18:27 ` [PATCH] module: use hidden visibility for weak symbol references Nick Desaulniers
2020-10-27 22:18 ` Fangrui Song
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).