diff mbox series

LoongArch: Fix lockdep static memory detection

Message ID ZQC5jS/Kc/JiBEOa@p100
State New
Headers
Series LoongArch: Fix lockdep static memory detection |

Commit Message

Helge Deller Sept. 12, 2023, 7:18 p.m. UTC 
  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,
- 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+

Comments

Guenter Roeck Sept. 12, 2023, 8:31 p.m. UTC | #1 
On Tue, Sep 12, 2023 at 09:18:37PM +0200, Helge Deller 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,
> - 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+

Tested-by: Guenter Roeck <linux@roeck-us.net>

> 
> 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)
>  	}
Helge Deller Sept. 14, 2023, 4:40 p.m. UTC | #2 
PING to Loongarch maintainers!

Without this patch, lockdep is broken on LoongArch on kernel v6.1 and above.
(patch below wrongly mentions kernel 6.4, but actually it needs backport to v6.1 too).

Helge

On 9/12/23 22:31, Guenter Roeck wrote:
> On Tue, Sep 12, 2023 at 09:18:37PM +0200, Helge Deller 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,
>> - 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+
>
> Tested-by: Guenter Roeck <linux@roeck-us.net>
>
>>
>> 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)
>>   	}
Huacai Chen Sept. 15, 2023, 3:22 a.m. UTC | #3 
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.

Huacai

> - 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)
>         }
Helge Deller Sept. 15, 2023, 8:16 a.m. UTC | #4 
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.

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)
>>          }
Huacai Chen Sept. 15, 2023, 9:23 a.m. UTC | #5 
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.

Huacai

>
> 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)
> >>          }
>
Helge Deller Sept. 15, 2023, 10:10 a.m. UTC | #6 
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)
>>>>           }
>>
Guenter Roeck Sept. 15, 2023, 2:07 p.m. UTC | #7 
Hi Helge,

On 9/15/23 03:10, Helge Deller wrote:
> 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.
> 

Just my $0.02 .. it might make sense to concentrate on the minimum to get the immediate
problem fixed. Loongarch maintainers can then decide at their own pace if they want
to apply any of the other changes you suggested. After all, unless I am missing
something, those additional changes are not really needed in stable releases.

Thanks,
Guenter
WANG Xuerui Sept. 15, 2023, 2:19 p.m. UTC | #8 
Hi,

On 9/15/23 22:07, Guenter Roeck wrote:
> Hi Helge,
>
> On 9/15/23 03:10, Helge Deller wrote:
>> 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.
>>
>
> Just my $0.02 .. it might make sense to concentrate on the minimum to 
> get the immediate
> problem fixed. Loongarch maintainers can then decide at their own pace 
> if they want
> to apply any of the other changes you suggested. After all, unless I 
> am missing
> something, those additional changes are not really needed in stable 
> releases.

Sorry for coming late, but as reviewer of arch/loongarch, I'd agree with 
Guenter and Helge here: let's fix the immediate problem and investigate 
the rest later -- it's not like the problems are *definitely* orthogonal 
in this case, and at least *some* progress would be appreciated.

I'll try to reproduce the problem and test the fix during the weekend, 
so hopefully Huacai can get the fix in before -rc2 or -rc3. Thanks for 
the attention and fix.
Huacai Chen Sept. 15, 2023, 3:17 p.m. UTC | #9 
On Fri, Sep 15, 2023 at 10:19 PM WANG Xuerui <kernel@xen0n.name> wrote:
>
> Hi,
>
> On 9/15/23 22:07, Guenter Roeck wrote:
> > Hi Helge,
> >
> > On 9/15/23 03:10, Helge Deller wrote:
> >> 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.
> >>
> >
> > Just my $0.02 .. it might make sense to concentrate on the minimum to
> > get the immediate
> > problem fixed. Loongarch maintainers can then decide at their own pace
> > if they want
> > to apply any of the other changes you suggested. After all, unless I
> > am missing
> > something, those additional changes are not really needed in stable
> > releases.
>
> Sorry for coming late, but as reviewer of arch/loongarch, I'd agree with
> Guenter and Helge here: let's fix the immediate problem and investigate
> the rest later -- it's not like the problems are *definitely* orthogonal
> in this case, and at least *some* progress would be appreciated.
>
> I'll try to reproduce the problem and test the fix during the weekend,
> so hopefully Huacai can get the fix in before -rc2 or -rc3. Thanks for
> the attention and fix.
If all changes are OK, I have no objection to putting them in a single patch.

Huacai

>
> --
> WANG "xen0n" Xuerui
>
> Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/
>
>
diff mbox series

Patch

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)
 	}