[PATCH] module: use hidden visibility for weak symbol references

[PATCH] module: use hidden visibility for weak symbol references

[Ard Biesheuvel]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] module: use hidden visibility for weak symbol references

[Geert Uytterhoeven]

On Tue, Oct 27, 2020 at 4:11 PM 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>

Thanks, this get rids of

    aarch64-linux-gnu-ld: Unexpected GOT/PLT entries detected!
    aarch64-linux-gnu-ld: Unexpected run-time procedure linkages detected!

which you may want to mention in the patch description, to make
it easier to be found.

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] module: use hidden visibility for weak symbol references

[Nick Desaulniers]

+ 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] module: use hidden visibility for weak symbol references

[Fangrui Song]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] module: use hidden visibility for weak symbol references

[Will Deacon]

Hi Ard,

On Tue, Oct 27, 2020 at 04:11:32PM +0100, Ard Biesheuvel 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(-)

Cheers. I gave this a spin, but I unfortunately still see the following
linker warning with allnoconfig:

  aarch64-linux-gnu-ld: warning: orphan section `.igot.plt' from `arch/arm64/kernel/head.o' being placed in section `.igot.plt'

which looks unrelated to symbol_get(), but maybe it's worth knocking these
things on the head (no pun intended) at the same time?

Cheers,

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] module: use hidden visibility for weak symbol references

[Ard Biesheuvel]

On Wed, 28 Oct 2020 at 11:00, Will Deacon <will@kernel.org> wrote:
>
> Hi Ard,
>
> On Tue, Oct 27, 2020 at 04:11:32PM +0100, Ard Biesheuvel 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(-)
>
> Cheers. I gave this a spin, but I unfortunately still see the following
> linker warning with allnoconfig:
>
>   aarch64-linux-gnu-ld: warning: orphan section `.igot.plt' from `arch/arm64/kernel/head.o' being placed in section `.igot.plt'
>
> which looks unrelated to symbol_get(), but maybe it's worth knocking these
> things on the head (no pun intended) at the same time?
>

Yeah, that is just one of those spurious sections that turns up empty
anyway. The head.o is a red herring, it is simply the first file
appearing in the link.

This should fix it

diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 6567d80dd15f..48b222f1c700 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -278,7 +278,7 @@ SECTIONS
         * explicitly check instead of blindly discarding.
         */
        .plt : {
-               *(.plt) *(.plt.*) *(.iplt) *(.igot)
+               *(.plt) *(.plt.*) *(.iplt) *(.igot .igot.plt)
        }
        ASSERT(SIZEOF(.plt) == 0, "Unexpected run-time procedure
linkages detected!")

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] module: use hidden visibility for weak symbol references

[Will Deacon]

On Wed, Oct 28, 2020 at 01:27:01PM +0100, Ard Biesheuvel wrote:
> On Wed, 28 Oct 2020 at 11:00, Will Deacon <will@kernel.org> wrote:
> > On Tue, Oct 27, 2020 at 04:11:32PM +0100, Ard Biesheuvel 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(-)
> >
> > Cheers. I gave this a spin, but I unfortunately still see the following
> > linker warning with allnoconfig:
> >
> >   aarch64-linux-gnu-ld: warning: orphan section `.igot.plt' from `arch/arm64/kernel/head.o' being placed in section `.igot.plt'
> >
> > which looks unrelated to symbol_get(), but maybe it's worth knocking these
> > things on the head (no pun intended) at the same time?
> >
> 
> Yeah, that is just one of those spurious sections that turns up empty
> anyway. The head.o is a red herring, it is simply the first file
> appearing in the link.
> 
> This should fix it
> 
> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> index 6567d80dd15f..48b222f1c700 100644
> --- a/arch/arm64/kernel/vmlinux.lds.S
> +++ b/arch/arm64/kernel/vmlinux.lds.S
> @@ -278,7 +278,7 @@ SECTIONS
>          * explicitly check instead of blindly discarding.
>          */
>         .plt : {
> -               *(.plt) *(.plt.*) *(.iplt) *(.igot)
> +               *(.plt) *(.plt.*) *(.iplt) *(.igot .igot.plt)
>         }
>         ASSERT(SIZEOF(.plt) == 0, "Unexpected run-time procedure
> linkages detected!")

Cheers, that fixes the extra warning for me. If you could send a proper
patch, I'm happy to queue as an arm64 fix! (I'm assuming the former is going
via Jessica, but I can also take that with her Ack).

Will


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] module: use hidden visibility for weak symbol references

[Jessica Yu]

+++ Will Deacon [28/10/20 13:24 +0000]:
>On Wed, Oct 28, 2020 at 01:27:01PM +0100, Ard Biesheuvel wrote:
>> On Wed, 28 Oct 2020 at 11:00, Will Deacon <will@kernel.org> wrote:
>> > On Tue, Oct 27, 2020 at 04:11:32PM +0100, Ard Biesheuvel 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(-)
>> >
>> > Cheers. I gave this a spin, but I unfortunately still see the following
>> > linker warning with allnoconfig:
>> >
>> >   aarch64-linux-gnu-ld: warning: orphan section `.igot.plt' from `arch/arm64/kernel/head.o' being placed in section `.igot.plt'
>> >
>> > which looks unrelated to symbol_get(), but maybe it's worth knocking these
>> > things on the head (no pun intended) at the same time?
>> >
>>
>> Yeah, that is just one of those spurious sections that turns up empty
>> anyway. The head.o is a red herring, it is simply the first file
>> appearing in the link.
>>
>> This should fix it
>>
>> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
>> index 6567d80dd15f..48b222f1c700 100644
>> --- a/arch/arm64/kernel/vmlinux.lds.S
>> +++ b/arch/arm64/kernel/vmlinux.lds.S
>> @@ -278,7 +278,7 @@ SECTIONS
>>          * explicitly check instead of blindly discarding.
>>          */
>>         .plt : {
>> -               *(.plt) *(.plt.*) *(.iplt) *(.igot)
>> +               *(.plt) *(.plt.*) *(.iplt) *(.igot .igot.plt)
>>         }
>>         ASSERT(SIZEOF(.plt) == 0, "Unexpected run-time procedure
>> linkages detected!")
>
>Cheers, that fixes the extra warning for me. If you could send a proper
>patch, I'm happy to queue as an arm64 fix! (I'm assuming the former is going
>via Jessica, but I can also take that with her Ack).

Hi! Yes, please feel free to take this patch along with the other fix:

Acked-by: Jessica Yu <jeyu@kernel.org>

Thanks,

Jessica


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] module: use hidden visibility for weak symbol references

[Will Deacon]

On Wed, Oct 28, 2020 at 03:03:44PM +0100, Jessica Yu wrote:
> +++ Will Deacon [28/10/20 13:24 +0000]:
> > On Wed, Oct 28, 2020 at 01:27:01PM +0100, Ard Biesheuvel wrote:
> > > On Wed, 28 Oct 2020 at 11:00, Will Deacon <will@kernel.org> wrote:
> > > > On Tue, Oct 27, 2020 at 04:11:32PM +0100, Ard Biesheuvel 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(-)
> > > >
> > > > Cheers. I gave this a spin, but I unfortunately still see the following
> > > > linker warning with allnoconfig:
> > > >
> > > >   aarch64-linux-gnu-ld: warning: orphan section `.igot.plt' from `arch/arm64/kernel/head.o' being placed in section `.igot.plt'
> > > >
> > > > which looks unrelated to symbol_get(), but maybe it's worth knocking these
> > > > things on the head (no pun intended) at the same time?
> > > >
> > > 
> > > Yeah, that is just one of those spurious sections that turns up empty
> > > anyway. The head.o is a red herring, it is simply the first file
> > > appearing in the link.
> > > 
> > > This should fix it
> > > 
> > > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> > > index 6567d80dd15f..48b222f1c700 100644
> > > --- a/arch/arm64/kernel/vmlinux.lds.S
> > > +++ b/arch/arm64/kernel/vmlinux.lds.S
> > > @@ -278,7 +278,7 @@ SECTIONS
> > >          * explicitly check instead of blindly discarding.
> > >          */
> > >         .plt : {
> > > -               *(.plt) *(.plt.*) *(.iplt) *(.igot)
> > > +               *(.plt) *(.plt.*) *(.iplt) *(.igot .igot.plt)
> > >         }
> > >         ASSERT(SIZEOF(.plt) == 0, "Unexpected run-time procedure
> > > linkages detected!")
> > 
> > Cheers, that fixes the extra warning for me. If you could send a proper
> > patch, I'm happy to queue as an arm64 fix! (I'm assuming the former is going
> > via Jessica, but I can also take that with her Ack).
> 
> Hi! Yes, please feel free to take this patch along with the other fix:
> 
> Acked-by: Jessica Yu <jeyu@kernel.org>

Cheers, Jessica -- I'll queue them in a sec!

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] module: use hidden visibility for weak symbol references

[Will Deacon]

On Tue, 27 Oct 2020 16:11:32 +0100, Ard Biesheuvel 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).
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] module: use hidden visibility for weak symbol references
      https://git.kernel.org/arm64/c/13150bc5416f

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel