diff mbox series

[powerpc-next] Fix powerpc64 alignment of .toc section in kernel modules

Message ID 20171206191228.30830-1-desnesn@linux.vnet.ibm.com (mailing list archive)
State Accepted
Commit 5c45b5280196a92c4437f5648209c5bd3f08e882
Headers show
Series [powerpc-next] Fix powerpc64 alignment of .toc section in kernel modules | expand

Commit Message

Desnes A. Nunes do Rosario Dec. 6, 2017, 7:12 p.m. UTC 
powerpc64 gcc can generate code that offsets an address, to access part of
an object in memory. If the address is a -mcmodel=medium toc pointer
relative address then code like the following is possible.

 addis r9,r2,var@toc@ha
 ld r3,var@toc@l(r9)
 ld r4,(var+8)@toc@l(r9)

This works fine so long as var is naturally aligned, *and* r2 is
sufficiently aligned. If not, there is a possibility that the offset added
to access var+8 wraps over a n*64k+32k boundary. Modules don't have any
guarantee that r2 is sufficiently aligned. Moreover, code generated by
older compilers generates a .toc section with 2**0 alignment, which can
result in relocation failures at module load time even without the wrap
problem.

Thus, this patch links modules with an aligned .toc section (Makefile and
module.lds changes), and forces alignment for out of tree modules or those
without a .toc section (module_64.c changes).

Signed-off-by: Alan Modra <amodra@gmail.com>
[ desnesn: updated patch to apply to powerpc-next kernel v4.15 ]
Signed-off-by: Desnes A. Nunes do Rosario <desnesn@linux.vnet.ibm.com>
---
 arch/powerpc/Makefile           |  1 +
 arch/powerpc/kernel/module.lds  |  8 ++++++++
 arch/powerpc/kernel/module_64.c | 13 +++++++++----
 3 files changed, 18 insertions(+), 4 deletions(-)
 create mode 100644 arch/powerpc/kernel/module.lds

Comments

Michael Ellerman Dec. 7, 2017, 12:25 p.m. UTC | #1 
Hi Desnes,

Am I right that Alan largely wrote this patch?

If so it should probably be From: him, so that he is the author in the
git log.


Desnes Augusto Nunes do Rosario <desnesn@linux.vnet.ibm.com> writes:
> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> index 1381693..c472f5b 100644
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -63,6 +63,7 @@ UTS_MACHINE := $(subst $(space),,$(machine-y))
>  ifdef CONFIG_PPC32
>  KBUILD_LDFLAGS_MODULE += arch/powerpc/lib/crtsavres.o
>  else
> +KBUILD_LDFLAGS_MODULE += -T arch/powerpc/kernel/module.lds

This needs to be:

KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/powerpc/kernel/module.lds

Otherwise building with O=../build fails with:

  ld: cannot open linker script file arch/powerpc/kernel/module.lds: No such file or directory

I'll fix it up.

> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 759104b..9b2c5c1 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -374,11 +377,13 @@ int module_frob_arch_sections(Elf64_Ehdr *hdr,
>  }
>  
>  /* r2 is the TOC pointer: it actually points 0x8000 into the TOC (this
> -   gives the value maximum span in an instruction which uses a signed
> -   offset) */
> + * gives the value maximum span in an instruction which uses a signed
> + * offset).  Round down to a 256 byte boundary for the odd case where
> + * we are setting up r2 without a .toc section.
> + */
>  static inline unsigned long my_r2(const Elf64_Shdr *sechdrs, struct module *me)
>  {
> -	return sechdrs[me->arch.toc_section].sh_addr + 0x8000;
> +	return (sechdrs[me->arch.toc_section].sh_addr & -256) + 0x8000;

I think it's more typical in the kernel to write -256 as ~0xff.

Again I can fix it up.

cheers
Desnes A. Nunes do Rosario Dec. 7, 2017, 1:24 p.m. UTC | #2 
Hello Michael,

On 12/07/2017 10:25 AM, Michael Ellerman wrote:
> Hi Desnes,
> 
> Am I right that Alan largely wrote this patch?
> 
> If so it should probably be From: him, so that he is the author in the
> git log.

Yes, Alan Modra is the main author and I am just committing it with 
minor changes. Thus, the author change is necessary.

> 
> 
> Desnes Augusto Nunes do Rosario <desnesn@linux.vnet.ibm.com> writes:
>> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
>> index 1381693..c472f5b 100644
>> --- a/arch/powerpc/Makefile
>> +++ b/arch/powerpc/Makefile
>> @@ -63,6 +63,7 @@ UTS_MACHINE := $(subst $(space),,$(machine-y))
>>   ifdef CONFIG_PPC32
>>   KBUILD_LDFLAGS_MODULE += arch/powerpc/lib/crtsavres.o
>>   else
>> +KBUILD_LDFLAGS_MODULE += -T arch/powerpc/kernel/module.lds
> 
> This needs to be:
> 
> KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/powerpc/kernel/module.lds
> 
> Otherwise building with O=../build fails with:
> 
>    ld: cannot open linker script file arch/powerpc/kernel/module.lds: No such file or directory
> 
> I'll fix it up.

Indeed; this change is necessary to avoid any path errors.

> 
>> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
>> index 759104b..9b2c5c1 100644
>> --- a/arch/powerpc/kernel/module_64.c
>> +++ b/arch/powerpc/kernel/module_64.c
>> @@ -374,11 +377,13 @@ int module_frob_arch_sections(Elf64_Ehdr *hdr,
>>   }
>>   
>>   /* r2 is the TOC pointer: it actually points 0x8000 into the TOC (this
>> -   gives the value maximum span in an instruction which uses a signed
>> -   offset) */
>> + * gives the value maximum span in an instruction which uses a signed
>> + * offset).  Round down to a 256 byte boundary for the odd case where
>> + * we are setting up r2 without a .toc section.
>> + */
>>   static inline unsigned long my_r2(const Elf64_Shdr *sechdrs, struct module *me)
>>   {
>> -	return sechdrs[me->arch.toc_section].sh_addr + 0x8000;
>> +	return (sechdrs[me->arch.toc_section].sh_addr & -256) + 0x8000;
> 
> I think it's more typical in the kernel to write -256 as ~0xff.
> 
> Again I can fix it up.

Good to know!

> 
> cheers
> 

Lastly, will you fix it up or do you want me to send a second version 
then? Whatever is best for you.

Thank you for the review.
Michael Ellerman Dec. 8, 2017, 11:25 a.m. UTC | #3 
Desnes Augusto Nunes do Rosário <desnesn@linux.vnet.ibm.com> writes:
...
>
> Lastly, will you fix it up or do you want me to send a second version 
> then? Whatever is best for you.

No that's fine I've done those changes, no need to send another version.

cheers
Michael Ellerman Dec. 12, 2017, 11:39 a.m. UTC | #4 
On Wed, 2017-12-06 at 19:12:28 UTC, Desnes Augusto Nunes do Rosario wrote:
> powerpc64 gcc can generate code that offsets an address, to access part of
> an object in memory. If the address is a -mcmodel=medium toc pointer
> relative address then code like the following is possible.
> 
>  addis r9,r2,var@toc@ha
>  ld r3,var@toc@l(r9)
>  ld r4,(var+8)@toc@l(r9)
> 
> This works fine so long as var is naturally aligned, *and* r2 is
> sufficiently aligned. If not, there is a possibility that the offset added
> to access var+8 wraps over a n*64k+32k boundary. Modules don't have any
> guarantee that r2 is sufficiently aligned. Moreover, code generated by
> older compilers generates a .toc section with 2**0 alignment, which can
> result in relocation failures at module load time even without the wrap
> problem.
> 
> Thus, this patch links modules with an aligned .toc section (Makefile and
> module.lds changes), and forces alignment for out of tree modules or those
> without a .toc section (module_64.c changes).
> 
> Signed-off-by: Alan Modra <amodra@gmail.com>
> [ desnesn: updated patch to apply to powerpc-next kernel v4.15 ]
> Signed-off-by: Desnes A. Nunes do Rosario <desnesn@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/5c45b5280196a92c4437f5648209c5

cheers
diff mbox series

Patch

diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 1381693..c472f5b 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -63,6 +63,7 @@  UTS_MACHINE := $(subst $(space),,$(machine-y))
 ifdef CONFIG_PPC32
 KBUILD_LDFLAGS_MODULE += arch/powerpc/lib/crtsavres.o
 else
+KBUILD_LDFLAGS_MODULE += -T arch/powerpc/kernel/module.lds
 ifeq ($(call ld-ifversion, -ge, 225000000, y),y)
 # Have the linker provide sfpr if possible.
 # There is a corresponding test in arch/powerpc/lib/Makefile
diff --git a/arch/powerpc/kernel/module.lds b/arch/powerpc/kernel/module.lds
new file mode 100644
index 0000000..cea5dc1
--- /dev/null
+++ b/arch/powerpc/kernel/module.lds
@@ -0,0 +1,8 @@ 
+/* Force alignment of .toc section.  */
+SECTIONS
+{
+	.toc 0 : ALIGN(256)
+	{
+		*(.got .toc)
+	}
+}
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 759104b..9b2c5c1 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -339,8 +339,11 @@  int module_frob_arch_sections(Elf64_Ehdr *hdr,
 		char *p;
 		if (strcmp(secstrings + sechdrs[i].sh_name, ".stubs") == 0)
 			me->arch.stubs_section = i;
-		else if (strcmp(secstrings + sechdrs[i].sh_name, ".toc") == 0)
+		else if (strcmp(secstrings + sechdrs[i].sh_name, ".toc") == 0) {
 			me->arch.toc_section = i;
+			if (sechdrs[i].sh_addralign < 8)
+				sechdrs[i].sh_addralign = 8;
+		}
 		else if (strcmp(secstrings+sechdrs[i].sh_name,"__versions")==0)
 			dedotify_versions((void *)hdr + sechdrs[i].sh_offset,
 					  sechdrs[i].sh_size);
@@ -374,11 +377,13 @@  int module_frob_arch_sections(Elf64_Ehdr *hdr,
 }
 
 /* r2 is the TOC pointer: it actually points 0x8000 into the TOC (this
-   gives the value maximum span in an instruction which uses a signed
-   offset) */
+ * gives the value maximum span in an instruction which uses a signed
+ * offset).  Round down to a 256 byte boundary for the odd case where
+ * we are setting up r2 without a .toc section.
+ */
 static inline unsigned long my_r2(const Elf64_Shdr *sechdrs, struct module *me)
 {
-	return sechdrs[me->arch.toc_section].sh_addr + 0x8000;
+	return (sechdrs[me->arch.toc_section].sh_addr & -256) + 0x8000;
 }
 
 /* Both low and high 16 bits are added as SIGNED additions, so if low