Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <1160d063-8396-b126-15ca-a46807ec4258@gmx.de>
Date:   Fri, 15 Sep 2023 12:10:23 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] LoongArch: Fix lockdep static memory detection
Content-Language: en-US
To:     Huacai Chen <chenhuacai@kernel.org>
Cc:     WANG Xuerui <kernel@xen0n.name>, loongarch@lists.linux.dev,
        Guenter Roeck <linux@roeck-us.net>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        linux-kernel@vger.kernel.org
References: <ZQC5jS/Kc/JiBEOa@p100>
 <CAAhV-H56oqmQhA1PKh3E6z0A04qfRPehQ8ATOiirAU2O+z=fgw@mail.gmail.com>
 <b1c729bf-86b9-8a1f-c6af-a69be2b4a44c@gmx.de>
 <CAAhV-H44mDiP2DWATVyvKq6n8qj=sLhjHxzMo_+_zyggoGR1pw@mail.gmail.com>
From:   Helge Deller <deller@gmx.de>
In-Reply-To: <CAAhV-H44mDiP2DWATVyvKq6n8qj=sLhjHxzMo_+_zyggoGR1pw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:jwSKsw9udR8=;isZiHDrDsLZ+ghtLWD50WoaAMiq
 MIFFEm3WzN/wzycBvj7khuNe4Qf76G7XCkmGXDy2VzJ9jGJLfR7HsHpq3uDd9ftWfjT7pIX7j
 1fj3jAwenj0BsADeERrVWq/zb9m9/H8SNgujWzjbEOgSOK0JGxHUGD6azHxw/QAHlbjgmLdzx
 2gfkqnVQMkk/ygfX0gyGYKMX9Q9u1k0upi54GrCkOHi5WETiMrpgc7oiT9/a9n29BDX5Rd41r
 MzLKuyTJmkTPhdWPIbEKWCwzKv1k+FdZkifM0wuSgXZRW+/m5fPs1AWD8oyG+IoBVVIEXmt9S
 XK/F+7iwQ06JG6KUX+rl+OsK5anNvZ6DvTZyLDcFL9NeG9AywkEsumLS2cRyEzXWBvaCv9Heg
 iNWqRZn9DxP44GJsaBM0plV60JMe7jU6nX3AFCRXEVQ/WSL/XFRS78ex/Jpl/UB8RWDaWwGRg
 Xpw/ozcyrTHszc2vpbfDIE1YhoYFZrRs4oyRB3ji/oH46LAqJZhZQH63YEcFnXgBaiNSfBsQ2
 Xj7u4K9YCgAcT8bQuKkr98hJniHeCPz9VNpZ+mZChzU5jQzise9WRIxRL+PltTUFajiYE6g0W
 piQTc4STxlRGES7mrzQZLSC61LDaT7UWdszcBNxgaZA/wd5QugTHK4JEDBp+VV71yvyiedMe7
 D3e5Fmu9TdgmEFQhqr5rm0ky3Vhr8nS9U2WLjeke3z6VtL9SRP/ZvfzM6emwQ7rFnpz9jSOwN
 JxfmnKMbpC9UZ9sFRzPhLiwq8g1vU+mS16h12Gl2JLE+F7whouS8hFKcquOpL2vYvazUMhHZf
 P6jkVhf4j5KN5NXpDiVZHyWWARVhkyyY8ttHL7yy+ftxo8cKvEbJc56H6YhGi68Acc1j2OEmP
 W5ZIXawASJ2PdDzjDqBM3/mxxSTwPUSaKVyk1wbC52dgC8p6vE+YSQHgSsPuDeaIKrKq6MhG1
 QNRWzw==
Precedence: bulk

On 9/15/23 11:23, Huacai Chen wrote:
> On Fri, Sep 15, 2023 at 4:16=E2=80=AFPM Helge Deller <deller@gmx.de> wro=
te:
>>
>> On 9/15/23 05:22, Huacai Chen wrote:
>>> Hi Helge,
>>>
>>> On Wed, Sep 13, 2023 at 3:18=E2=80=AFAM Helge Deller <deller@gmx.de> w=
rote:
>>>>
>>>> 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.ld=
s.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 i=
s 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 k=
eeps
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) =3D=3D 0, "Unexpected run-time procedure link=
ages 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 wh=
at 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 sectio=
n
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/ker=
nel/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
>>>>           . =3D ALIGN(PECOFF_SEGMENT_ALIGN);
>>>>           _etext =3D .;
>>>>
>>>> -       /*
>>>> -        * struct alt_inst entries. From the header (alternative.h):
>>>> -        * "Alternative instructions for different CPU types or capab=
ilities"
>>>> -        * Think locking instructions on spinlocks.
>>>> -        */
>>>> -       . =3D ALIGN(4);
>>>> -       .altinstructions : AT(ADDR(.altinstructions) - LOAD_OFFSET) {
>>>> -               __alt_instructions =3D .;
>>>> -               *(.altinstructions)
>>>> -               __alt_instructions_end =3D .;
>>>> -       }
>>>> -
>>>> -#ifdef CONFIG_RELOCATABLE
>>>> -       . =3D ALIGN(8);
>>>> -       .la_abs : AT(ADDR(.la_abs) - LOAD_OFFSET) {
>>>> -               __la_abs_begin =3D .;
>>>> -               *(.la_abs)
>>>> -               __la_abs_end =3D .;
>>>> -       }
>>>> -#endif
>>>> -
>>>> -       .got : ALIGN(16) { *(.got) }
>>>> -       .plt : ALIGN(16) { *(.plt) }
>>>> -       .got.plt : ALIGN(16) { *(.got.plt) }
>>>> -
>>>> -       .data.rel : { *(.data.rel*) }
>>>> -
>>>>           . =3D ALIGN(PECOFF_SEGMENT_ALIGN);
>>>>           __init_begin =3D .;
>>>>           __inittext_begin =3D .;
>>>> @@ -94,6 +67,18 @@ SECTIONS
>>>>
>>>>           __initdata_begin =3D .;
>>>>
>>>> +       /*
>>>> +        * struct alt_inst entries. From the header (alternative.h):
>>>> +        * "Alternative instructions for different CPU types or capab=
ilities"
>>>> +        * Think locking instructions on spinlocks.
>>>> +        */
>>>> +       . =3D ALIGN(4);
>>>> +       .altinstructions : AT(ADDR(.altinstructions) - LOAD_OFFSET) {
>>>> +               __alt_instructions =3D .;
>>>> +               *(.altinstructions)
>>>> +               __alt_instructions_end =3D .;
>>>> +       }
>>>> +
>>>>           INIT_DATA_SECTION(16)
>>>>           .exit.data : {
>>>>                   EXIT_DATA
>>>> @@ -113,6 +98,11 @@ SECTIONS
>>>>
>>>>           _sdata =3D .;
>>>>           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 =3D .;
>>>>           }
>>>>
>>>> +       .data.rel : { *(.data.rel*) }
>>>> +
>>>> +#ifdef CONFIG_RELOCATABLE
>>>> +       . =3D ALIGN(8);
>>>> +       .la_abs : AT(ADDR(.la_abs) - LOAD_OFFSET) {
>>>> +               __la_abs_begin =3D .;
>>>> +               *(.la_abs)
>>>> +               __la_abs_end =3D .;
>>>> +       }
>>>> +#endif
>>>> +
>>>>           .sdata : {
>>>>                   *(.sdata)
>>>>           }
>>