diff mbox

[v4,3/3] modversions: treat symbol CRCs as 32 bit quantities on 64 bit archs

Message ID CAKv+Gu-wqoQVXvZcEF8xfAnHmrakKU73ToMNO+2Lw158B4teJA@mail.gmail.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Ard Biesheuvel Jan. 18, 2017, 10:37 p.m. UTC 
On 18 January 2017 at 18:35, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Jan 18, 2017 at 10:27 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> I wonder what happened to gold, and why it didn't take over. I'm
>> assuming it had _other_ bugs.. Oh well.
>
> Google for gold problems, I note that it has been reported to get
> "internal error"s during kernel builds - and at least some of them
> have been due to ksyms.
>
> So the core problem seems to mainly be that gcc normally itself never
> generates any absolute symbols, so the whole ksyms model depends on
> things that get almost zero testing in the toolchain.
>
> Oh well.
>

There is one alternative that gets rid of all the hackiness, but it
does increase the CRC footprint on 32-bit platforms:


(and an equivalent change in include/linux/export.h)

So the kcrctab contains the relative (signed) offset to where the CRC
is stored, which gets rid of any absolute relocations. To grab the
CRC, we only have to add the value of the kcrctab entry to its
address, and dereference the resulting pointer.

This would remove the need for patches 1 and 2, and get rid of the
overhead of the runtime relocation entries not only for arm64 but for
powerpc as well. It would also get rid of the abuse of ELF symbols
once and for all, hopefully avoiding bugs in GNU ld and gold in the
future.

For a ballpark number of 10,000 CRCs in the core kernel, this would
increase the size of the image by 40 KB for 32-bit architectures (and
if saving 40 KB is essential, chances are you won't be using
modversions in the first place). For 64-bit architectures, there would
be no change in size, of course, except for saving 24 bytes of __init
space *per CRC* for arm64 and powerpc64 with CONFIG_RELOCATABLE=y

I will send out a separate RFC so we can discuss this alternative

Comments

Linus Torvalds Jan. 19, 2017, 12:15 a.m. UTC | #1 
On Wed, Jan 18, 2017 at 2:37 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> For a ballpark number of 10,000 CRCs in the core kernel, this would
> increase the size of the image by 40 KB for 32-bit architectures (and
> if saving 40 KB is essential, chances are you won't be using
> modversions in the first place).

As you say, I don't think the space issue is much of a problem.

I'm more worried about the replacement of one crazy model that has
problems due to linker subtlety with _another_ one.

Your genksyms.c change is not exactly obvious. I looked at it, and my
brain just shut down. Why both the

  LONG(0x%08lx);

_and_ the

  "%s__crc_%s = 0x%08lx;\n"

in the linker script? I'm sure there's a good reason, but I'd like to
see a more explicit explanation fo what the generated linker script
does and what the rules are.

         Linus
Ard Biesheuvel Jan. 19, 2017, 9:22 a.m. UTC | #2 
On 19 January 2017 at 00:15, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Jan 18, 2017 at 2:37 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>>
>> For a ballpark number of 10,000 CRCs in the core kernel, this would
>> increase the size of the image by 40 KB for 32-bit architectures (and
>> if saving 40 KB is essential, chances are you won't be using
>> modversions in the first place).
>
> As you say, I don't think the space issue is much of a problem.
>
> I'm more worried about the replacement of one crazy model that has
> problems due to linker subtlety with _another_ one.
>
> Your genksyms.c change is not exactly obvious. I looked at it, and my
> brain just shut down. Why both the
>
>   LONG(0x%08lx);
>
> _and_ the
>
>   "%s__crc_%s = 0x%08lx;\n"
>
> in the linker script? I'm sure there's a good reason, but I'd like to
> see a more explicit explanation fo what the generated linker script
> does and what the rules are.
>

This is simply because modpost still uses the value of the symbol
rather than the value it points to to generate the /other/ side of the
comparison (i.e., Module.symvers etc)

I will look into updating modpost to dereference the symbol as well,
and update the RFC patch accordingly.
Linus Torvalds Jan. 19, 2017, 5:24 p.m. UTC | #3 
On Thu, Jan 19, 2017 at 1:22 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>>
>> Your genksyms.c change is not exactly obvious. I looked at it, and my
>> brain just shut down. Why both the
>>
>>   LONG(0x%08lx);
>>
>> _and_ the
>>
>>   "%s__crc_%s = 0x%08lx;\n"
>>
>> in the linker script? I'm sure there's a good reason, but I'd like to
>> see a more explicit explanation fo what the generated linker script
>> does and what the rules are.
>
> This is simply because modpost still uses the value of the symbol
> rather than the value it points to to generate the /other/ side of the
> comparison (i.e., Module.symvers etc)

Ahh, now that you explained it, it was obvious. Thanks.

But yes, I don't think we want that "both belt and suspenders"
approach, so your updated patch that does things just one way is I
think the right way.

> I will look into updating modpost to dereference the symbol as well,
> and update the RFC patch accordingly.

Yes, so your updated patch looks good to me.

I think our old "symbol with an absolute value" model was simpler
conceptually, but given the existing absolute (sic) braindamage of
linkers, I think your latest patch is probably the way to go.

If for no other reason than the fact that it doesn't depend on
something that clearly nobody else uses, and even the linker people
were confused about.

So I think the slightly more complex model of relative offsets is the
simpler one in the end if it means that we don't have to have
completely insane workarounds for linker damage.

But maybe somebody else wants to pipe up. Preferably somebody who
doesn't hate the symversions code as much as I do by now, and actually
_uses_ it ;)

                Linus
Ard Biesheuvel Jan. 20, 2017, 12:21 p.m. UTC | #4 
On 19 January 2017 at 17:24, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Jan 19, 2017 at 1:22 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>>>
>>> Your genksyms.c change is not exactly obvious. I looked at it, and my
>>> brain just shut down. Why both the
>>>
>>>   LONG(0x%08lx);
>>>
>>> _and_ the
>>>
>>>   "%s__crc_%s = 0x%08lx;\n"
>>>
>>> in the linker script? I'm sure there's a good reason, but I'd like to
>>> see a more explicit explanation fo what the generated linker script
>>> does and what the rules are.
>>
>> This is simply because modpost still uses the value of the symbol
>> rather than the value it points to to generate the /other/ side of the
>> comparison (i.e., Module.symvers etc)
>
> Ahh, now that you explained it, it was obvious. Thanks.
>
> But yes, I don't think we want that "both belt and suspenders"
> approach, so your updated patch that does things just one way is I
> think the right way.
>
>> I will look into updating modpost to dereference the symbol as well,
>> and update the RFC patch accordingly.
>
> Yes, so your updated patch looks good to me.
>
> I think our old "symbol with an absolute value" model was simpler
> conceptually, but given the existing absolute (sic) braindamage of
> linkers, I think your latest patch is probably the way to go.
>

OK.

> If for no other reason than the fact that it doesn't depend on
> something that clearly nobody else uses, and even the linker people
> were confused about.
>
> So I think the slightly more complex model of relative offsets is the
> simpler one in the end if it means that we don't have to have
> completely insane workarounds for linker damage.
>
> But maybe somebody else wants to pipe up. Preferably somebody who
> doesn't hate the symversions code as much as I do by now, and actually
> _uses_ it ;)
>

I am not crazy about it either: I am simply trying to get rid of the
~10,000 pointless relocations in the arm64 KASLR kernel rather than
having to rely on the dodgy code that 'repairs' the CRCs at runtime.

I have noticed one slight snag though: the ARM module loader currently
has no support for the R_ARM_REL32 relocations that are emitted by the
relative references in the kcrctabs. I looked at other arches, x86,
ia64, s390, power and arm64, and those all seem to be fully equipped
in this regard, but it would be good if we could get some coverage for
this code on other architectures to find out which ones need to have
this support added as well.

Thanks,
Ard.
diff mbox

Patch

diff --git a/scripts/genksyms/genksyms.c b/scripts/genksyms/genksyms.c
index 06121ce524a7..9f739c7224c3 100644
--- a/scripts/genksyms/genksyms.c
+++ b/scripts/genksyms/genksyms.c
@@ -693,7 +693,10 @@  void export_symbol(const char *name)
                        fputs(">\n", debugfile);

                /* Used as a linker script. */
-               printf("%s__crc_%s = 0x%08lx ;\n", mod_prefix, name, crc);
+               printf("SECTIONS { .rodata.__crc_%s : ALIGN(4) "
+                      "{ %s__crcp_%s = .; LONG(0x%08lx); } }\n"
+                      "%s__crc_%s = 0x%08lx;\n",
+                      name, mod_prefix, name, crc, mod_prefix, name, crc);
        }
 }

diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
index e3508a3d6e53..5e95a552a871 100644
--- a/include/asm-generic/export.h
+++ b/include/asm-generic/export.h
@@ -49,8 +49,8 @@  KSYM(__kstrtab_\name):
        .section ___kcrctab\sec+\name,"a"
        .balign KCRC_ALIGN
 KSYM(__kcrctab_\name):
-       .long KSYM(__crc_\name)
-       .weak KSYM(__crc_\name)
+       .long KSYM(__crcp_\name) - .
+       .weak KSYM(__crcp_\name)
        .previous
 #endif
 #endif