| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e7965d97-48cd-b2ed-328f-c1947121c8e7@roeck-us.net>
Date: Fri, 8 Sep 2023 13:52:04 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Helge Deller <deller@....de>, Huacai Chen <chenhuacai@...nel.org>,
loongarch@...ts.linux.dev
Cc: linux-kernel@...r.kernel.org,
"Rafael J. Wysocki" <rafael@...nel.org>,
Borislav Petkov <bp@...e.de>,
Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] lockdep: Fix static memory detection even more
On 9/7/23 00:40, Helge Deller wrote:
> * Guenter Roeck <linux@...ck-us.net>:
>> On 9/6/23 00:18, Helge Deller wrote:
>>> * Guenter Roeck <linux@...ck-us.net>:
>>>> On 9/3/23 14:11, Helge Deller wrote:
>>>>> * Guenter Roeck <linux@...ck-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@....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)
> }
>
Powered by blists - more mailing lists