On 16 April 2018 at 16:10, Michael Ellerman <m...@ellerman.id.au> wrote:
> Ard Biesheuvel <ard.biesheu...@linaro.org> writes:
>
>> On 11 April 2018 at 16:49, Michael Ellerman
>> <patch-notificati...@ellerman.id.au> wrote:
>>> On Tue, 2018-04-10 at 01:22:06 UTC, Michael Ellerman wrote:
>>>> If you build the kernel with CONFIG_RELOCATABLE=n, then install the
>>>> modules, rebuild the kernel with CONFIG_RELOCATABLE=y and leave the
>>>> old modules installed, we crash something like:
>>>>
>>>> Unable to handle kernel paging request for data at address
>>>> 0xd000000018d66cef
>>>> Faulting instruction address: 0xc0000000021ddd08
>>>> Oops: Kernel access of bad area, sig: 11 [#1]
>>>> Modules linked in: x_tables autofs4
>>>> CPU: 2 PID: 1 Comm: systemd Not tainted
>>>> 4.16.0-rc6-gcc_ubuntu_le-g99fec39 #1
>>>> ...
>>>> NIP check_version.isra.22+0x118/0x170
>>>> Call Trace:
>>>> __ksymtab_xt_unregister_table+0x58/0xfffffffffffffcb8 [x_tables]
>>>> (unreliable)
>>>> resolve_symbol+0xb4/0x150
>>>> load_module+0x10e8/0x29a0
>>>> SyS_finit_module+0x110/0x140
>>>> system_call+0x58/0x6c
>>>>
>>>> This happens because since commit 71810db27c1c ("modversions: treat
>>>> symbol CRCs as 32 bit quantities"), a relocatable kernel encodes and
>>>> handles symbol CRCs differently from a non-relocatable kernel.
>>>>
>>>> Although it's possible we could try and detect this situation and
>>>> handle it, it's much more robust to simply make the state of
>>>> CONFIG_RELOCATABLE part of the module vermagic.
>>>>
>>>> Fixes: 71810db27c1c ("modversions: treat symbol CRCs as 32 bit quantities")
>>>> Signed-off-by: Michael Ellerman <m...@ellerman.id.au>
>>>
>>> Applied to powerpc fixes.
>>>
>>> https://git.kernel.org/powerpc/c/73aca179d78eaa11604ba0783a6d8b
>>
>> Thanks for the cc. I guess this only affects powerpc, given that it is
>> the only arch that switches between CRC immediate values and CRC
>> offsets depending on the configuration.
>
> No worries.
>
> Is there any reason we shouldn't always turn on CONFIG_MODULE_REL_CRCS?
> It seems to work, but I wanted to test it more before switching to that,
> hence the quick fix above.
>
>
> arch/um looks like it might be switching based on config, but I don't
> know enough to say:
>
> config LD_SCRIPT_STATIC
> bool
> default y
> depends on STATIC_LINK
>
> config LD_SCRIPT_DYN
> bool
> default y
> depends on !LD_SCRIPT_STATIC
> select MODULE_REL_CRCS if MODVERSIONS
>
The only reason not to enable it is that it ends up taking more space
on a 32-bit architecture with CONFIG_RELOCATABLE=n, given that you
need to record both the relative offset and the actual CRC value (both
32-bit quantities) rather than just the CRC itself. On a 64-bit arch
with CONFIG_RELOCATABLE=n, you end up replacing a single 64-bit
quantity with two 32-bit quantities, so it doesn't really matter.
