| Message ID | 20201027151132.14066-1-ardb@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | module: use hidden visibility for weak symbol references | expand |
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
+ 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 >
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
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
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!")
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
+++ 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
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
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,
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)
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(-)