Re: [PATCH] LoongArch: Fix lockdep static memory detection

[Helge Deller <deller@gmx.de>]

On 9/15/23 11:23, Huacai Chen wrote:
> On Fri, Sep 15, 2023 at 4:16 PM Helge Deller <deller@gmx.de> wrote:
>>
>> On 9/15/23 05:22, Huacai Chen wrote:
>>> Hi Helge,
>>>
>>> On Wed, Sep 13, 2023 at 3:18 AM Helge Deller <deller@gmx.de> wrote:
>>>>
>>>> Since commit 0a6b58c5cd0d ("lockdep: fix static memory detection even
>>>> more") the lockdep code uses is_kernel_core_data(), is_kernel_rodata()
>>>> and init_section_contains() to verify if a lock is located inside a
>>>> kernel static data section.
>>>>
>>>> This change triggers a failure on LoongArch, for which the vmlinux.lds.S
>>>> script misses to put the locks (as part of in the .data.rel symbols)
>>>> into the Linux data section.
>>>> This patch fixes the lockdep problem by moving *(.data.rel*) symbols
>>>> into the kernel data section (from _sdata to _edata).
>>>>
>>>> Additionally, move other wrongly assigned symbols too:
>>>> - altinstructions into the _initdata section,
>>
>>> I think altinstructions cannot  be put into _initdata because it will
>>> be used by modules.
>>
>> No.
>> arch/loongarch/kernel/vmlinux.lds.S is used for the static parts of the kernel
>> and altinstructions are replaced before modules are loaded.
>> For altinstructions in modules the linker script scripts/module.lds.S is used.

> OK, then what about .got/.plt? It seems arm64 also doesn't put them in
> the data section.

arm64 seems to throw away all plt entries already at link time (and just keeps
the got.plt in the read-only data section).
It even checks at link time, that there are no plt entries in the binary:
         ASSERT(SIZEOF(.plt) == 0, "Unexpected run-time procedure linkages detected!")

I don't know for loongarch, but if you need the plt entries for loongarch, it's
safest & best to put them into the read-only data section too, which is what my patch does.
Up to now, you have them completely outside of code & data sections.

In the end you need to decide for your platform. My patch is a suggestion, which I think
is correct (untested by me, but Guenter replied he tested it).
But to fix the lockdep problem at minimum the move of the .data.rel section
is needed.

Helge

>>
>>>> - PLT symbols behind the read-only section, and
>>>> - *(.la_abs) into the data section.
>>>>
>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>>>> Fixes: 0a6b58c5cd0d ("lockdep: fix static memory detection even more")
>>>> Cc: stable <stable@kernel.org> # v6.4+
>>>>
>>>> diff --git a/arch/loongarch/kernel/vmlinux.lds.S b/arch/loongarch/kernel/vmlinux.lds.S
>>>> index b1686afcf876..bb2ec86f37a8 100644
>>>> --- a/arch/loongarch/kernel/vmlinux.lds.S
>>>> +++ b/arch/loongarch/kernel/vmlinux.lds.S
>>>> @@ -53,33 +53,6 @@ SECTIONS
>>>>           . = ALIGN(PECOFF_SEGMENT_ALIGN);
>>>>           _etext = .;
>>>>
>>>> -       /*
>>>> -        * struct alt_inst entries. From the header (alternative.h):
>>>> -        * "Alternative instructions for different CPU types or capabilities"
>>>> -        * Think locking instructions on spinlocks.
>>>> -        */
>>>> -       . = ALIGN(4);
>>>> -       .altinstructions : AT(ADDR(.altinstructions) - LOAD_OFFSET) {
>>>> -               __alt_instructions = .;
>>>> -               *(.altinstructions)
>>>> -               __alt_instructions_end = .;
>>>> -       }
>>>> -
>>>> -#ifdef CONFIG_RELOCATABLE
>>>> -       . = ALIGN(8);
>>>> -       .la_abs : AT(ADDR(.la_abs) - LOAD_OFFSET) {
>>>> -               __la_abs_begin = .;
>>>> -               *(.la_abs)
>>>> -               __la_abs_end = .;
>>>> -       }
>>>> -#endif
>>>> -
>>>> -       .got : ALIGN(16) { *(.got) }
>>>> -       .plt : ALIGN(16) { *(.plt) }
>>>> -       .got.plt : ALIGN(16) { *(.got.plt) }
>>>> -
>>>> -       .data.rel : { *(.data.rel*) }
>>>> -
>>>>           . = ALIGN(PECOFF_SEGMENT_ALIGN);
>>>>           __init_begin = .;
>>>>           __inittext_begin = .;
>>>> @@ -94,6 +67,18 @@ SECTIONS
>>>>
>>>>           __initdata_begin = .;
>>>>
>>>> +       /*
>>>> +        * struct alt_inst entries. From the header (alternative.h):
>>>> +        * "Alternative instructions for different CPU types or capabilities"
>>>> +        * Think locking instructions on spinlocks.
>>>> +        */
>>>> +       . = ALIGN(4);
>>>> +       .altinstructions : AT(ADDR(.altinstructions) - LOAD_OFFSET) {
>>>> +               __alt_instructions = .;
>>>> +               *(.altinstructions)
>>>> +               __alt_instructions_end = .;
>>>> +       }
>>>> +
>>>>           INIT_DATA_SECTION(16)
>>>>           .exit.data : {
>>>>                   EXIT_DATA
>>>> @@ -113,6 +98,11 @@ SECTIONS
>>>>
>>>>           _sdata = .;
>>>>           RO_DATA(4096)
>>>> +
>>>> +       .got : ALIGN(16) { *(.got) }
>>>> +       .plt : ALIGN(16) { *(.plt) }
>>>> +       .got.plt : ALIGN(16) { *(.got.plt) }
>>>> +
>>>>           RW_DATA(1 << CONFIG_L1_CACHE_SHIFT, PAGE_SIZE, THREAD_SIZE)
>>>>
>>>>           .rela.dyn : ALIGN(8) {
>>>> @@ -121,6 +111,17 @@ SECTIONS
>>>>                   __rela_dyn_end = .;
>>>>           }
>>>>
>>>> +       .data.rel : { *(.data.rel*) }
>>>> +
>>>> +#ifdef CONFIG_RELOCATABLE
>>>> +       . = ALIGN(8);
>>>> +       .la_abs : AT(ADDR(.la_abs) - LOAD_OFFSET) {
>>>> +               __la_abs_begin = .;
>>>> +               *(.la_abs)
>>>> +               __la_abs_end = .;
>>>> +       }
>>>> +#endif
>>>> +
>>>>           .sdata : {
>>>>                   *(.sdata)
>>>>           }
>>