Re: [PATCH v2] modversions: treat symbol CRCs as 32 bit quantities on 64 bit archs

[Ard Biesheuvel]

On 26 October 2016 at 14:04, Ard Biesheuvel  wrote:
> On 26 October 2016 at 11:07, Michael Ellerman  wrote:
>> Hi Ard,
>>
>> I like the concept, but ...
>>
>> Ard Biesheuvel  writes:
>>> The symbol CRCs are emitted as ELF symbols, which allows us to easily
>>> populate the kcrctab sections by relying on the linker to associate
>>> each kcrctab slot with the correct value.
>>>
>>> This has two downsides:
>>> - given that the CRCs are treated as pointers, we waste 4 bytes for
>>>   each CRC on 64 bit architectures,
>>> - on architectures that support runtime relocation, a relocation entry is
>>>   emitted for each CRC value, which may take up 24 bytes of __init space
>>>   (on ELF64 systems)
>>>
>>> This comes down to a x8 overhead in [uncompressed] kernel size. In addition,
>>> each relocation has to be reverted before the CRC value can be used.
>>>
>>> Switching to explicit 32 bit values on 64 bit architectures fixes both
>>> issues, since 32 bit values are not treated as relocatable quantities on
>>> ELF64 systems, even if the value ultimately resolves to a linker supplied
>>> value.
>>
>> Are we sure that part is true? ("not treated as relocatable")
>>
>
> Thanks for testing this.
>
>> A quick test build on powerpc gives me:
>>
>>   WARNING: 6829 bad relocations
>>   c0ca3748 R_PPC64_ADDR16*ABS*+0x13f53da6
>>   c0ca374a R_PPC64_ADDR16*ABS*+0xf7272059
>>   c0ca374c R_PPC64_ADDR16*ABS*+0x02013d36
>>   c0ca374e R_PPC64_ADDR16*ABS*+0xa59dffc8
>>   ...
>>
>> Which is coming from our relocs_check.sh script, which checks that the
>> generated relocations are ones we know how to handle.
>>
>
> OK, first of all, it appears the ppc64 assembler interprets .word
> differently than the arm64 one, so I will need to fold this in
>
> """
> diff --git a/include/linux/export.h b/include/linux/export.h
> index fa51ab2ad190..a000d421526d 100644
> --- a/include/linux/export.h
> +++ b/include/linux/export.h
> @@ -54,7 +54,7 @@ extern struct module __this_module;
>  #define __CRC_SYMBOL(sym, sec) \
> asm("   .section \"___kcrctab" sec "+" #sym "\", \"a\"  \n" \
> "   .weak   " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
> -   "   .word   " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
> +   "   .long   " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
> "   .previous   \n");
>  #endif
>  #else
> """
>
> With that change, the CRCs are actually emitted as
>
> WARNING: 7525 bad relocations
> c0ce7f50 R_PPC64_ADDR32*ABS*+0x13f53da6
> c0ce7f54 R_PPC64_ADDR32*ABS*+0x04f7c64e
> c0ce7f58 R_PPC64_ADDR32*ABS*+0x92be8a3e
>
> which is still a bit disappointing, given that we still have 7525 RELA
> entries to process.
> I tried several thing, i.e., adding -Bsymbolic to the linker command
> line, emitting the reference above as .hidden or emit  the definition
> from the linker script as HIDDEN(), but nothing seems to make any
> difference. (On arm64, -Bsymbolic eliminates *all* runtime relocations
> except R__RELATIVE ones)
>
>> And when I try to boot it I get:
>>
>>   virtio: disagrees about version of symbol module_layout
>>   virtio: disagrees about version of symbol module_layout
>>   scsi_mod: disagrees about version of symbol module_layout
>>
>> And it can't find my root file system (unsurprisingly as it's on scsi).
>>
>
> Something like the below should fix it, I hope.
>
> """
> diff --git a/arch/powerpc/kernel/reloc_64.S b/arch/powerpc/kernel/reloc_64.S
> index d88736fbece6..99cdf2311ab5 100644
> --- a/arch/powerpc/kernel/reloc_64.S
> +++ b/arch/powerpc/kernel/reloc_64.S
> @@ -14,6 +14,7 @@
>  RELA = 7
>  RELACOUNT = 0x6ff9
>  R_PPC64_RELATIVE = 22
> +R_PPC64_ADDR32 = 1
>
>  /*
>   * r3 = desired final address of kernel
> @@ -77,9 +78,22 @@ _GLOBAL(relocate)
> add r0,r0,r3
> stdxr0,r7,r6
> addir9,r9,24
> -   bdnz5b
> +   b   7f
> +
> +   /*
> +* CRCs of exported symbols are emitted as 32-bit relocations against
> +* the *ABS* section with the CRC value recorded in the addend.
> +*/
> +6: cmpdi   r0,R_PPC64_ADDR32
> +   bne 7f
> +   ld  r6,0(r9)/* reloc->r_offset */
> +   ld  r0,16(r9)   /* reloc->r_addend */
> +   stwxr0,r7,r6
> +   addir9,r9,24
> +
> +7: bdnz5b
> +   blr
>
> -6: blr
>
>  .balign 8
>  p_dyn: .llong  __dynamic_start - 0b
> """
>
> Note that the loop no longer terminates at the first
> non-R_PPC64_RELATIVE relocation, but that seems safer to me in any
> case. It simply stores the value of r_addend at r_offset, which is the
> correct thing to do for R_PPC64_ADDR32 relocations against the *ABS*
> section, regardless of 

Re: [PATCH v2] modversions: treat symbol CRCs as 32 bit quantities on 64 bit archs

[Ard Biesheuvel]

On 26 October 2016 at 14:04, Ard Biesheuvel  wrote:
> On 26 October 2016 at 11:07, Michael Ellerman  wrote:
>> Hi Ard,
>>
>> I like the concept, but ...
>>
>> Ard Biesheuvel  writes:
>>> The symbol CRCs are emitted as ELF symbols, which allows us to easily
>>> populate the kcrctab sections by relying on the linker to associate
>>> each kcrctab slot with the correct value.
>>>
>>> This has two downsides:
>>> - given that the CRCs are treated as pointers, we waste 4 bytes for
>>>   each CRC on 64 bit architectures,
>>> - on architectures that support runtime relocation, a relocation entry is
>>>   emitted for each CRC value, which may take up 24 bytes of __init space
>>>   (on ELF64 systems)
>>>
>>> This comes down to a x8 overhead in [uncompressed] kernel size. In addition,
>>> each relocation has to be reverted before the CRC value can be used.
>>>
>>> Switching to explicit 32 bit values on 64 bit architectures fixes both
>>> issues, since 32 bit values are not treated as relocatable quantities on
>>> ELF64 systems, even if the value ultimately resolves to a linker supplied
>>> value.
>>
>> Are we sure that part is true? ("not treated as relocatable")
>>
>
> Thanks for testing this.
>
>> A quick test build on powerpc gives me:
>>
>>   WARNING: 6829 bad relocations
>>   c0ca3748 R_PPC64_ADDR16*ABS*+0x13f53da6
>>   c0ca374a R_PPC64_ADDR16*ABS*+0xf7272059
>>   c0ca374c R_PPC64_ADDR16*ABS*+0x02013d36
>>   c0ca374e R_PPC64_ADDR16*ABS*+0xa59dffc8
>>   ...
>>
>> Which is coming from our relocs_check.sh script, which checks that the
>> generated relocations are ones we know how to handle.
>>
>
> OK, first of all, it appears the ppc64 assembler interprets .word
> differently than the arm64 one, so I will need to fold this in
>
> """
> diff --git a/include/linux/export.h b/include/linux/export.h
> index fa51ab2ad190..a000d421526d 100644
> --- a/include/linux/export.h
> +++ b/include/linux/export.h
> @@ -54,7 +54,7 @@ extern struct module __this_module;
>  #define __CRC_SYMBOL(sym, sec) \
> asm("   .section \"___kcrctab" sec "+" #sym "\", \"a\"  \n" \
> "   .weak   " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
> -   "   .word   " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
> +   "   .long   " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
> "   .previous   \n");
>  #endif
>  #else
> """
>
> With that change, the CRCs are actually emitted as
>
> WARNING: 7525 bad relocations
> c0ce7f50 R_PPC64_ADDR32*ABS*+0x13f53da6
> c0ce7f54 R_PPC64_ADDR32*ABS*+0x04f7c64e
> c0ce7f58 R_PPC64_ADDR32*ABS*+0x92be8a3e
>
> which is still a bit disappointing, given that we still have 7525 RELA
> entries to process.
> I tried several thing, i.e., adding -Bsymbolic to the linker command
> line, emitting the reference above as .hidden or emit  the definition
> from the linker script as HIDDEN(), but nothing seems to make any
> difference. (On arm64, -Bsymbolic eliminates *all* runtime relocations
> except R__RELATIVE ones)
>
>> And when I try to boot it I get:
>>
>>   virtio: disagrees about version of symbol module_layout
>>   virtio: disagrees about version of symbol module_layout
>>   scsi_mod: disagrees about version of symbol module_layout
>>
>> And it can't find my root file system (unsurprisingly as it's on scsi).
>>
>
> Something like the below should fix it, I hope.
>
> """
> diff --git a/arch/powerpc/kernel/reloc_64.S b/arch/powerpc/kernel/reloc_64.S
> index d88736fbece6..99cdf2311ab5 100644
> --- a/arch/powerpc/kernel/reloc_64.S
> +++ b/arch/powerpc/kernel/reloc_64.S
> @@ -14,6 +14,7 @@
>  RELA = 7
>  RELACOUNT = 0x6ff9
>  R_PPC64_RELATIVE = 22
> +R_PPC64_ADDR32 = 1
>
>  /*
>   * r3 = desired final address of kernel
> @@ -77,9 +78,22 @@ _GLOBAL(relocate)
> add r0,r0,r3
> stdxr0,r7,r6
> addir9,r9,24
> -   bdnz5b
> +   b   7f
> +
> +   /*
> +* CRCs of exported symbols are emitted as 32-bit relocations against
> +* the *ABS* section with the CRC value recorded in the addend.
> +*/
> +6: cmpdi   r0,R_PPC64_ADDR32
> +   bne 7f
> +   ld  r6,0(r9)/* reloc->r_offset */
> +   ld  r0,16(r9)   /* reloc->r_addend */
> +   stwxr0,r7,r6
> +   addir9,r9,24
> +
> +7: bdnz5b
> +   blr
>
> -6: blr
>
>  .balign 8
>  p_dyn: .llong  __dynamic_start - 0b
> """
>
> Note that the loop no longer terminates at the first
> non-R_PPC64_RELATIVE relocation, but that seems safer to me in any
> case. It simply stores the value of r_addend at r_offset, which is the
> correct thing to do for R_PPC64_ADDR32 relocations against the *ABS*
> section, regardless of whether we are dealing with CRCs or something
> else. Note that the comparison 

Re: [PATCH v2] modversions: treat symbol CRCs as 32 bit quantities on 64 bit archs

[Ard Biesheuvel]

On 26 October 2016 at 11:07, Michael Ellerman  wrote:
> Hi Ard,
>
> I like the concept, but ...
>
> Ard Biesheuvel  writes:
>> The symbol CRCs are emitted as ELF symbols, which allows us to easily
>> populate the kcrctab sections by relying on the linker to associate
>> each kcrctab slot with the correct value.
>>
>> This has two downsides:
>> - given that the CRCs are treated as pointers, we waste 4 bytes for
>>   each CRC on 64 bit architectures,
>> - on architectures that support runtime relocation, a relocation entry is
>>   emitted for each CRC value, which may take up 24 bytes of __init space
>>   (on ELF64 systems)
>>
>> This comes down to a x8 overhead in [uncompressed] kernel size. In addition,
>> each relocation has to be reverted before the CRC value can be used.
>>
>> Switching to explicit 32 bit values on 64 bit architectures fixes both
>> issues, since 32 bit values are not treated as relocatable quantities on
>> ELF64 systems, even if the value ultimately resolves to a linker supplied
>> value.
>
> Are we sure that part is true? ("not treated as relocatable")
>

Thanks for testing this.

> A quick test build on powerpc gives me:
>
>   WARNING: 6829 bad relocations
>   c0ca3748 R_PPC64_ADDR16*ABS*+0x13f53da6
>   c0ca374a R_PPC64_ADDR16*ABS*+0xf7272059
>   c0ca374c R_PPC64_ADDR16*ABS*+0x02013d36
>   c0ca374e R_PPC64_ADDR16*ABS*+0xa59dffc8
>   ...
>
> Which is coming from our relocs_check.sh script, which checks that the
> generated relocations are ones we know how to handle.
>

OK, first of all, it appears the ppc64 assembler interprets .word
differently than the arm64 one, so I will need to fold this in

"""
diff --git a/include/linux/export.h b/include/linux/export.h
index fa51ab2ad190..a000d421526d 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -54,7 +54,7 @@ extern struct module __this_module;
 #define __CRC_SYMBOL(sym, sec) \
asm("   .section \"___kcrctab" sec "+" #sym "\", \"a\"  \n" \
"   .weak   " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
-   "   .word   " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
+   "   .long   " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
"   .previous   \n");
 #endif
 #else
"""

With that change, the CRCs are actually emitted as

WARNING: 7525 bad relocations
c0ce7f50 R_PPC64_ADDR32*ABS*+0x13f53da6
c0ce7f54 R_PPC64_ADDR32*ABS*+0x04f7c64e
c0ce7f58 R_PPC64_ADDR32*ABS*+0x92be8a3e

which is still a bit disappointing, given that we still have 7525 RELA
entries to process.
I tried several thing, i.e., adding -Bsymbolic to the linker command
line, emitting the reference above as .hidden or emit  the definition
from the linker script as HIDDEN(), but nothing seems to make any
difference. (On arm64, -Bsymbolic eliminates *all* runtime relocations
except R__RELATIVE ones)

> And when I try to boot it I get:
>
>   virtio: disagrees about version of symbol module_layout
>   virtio: disagrees about version of symbol module_layout
>   scsi_mod: disagrees about version of symbol module_layout
>
> And it can't find my root file system (unsurprisingly as it's on scsi).
>

Something like the below should fix it, I hope.

"""
diff --git a/arch/powerpc/kernel/reloc_64.S b/arch/powerpc/kernel/reloc_64.S
index d88736fbece6..99cdf2311ab5 100644
--- a/arch/powerpc/kernel/reloc_64.S
+++ b/arch/powerpc/kernel/reloc_64.S
@@ -14,6 +14,7 @@
 RELA = 7
 RELACOUNT = 0x6ff9
 R_PPC64_RELATIVE = 22
+R_PPC64_ADDR32 = 1

 /*
  * r3 = desired final address of kernel
@@ -77,9 +78,22 @@ _GLOBAL(relocate)
add r0,r0,r3
stdxr0,r7,r6
addir9,r9,24
-   bdnz5b
+   b   7f
+
+   /*
+* CRCs of exported symbols are emitted as 32-bit relocations against
+* the *ABS* section with the CRC value recorded in the addend.
+*/
+6: cmpdi   r0,R_PPC64_ADDR32
+   bne 7f
+   ld  r6,0(r9)/* reloc->r_offset */
+   ld  r0,16(r9)   /* reloc->r_addend */
+   stwxr0,r7,r6
+   addir9,r9,24
+
+7: bdnz5b
+   blr

-6: blr

 .balign 8
 p_dyn: .llong  __dynamic_start - 0b
"""

Note that the loop no longer terminates at the first
non-R_PPC64_RELATIVE relocation, but that seems safer to me in any
case. It simply stores the value of r_addend at r_offset, which is the
correct thing to do for R_PPC64_ADDR32 relocations against the *ABS*
section, regardless of whether we are dealing with CRCs or something
else. Note that the comparison above will fail for R_PPC64_ADDR32
relocations against named symbols, since we compare the entire r_info
field and not just the type (as the comment a few lines higher up
suggests)

Also a fix for 

Re: [PATCH v2] modversions: treat symbol CRCs as 32 bit quantities on 64 bit archs

[Ard Biesheuvel]

On 26 October 2016 at 11:07, Michael Ellerman  wrote:
> Hi Ard,
>
> I like the concept, but ...
>
> Ard Biesheuvel  writes:
>> The symbol CRCs are emitted as ELF symbols, which allows us to easily
>> populate the kcrctab sections by relying on the linker to associate
>> each kcrctab slot with the correct value.
>>
>> This has two downsides:
>> - given that the CRCs are treated as pointers, we waste 4 bytes for
>>   each CRC on 64 bit architectures,
>> - on architectures that support runtime relocation, a relocation entry is
>>   emitted for each CRC value, which may take up 24 bytes of __init space
>>   (on ELF64 systems)
>>
>> This comes down to a x8 overhead in [uncompressed] kernel size. In addition,
>> each relocation has to be reverted before the CRC value can be used.
>>
>> Switching to explicit 32 bit values on 64 bit architectures fixes both
>> issues, since 32 bit values are not treated as relocatable quantities on
>> ELF64 systems, even if the value ultimately resolves to a linker supplied
>> value.
>
> Are we sure that part is true? ("not treated as relocatable")
>

Thanks for testing this.

> A quick test build on powerpc gives me:
>
>   WARNING: 6829 bad relocations
>   c0ca3748 R_PPC64_ADDR16*ABS*+0x13f53da6
>   c0ca374a R_PPC64_ADDR16*ABS*+0xf7272059
>   c0ca374c R_PPC64_ADDR16*ABS*+0x02013d36
>   c0ca374e R_PPC64_ADDR16*ABS*+0xa59dffc8
>   ...
>
> Which is coming from our relocs_check.sh script, which checks that the
> generated relocations are ones we know how to handle.
>

OK, first of all, it appears the ppc64 assembler interprets .word
differently than the arm64 one, so I will need to fold this in

"""
diff --git a/include/linux/export.h b/include/linux/export.h
index fa51ab2ad190..a000d421526d 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -54,7 +54,7 @@ extern struct module __this_module;
 #define __CRC_SYMBOL(sym, sec) \
asm("   .section \"___kcrctab" sec "+" #sym "\", \"a\"  \n" \
"   .weak   " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
-   "   .word   " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
+   "   .long   " VMLINUX_SYMBOL_STR(__crc_##sym) " \n" \
"   .previous   \n");
 #endif
 #else
"""

With that change, the CRCs are actually emitted as

WARNING: 7525 bad relocations
c0ce7f50 R_PPC64_ADDR32*ABS*+0x13f53da6
c0ce7f54 R_PPC64_ADDR32*ABS*+0x04f7c64e
c0ce7f58 R_PPC64_ADDR32*ABS*+0x92be8a3e

which is still a bit disappointing, given that we still have 7525 RELA
entries to process.
I tried several thing, i.e., adding -Bsymbolic to the linker command
line, emitting the reference above as .hidden or emit  the definition
from the linker script as HIDDEN(), but nothing seems to make any
difference. (On arm64, -Bsymbolic eliminates *all* runtime relocations
except R__RELATIVE ones)

> And when I try to boot it I get:
>
>   virtio: disagrees about version of symbol module_layout
>   virtio: disagrees about version of symbol module_layout
>   scsi_mod: disagrees about version of symbol module_layout
>
> And it can't find my root file system (unsurprisingly as it's on scsi).
>

Something like the below should fix it, I hope.

"""
diff --git a/arch/powerpc/kernel/reloc_64.S b/arch/powerpc/kernel/reloc_64.S
index d88736fbece6..99cdf2311ab5 100644
--- a/arch/powerpc/kernel/reloc_64.S
+++ b/arch/powerpc/kernel/reloc_64.S
@@ -14,6 +14,7 @@
 RELA = 7
 RELACOUNT = 0x6ff9
 R_PPC64_RELATIVE = 22
+R_PPC64_ADDR32 = 1

 /*
  * r3 = desired final address of kernel
@@ -77,9 +78,22 @@ _GLOBAL(relocate)
add r0,r0,r3
stdxr0,r7,r6
addir9,r9,24
-   bdnz5b
+   b   7f
+
+   /*
+* CRCs of exported symbols are emitted as 32-bit relocations against
+* the *ABS* section with the CRC value recorded in the addend.
+*/
+6: cmpdi   r0,R_PPC64_ADDR32
+   bne 7f
+   ld  r6,0(r9)/* reloc->r_offset */
+   ld  r0,16(r9)   /* reloc->r_addend */
+   stwxr0,r7,r6
+   addir9,r9,24
+
+7: bdnz5b
+   blr

-6: blr

 .balign 8
 p_dyn: .llong  __dynamic_start - 0b
"""

Note that the loop no longer terminates at the first
non-R_PPC64_RELATIVE relocation, but that seems safer to me in any
case. It simply stores the value of r_addend at r_offset, which is the
correct thing to do for R_PPC64_ADDR32 relocations against the *ABS*
section, regardless of whether we are dealing with CRCs or something
else. Note that the comparison above will fail for R_PPC64_ADDR32
relocations against named symbols, since we compare the entire r_info
field and not just the type (as the comment a few lines higher up
suggests)

Also a fix for relocs_check.sh:

"""
diff --git a/arch/powerpc/relocs_check.sh 

Re: [PATCH v2] modversions: treat symbol CRCs as 32 bit quantities on 64 bit archs

[Michael Ellerman]

Hi Ard,

I like the concept, but ...

Ard Biesheuvel  writes:
> The symbol CRCs are emitted as ELF symbols, which allows us to easily
> populate the kcrctab sections by relying on the linker to associate
> each kcrctab slot with the correct value.
>
> This has two downsides:
> - given that the CRCs are treated as pointers, we waste 4 bytes for
>   each CRC on 64 bit architectures,
> - on architectures that support runtime relocation, a relocation entry is
>   emitted for each CRC value, which may take up 24 bytes of __init space
>   (on ELF64 systems)
>
> This comes down to a x8 overhead in [uncompressed] kernel size. In addition,
> each relocation has to be reverted before the CRC value can be used.
>
> Switching to explicit 32 bit values on 64 bit architectures fixes both
> issues, since 32 bit values are not treated as relocatable quantities on
> ELF64 systems, even if the value ultimately resolves to a linker supplied
> value.

Are we sure that part is true? ("not treated as relocatable")

A quick test build on powerpc gives me:

  WARNING: 6829 bad relocations
  c0ca3748 R_PPC64_ADDR16*ABS*+0x13f53da6
  c0ca374a R_PPC64_ADDR16*ABS*+0xf7272059
  c0ca374c R_PPC64_ADDR16*ABS*+0x02013d36
  c0ca374e R_PPC64_ADDR16*ABS*+0xa59dffc8
  ...

Which is coming from our relocs_check.sh script, which checks that the
generated relocations are ones we know how to handle.

And when I try to boot it I get:

  virtio: disagrees about version of symbol module_layout
  virtio: disagrees about version of symbol module_layout
  scsi_mod: disagrees about version of symbol module_layout

And it can't find my root file system (unsurprisingly as it's on scsi).

Will try and investigate more tomorrow.

cheers

Re: [PATCH v2] modversions: treat symbol CRCs as 32 bit quantities on 64 bit archs

[Michael Ellerman]

Hi Ard,

I like the concept, but ...

Ard Biesheuvel  writes:
> The symbol CRCs are emitted as ELF symbols, which allows us to easily
> populate the kcrctab sections by relying on the linker to associate
> each kcrctab slot with the correct value.
>
> This has two downsides:
> - given that the CRCs are treated as pointers, we waste 4 bytes for
>   each CRC on 64 bit architectures,
> - on architectures that support runtime relocation, a relocation entry is
>   emitted for each CRC value, which may take up 24 bytes of __init space
>   (on ELF64 systems)
>
> This comes down to a x8 overhead in [uncompressed] kernel size. In addition,
> each relocation has to be reverted before the CRC value can be used.
>
> Switching to explicit 32 bit values on 64 bit architectures fixes both
> issues, since 32 bit values are not treated as relocatable quantities on
> ELF64 systems, even if the value ultimately resolves to a linker supplied
> value.

Are we sure that part is true? ("not treated as relocatable")

A quick test build on powerpc gives me:

  WARNING: 6829 bad relocations
  c0ca3748 R_PPC64_ADDR16*ABS*+0x13f53da6
  c0ca374a R_PPC64_ADDR16*ABS*+0xf7272059
  c0ca374c R_PPC64_ADDR16*ABS*+0x02013d36
  c0ca374e R_PPC64_ADDR16*ABS*+0xa59dffc8
  ...

Which is coming from our relocs_check.sh script, which checks that the
generated relocations are ones we know how to handle.

And when I try to boot it I get:

  virtio: disagrees about version of symbol module_layout
  virtio: disagrees about version of symbol module_layout
  scsi_mod: disagrees about version of symbol module_layout

And it can't find my root file system (unsurprisingly as it's on scsi).

Will try and investigate more tomorrow.

cheers

Re: [PATCH v2] modversions: treat symbol CRCs as 32 bit quantities on 64 bit archs

[Rusty Russell]

Ard Biesheuvel  writes:
> The symbol CRCs are emitted as ELF symbols, which allows us to easily
> populate the kcrctab sections by relying on the linker to associate
> each kcrctab slot with the correct value.
>
> This has two downsides:
> - given that the CRCs are treated as pointers, we waste 4 bytes for
>   each CRC on 64 bit architectures,
> - on architectures that support runtime relocation, a relocation entry is
>   emitted for each CRC value, which may take up 24 bytes of __init space
>   (on ELF64 systems)
>
> This comes down to a x8 overhead in [uncompressed] kernel size. In addition,
> each relocation has to be reverted before the CRC value can be used.
>
> Switching to explicit 32 bit values on 64 bit architectures fixes both
> issues, since 32 bit values are not treated as relocatable quantities on
> ELF64 systems, even if the value ultimately resolves to a linker supplied
> value.
>
> So redefine all CRC fields and variables as u32, and redefine the
> __CRC_SYMBOL() macro for 64 bit builds to emit the CRC reference using
> inline assembler (which is necessary since 64-bit C code cannot use
> 32-bit types to hold memory addresses, even if they are ultimately
> resolved using values that do no exceed 0x).
>
> Also remove the special handling for PPC64, this should no longer be
> required.
>
> Signed-off-by: Ard Biesheuvel 

This looks good!  Thanks for this, it fixes a nasty wart with the
relocation of crcs.

If the ppc and arm maintainers are happy, I'm happy for Jessica to take
it into her module tree.

Acked-by: Rusty Russell 

Thanks,
Rusty.

> ---
> v2: drop the change to struct modversion_info: it affects the layout of the
> __versions section, which is consumed by userland tools as well, so it is
> effectively ABI
>
> On an arm64 defconfig build with CONFIG_RELOCATABLE=y, this patch reduces
> the CRC footprint by 24 KB for .rodata, and by 217 KB for .init
>
> Before:
>   [ 9] __kcrctab PROGBITS 08b992a8  00b292a8
>9440     A   0 0 8
>   [10] __kcrctab_gpl PROGBITS 08ba26e8  00b326e8
>8d40     A   0 0 8
>   ...
>   [22] .rela RELA 08c96e20  00c26e20
>001cc758  0018   A   0 0 8
>
> After:
>   [ 9] __kcrctab PROGBITS 08b728a8  00b028a8
>4a20     A   0 0 1
>   [10] __kcrctab_gpl PROGBITS 08b772c8  00b072c8
>46a0     A   0 0 1
>   ...
>   [22] .rela RELA 08c66e20  00bf6e20
>001962d8  0018   A   0 0 8
>
>  arch/powerpc/include/asm/module.h |  4 --
>  arch/powerpc/kernel/module_64.c   |  8 
>  include/linux/export.h|  8 
>  include/linux/module.h| 14 +++
>  kernel/module.c   | 39 +++-
>  5 files changed, 29 insertions(+), 44 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/module.h 
> b/arch/powerpc/include/asm/module.h
> index cd4ffd86765f..94a7f7aa3ae8 100644
> --- a/arch/powerpc/include/asm/module.h
> +++ b/arch/powerpc/include/asm/module.h
> @@ -94,9 +94,5 @@ struct exception_table_entry;
>  void sort_ex_table(struct exception_table_entry *start,
>  struct exception_table_entry *finish);
>  
> -#if defined(CONFIG_MODVERSIONS) && defined(CONFIG_PPC64)
> -#define ARCH_RELOCATES_KCRCTAB
> -#define reloc_start PHYSICAL_START
> -#endif
>  #endif /* __KERNEL__ */
>  #endif   /* _ASM_POWERPC_MODULE_H */
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 183368e008cf..be9b2d5ff846 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -286,14 +286,6 @@ static void dedotify_versions(struct modversion_info 
> *vers,
>   for (end = (void *)vers + size; vers < end; vers++)
>   if (vers->name[0] == '.') {
>   memmove(vers->name, vers->name+1, strlen(vers->name));
> -#ifdef ARCH_RELOCATES_KCRCTAB
> - /* The TOC symbol has no CRC computed. To avoid CRC
> -  * check failing, we must force it to the expected
> -  * value (see CRC check in module.c).
> -  */
> - if (!strcmp(vers->name, "TOC."))
> - vers->crc = -(unsigned long)reloc_start;
> -#endif
>   }
>  }
>  
> diff --git a/include/linux/export.h b/include/linux/export.h
> index 2a0f61fbc731..fa51ab2ad190 100644
> --- a/include/linux/export.h
> +++ b/include/linux/export.h
> @@ -41,6 +41,7 @@ extern struct module __this_module;
>  
>  #if defined(__KERNEL__) && !defined(__GENKSYMS__)
>  #ifdef 

Re: [PATCH v2] modversions: treat symbol CRCs as 32 bit quantities on 64 bit archs

[Rusty Russell]

Ard Biesheuvel  writes:
> The symbol CRCs are emitted as ELF symbols, which allows us to easily
> populate the kcrctab sections by relying on the linker to associate
> each kcrctab slot with the correct value.
>
> This has two downsides:
> - given that the CRCs are treated as pointers, we waste 4 bytes for
>   each CRC on 64 bit architectures,
> - on architectures that support runtime relocation, a relocation entry is
>   emitted for each CRC value, which may take up 24 bytes of __init space
>   (on ELF64 systems)
>
> This comes down to a x8 overhead in [uncompressed] kernel size. In addition,
> each relocation has to be reverted before the CRC value can be used.
>
> Switching to explicit 32 bit values on 64 bit architectures fixes both
> issues, since 32 bit values are not treated as relocatable quantities on
> ELF64 systems, even if the value ultimately resolves to a linker supplied
> value.
>
> So redefine all CRC fields and variables as u32, and redefine the
> __CRC_SYMBOL() macro for 64 bit builds to emit the CRC reference using
> inline assembler (which is necessary since 64-bit C code cannot use
> 32-bit types to hold memory addresses, even if they are ultimately
> resolved using values that do no exceed 0x).
>
> Also remove the special handling for PPC64, this should no longer be
> required.
>
> Signed-off-by: Ard Biesheuvel 

This looks good!  Thanks for this, it fixes a nasty wart with the
relocation of crcs.

If the ppc and arm maintainers are happy, I'm happy for Jessica to take
it into her module tree.

Acked-by: Rusty Russell 

Thanks,
Rusty.

> ---
> v2: drop the change to struct modversion_info: it affects the layout of the
> __versions section, which is consumed by userland tools as well, so it is
> effectively ABI
>
> On an arm64 defconfig build with CONFIG_RELOCATABLE=y, this patch reduces
> the CRC footprint by 24 KB for .rodata, and by 217 KB for .init
>
> Before:
>   [ 9] __kcrctab PROGBITS 08b992a8  00b292a8
>9440     A   0 0 8
>   [10] __kcrctab_gpl PROGBITS 08ba26e8  00b326e8
>8d40     A   0 0 8
>   ...
>   [22] .rela RELA 08c96e20  00c26e20
>001cc758  0018   A   0 0 8
>
> After:
>   [ 9] __kcrctab PROGBITS 08b728a8  00b028a8
>4a20     A   0 0 1
>   [10] __kcrctab_gpl PROGBITS 08b772c8  00b072c8
>46a0     A   0 0 1
>   ...
>   [22] .rela RELA 08c66e20  00bf6e20
>001962d8  0018   A   0 0 8
>
>  arch/powerpc/include/asm/module.h |  4 --
>  arch/powerpc/kernel/module_64.c   |  8 
>  include/linux/export.h|  8 
>  include/linux/module.h| 14 +++
>  kernel/module.c   | 39 +++-
>  5 files changed, 29 insertions(+), 44 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/module.h 
> b/arch/powerpc/include/asm/module.h
> index cd4ffd86765f..94a7f7aa3ae8 100644
> --- a/arch/powerpc/include/asm/module.h
> +++ b/arch/powerpc/include/asm/module.h
> @@ -94,9 +94,5 @@ struct exception_table_entry;
>  void sort_ex_table(struct exception_table_entry *start,
>  struct exception_table_entry *finish);
>  
> -#if defined(CONFIG_MODVERSIONS) && defined(CONFIG_PPC64)
> -#define ARCH_RELOCATES_KCRCTAB
> -#define reloc_start PHYSICAL_START
> -#endif
>  #endif /* __KERNEL__ */
>  #endif   /* _ASM_POWERPC_MODULE_H */
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 183368e008cf..be9b2d5ff846 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -286,14 +286,6 @@ static void dedotify_versions(struct modversion_info 
> *vers,
>   for (end = (void *)vers + size; vers < end; vers++)
>   if (vers->name[0] == '.') {
>   memmove(vers->name, vers->name+1, strlen(vers->name));
> -#ifdef ARCH_RELOCATES_KCRCTAB
> - /* The TOC symbol has no CRC computed. To avoid CRC
> -  * check failing, we must force it to the expected
> -  * value (see CRC check in module.c).
> -  */
> - if (!strcmp(vers->name, "TOC."))
> - vers->crc = -(unsigned long)reloc_start;
> -#endif
>   }
>  }
>  
> diff --git a/include/linux/export.h b/include/linux/export.h
> index 2a0f61fbc731..fa51ab2ad190 100644
> --- a/include/linux/export.h
> +++ b/include/linux/export.h
> @@ -41,6 +41,7 @@ extern struct module __this_module;
>  
>  #if defined(__KERNEL__) && !defined(__GENKSYMS__)
>  #ifdef CONFIG_MODVERSIONS
> +#ifndef CONFIG_64BIT
>  /* Mark the CRC weak since genksyms apparently