Re: [PATCH] lockdep: Fix static memory detection even more

[Guenter Roeck <linux@roeck-us.net>]

On 9/7/23 00:40, Helge Deller wrote:
> * Guenter Roeck <linux@roeck-us.net>:
>> On 9/6/23 00:18, Helge Deller wrote:
>>> * Guenter Roeck <linux@roeck-us.net>:
>>>> On 9/3/23 14:11, Helge Deller wrote:
>>>>> * Guenter Roeck <linux@roeck-us.net>:
>>>>>> Hi,
>>>>>>
>>>>>> On Sat, Aug 12, 2023 at 05:48:52PM +0200, Helge Deller wrote:
>>>>>>> On the parisc architecture, lockdep reports for all static objects which
>>>>>>> are in the __initdata section (e.g. "setup_done" in devtmpfs,
>>>>>>> "kthreadd_done" in init/main.c) this warning:
>>>>>>>
>>>>>>> 	INFO: trying to register non-static key.
>>>>>>>
>>>>>>> The warning itself is wrong, because those objects are in the __initdata
>>>>>>> section, but the section itself is on parisc outside of range from
>>>>>>> _stext to _end, which is why the static_obj() functions returns a wrong
>>>>>>> answer.
>>>>>>>
>>>>>>> While fixing this issue, I noticed that the whole existing check can
>>>>>>> be simplified a lot.
>>>>>>> Instead of checking against the _stext and _end symbols (which include
>>>>>>> code areas too) just check for the .data and .bss segments (since we check a
>>>>>>> data object). This can be done with the existing is_kernel_core_data()
>>>>>>> macro.
>>>>>>>
>>>>>>> In addition objects in the __initdata section can be checked with
>>>>>>> init_section_contains().
>>>>>>>
>>>>>>> This partly reverts and simplifies commit bac59d18c701 ("x86/setup: Fix static
>>>>>>> memory detection").
>>>>>>>
>>>>>>> Tested on x86-64 and parisc.
>>>>>>>
>>>>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>>>>> Fixes: bac59d18c701 ("x86/setup: Fix static memory detection")
>>>>>>
>>>>>> On loongarch, this patch results in the following backtrace.
>>>>>>
>>>>>> EFI stub: Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path
>>>>>> EFI stub: Exiting boot services
>>>>>> [    0.000000] INFO: trying to register non-static key.
>>>>>> [    0.000000] The code is fine but needs lockdep annotation, or maybe
>>>>>> [    0.000000] you didn't initialize this object before use?
>>>>>> [    0.000000] turning off the locking correctness validator.
>>>>>> [    0.000000]         ...
>>>>>> [    0.000000] Call Trace:
>>>>>> [    0.000000] [<9000000000223d84>] show_stack+0x5c/0x180
>>>>>> [    0.000000] [<900000000153e0b4>] dump_stack_lvl+0x88/0xd0
>>>>>> [    0.000000] [<90000000002bc548>] register_lock_class+0x768/0x770
>>>>>> [    0.000000] [<90000000002bc710>] __lock_acquire+0xb0/0x2a18
>>>>>> [    0.000000] [<90000000002bba1c>] lock_acquire+0x11c/0x328
>>>>>> [    0.000000] [<9000000000b34a60>] __debug_object_init+0x60/0x244
>>>>>> [    0.000000] [<9000000000337f94>] init_cgroup_housekeeping+0xe8/0x144
>>>>>> [    0.000000] [<900000000033e364>] init_cgroup_root+0x38/0xa0
>>>>>> [    0.000000] [<90000000017801ac>] cgroup_init_early+0x44/0x16c
>>>>>> [    0.000000] [<9000000001770758>] start_kernel+0x50/0x624
>>>>>> [    0.000000] [<90000000015410b4>] kernel_entry+0xb4/0xc4
>>>>>>
>>>>>> Reverting it fixes the problem. Bisect log attached.
>>>>>>
>>>>>> This is also seen in v6.5.y and v6.4.y since the patch has been applied
>>>>>> to those branches.
>>>>>
>>>>> Does this happens with CONFIG_SMP=n ?
>>>>> If so, I think the untested patch below might fix the issue.
>>>>>
>>>>
>>>> No, this is loongarch:defconfig with various debug options enabled.
>>>> That has CONFIG_SMP=y.
>>>
>>> Could you apply below patch and verify with the contents of the
>>> System.map file where the lock is located ?
>>>
>>> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
>>> index e85b5ad3e206..db0a301f9740 100644
>>> --- a/kernel/locking/lockdep.c
>>> +++ b/kernel/locking/lockdep.c
>>> @@ -969,7 +969,7 @@ static bool assign_lock_key(struct lockdep_map *lock)
>>>    	else {
>>>    		/* Debug-check: all keys must be persistent! */
>>>    		debug_locks_off();
>>> -		pr_err("INFO: trying to register non-static key.\n");
>>> +		pr_err("INFO: trying to register non-static key at %08lx.\n", addr);
>>>    		pr_err("The code is fine but needs lockdep annotation, or maybe\n");
>>>    		pr_err("you didn't initialize this object before use?\n");
>>>    		pr_err("turning off the locking correctness validator.\n");
>>
>> 90000000015602d0 D __la_abs_end
>> ...
>> 90000000016815c0 d fill_pool_map.3  <--- lock pointer
>> ...
>> 9000000001770000 T __init_begin
> 
> The problem is, that loongarch's  vmlinux.lds.S file puts data into
> areas which are not marked "data" or "code".
> 
> can you try below patch?
> The relevant part is here the move of ".data.rel : { *(.data.rel*) }"
> into the data section.
> The other parts are suggestions.
> 

The patch below fixes the problem for me.

Guenter

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