[v2] powerpc/lockdep: fix a false positive warning
diff mbox series

Message ID 20190906231754.830-1-cai@lca.pw
State Not Applicable
Headers show
Series
  • [v2] powerpc/lockdep: fix a false positive warning
Related show

Commit Message

Qian Cai Sept. 6, 2019, 11:17 p.m. UTC
The commit 108c14858b9e ("locking/lockdep: Add support for dynamic
keys") introduced a boot warning on powerpc below, because since the
commit 2d4f567103ff ("KVM: PPC: Introduce kvm_tmp framework") adds
kvm_tmp[] into the .bss section and then free the rest of unused spaces
back to the page allocator.

kernel_init
  kvm_guest_init
    kvm_free_tmp
      free_reserved_area
        free_unref_page
          free_unref_page_prepare

Later, alloc_workqueue() happens to allocate some pages from there and
trigger the warning at,

if (WARN_ON_ONCE(static_obj(key)))

Fix it by adding a generic helper arch_is_bss_hole() to skip those areas
in static_obj(). Since kvm_free_tmp() is only done early during the
boot, just go lockless to make the implementation simple for now.

WARNING: CPU: 0 PID: 13 at kernel/locking/lockdep.c:1120
Workqueue: events work_for_cpu_fn
Call Trace:
  lockdep_register_key+0x68/0x200
  wq_init_lockdep+0x40/0xc0
  trunc_msg+0x385f9/0x4c30f (unreliable)
  wq_init_lockdep+0x40/0xc0
  alloc_workqueue+0x1e0/0x620
  scsi_host_alloc+0x3d8/0x490
  ata_scsi_add_hosts+0xd0/0x220 [libata]
  ata_host_register+0x178/0x400 [libata]
  ata_host_activate+0x17c/0x210 [libata]
  ahci_host_activate+0x84/0x250 [libahci]
  ahci_init_one+0xc74/0xdc0 [ahci]
  local_pci_probe+0x78/0x100
  work_for_cpu_fn+0x40/0x70
  process_one_work+0x388/0x750
  process_scheduled_works+0x50/0x90
  worker_thread+0x3d0/0x570
  kthread+0x1b8/0x1e0
  ret_from_kernel_thread+0x5c/0x7c

Fixes: 108c14858b9e ("locking/lockdep: Add support for dynamic keys")
Signed-off-by: Qian Cai <cai@lca.pw>
---

v2: No need to actually define arch_is_bss_hole() powerpc64 only.

 arch/powerpc/include/asm/sections.h | 11 +++++++++++
 arch/powerpc/kernel/kvm.c           |  5 +++++
 include/asm-generic/sections.h      |  7 +++++++
 kernel/locking/lockdep.c            |  3 +++
 4 files changed, 26 insertions(+)

Comments

Ingo Molnar Sept. 7, 2019, 7:05 a.m. UTC | #1
* Qian Cai <cai@lca.pw> wrote:

> The commit 108c14858b9e ("locking/lockdep: Add support for dynamic
> keys") introduced a boot warning on powerpc below, because since the
> commit 2d4f567103ff ("KVM: PPC: Introduce kvm_tmp framework") adds
> kvm_tmp[] into the .bss section and then free the rest of unused spaces
> back to the page allocator.
> 
> kernel_init
>   kvm_guest_init
>     kvm_free_tmp
>       free_reserved_area
>         free_unref_page
>           free_unref_page_prepare
> 
> Later, alloc_workqueue() happens to allocate some pages from there and
> trigger the warning at,
> 
> if (WARN_ON_ONCE(static_obj(key)))
> 
> Fix it by adding a generic helper arch_is_bss_hole() to skip those areas
> in static_obj(). Since kvm_free_tmp() is only done early during the
> boot, just go lockless to make the implementation simple for now.
> 
> WARNING: CPU: 0 PID: 13 at kernel/locking/lockdep.c:1120
> Workqueue: events work_for_cpu_fn
> Call Trace:
>   lockdep_register_key+0x68/0x200
>   wq_init_lockdep+0x40/0xc0
>   trunc_msg+0x385f9/0x4c30f (unreliable)
>   wq_init_lockdep+0x40/0xc0
>   alloc_workqueue+0x1e0/0x620
>   scsi_host_alloc+0x3d8/0x490
>   ata_scsi_add_hosts+0xd0/0x220 [libata]
>   ata_host_register+0x178/0x400 [libata]
>   ata_host_activate+0x17c/0x210 [libata]
>   ahci_host_activate+0x84/0x250 [libahci]
>   ahci_init_one+0xc74/0xdc0 [ahci]
>   local_pci_probe+0x78/0x100
>   work_for_cpu_fn+0x40/0x70
>   process_one_work+0x388/0x750
>   process_scheduled_works+0x50/0x90
>   worker_thread+0x3d0/0x570
>   kthread+0x1b8/0x1e0
>   ret_from_kernel_thread+0x5c/0x7c
> 
> Fixes: 108c14858b9e ("locking/lockdep: Add support for dynamic keys")
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
> 
> v2: No need to actually define arch_is_bss_hole() powerpc64 only.
> 
>  arch/powerpc/include/asm/sections.h | 11 +++++++++++
>  arch/powerpc/kernel/kvm.c           |  5 +++++
>  include/asm-generic/sections.h      |  7 +++++++
>  kernel/locking/lockdep.c            |  3 +++
>  4 files changed, 26 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
> index 4a1664a8658d..4f5d69c42017 100644
> --- a/arch/powerpc/include/asm/sections.h
> +++ b/arch/powerpc/include/asm/sections.h
> @@ -5,8 +5,19 @@
>  
>  #include <linux/elf.h>
>  #include <linux/uaccess.h>
> +
> +#define arch_is_bss_hole arch_is_bss_hole
> +
>  #include <asm-generic/sections.h>
>  
> +extern void *bss_hole_start, *bss_hole_end;
> +
> +static inline int arch_is_bss_hole(unsigned long addr)
> +{
> +	return addr >= (unsigned long)bss_hole_start &&
> +	       addr < (unsigned long)bss_hole_end;
> +}
> +
>  extern char __head_end[];
>  
>  #ifdef __powerpc64__
> diff --git a/arch/powerpc/kernel/kvm.c b/arch/powerpc/kernel/kvm.c
> index b7b3a5e4e224..89e0e522e125 100644
> --- a/arch/powerpc/kernel/kvm.c
> +++ b/arch/powerpc/kernel/kvm.c
> @@ -66,6 +66,7 @@
>  static bool kvm_patching_worked = true;
>  char kvm_tmp[1024 * 1024];
>  static int kvm_tmp_index;
> +void *bss_hole_start, *bss_hole_end;
>  
>  static inline void kvm_patch_ins(u32 *inst, u32 new_inst)
>  {
> @@ -707,6 +708,10 @@ static __init void kvm_free_tmp(void)
>  	 */
>  	kmemleak_free_part(&kvm_tmp[kvm_tmp_index],
>  			   ARRAY_SIZE(kvm_tmp) - kvm_tmp_index);
> +
> +	bss_hole_start = &kvm_tmp[kvm_tmp_index];
> +	bss_hole_end = &kvm_tmp[ARRAY_SIZE(kvm_tmp)];
> +
>  	free_reserved_area(&kvm_tmp[kvm_tmp_index],
>  			   &kvm_tmp[ARRAY_SIZE(kvm_tmp)], -1, NULL);
>  }
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index d1779d442aa5..4d8b1f2c5fd9 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -91,6 +91,13 @@ static inline int arch_is_kernel_initmem_freed(unsigned long addr)
>  }
>  #endif
>  
> +#ifndef arch_is_bss_hole
> +static inline int arch_is_bss_hole(unsigned long addr)
> +{
> +	return 0;
> +}
> +#endif
> +
>  /**
>   * memory_contains - checks if an object is contained within a memory region
>   * @begin: virtual address of the beginning of the memory region
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 4861cf8e274b..cd75b51f15ce 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -675,6 +675,9 @@ static int static_obj(const void *obj)
>  	if (arch_is_kernel_initmem_freed(addr))
>  		return 0;
>  
> +	if (arch_is_bss_hole(addr))
> +		return 0;

arch_is_bss_hole() should use a 'bool' - but other than that, this 
looks good to me, if the PowerPC maintainers agree too.

Thanks,

	Ingo
Qian Cai Sept. 7, 2019, 11:35 a.m. UTC | #2
> On Sep 7, 2019, at 3:05 AM, Ingo Molnar <mingo@kernel.org> wrote:
> 
> 
> * Qian Cai <cai@lca.pw> wrote:
> 
>> The commit 108c14858b9e ("locking/lockdep: Add support for dynamic
>> keys") introduced a boot warning on powerpc below, because since the
>> commit 2d4f567103ff ("KVM: PPC: Introduce kvm_tmp framework") adds
>> kvm_tmp[] into the .bss section and then free the rest of unused spaces
>> back to the page allocator.
>> 
>> kernel_init
>>  kvm_guest_init
>>    kvm_free_tmp
>>      free_reserved_area
>>        free_unref_page
>>          free_unref_page_prepare
>> 
>> Later, alloc_workqueue() happens to allocate some pages from there and
>> trigger the warning at,
>> 
>> if (WARN_ON_ONCE(static_obj(key)))
>> 
>> Fix it by adding a generic helper arch_is_bss_hole() to skip those areas
>> in static_obj(). Since kvm_free_tmp() is only done early during the
>> boot, just go lockless to make the implementation simple for now.
>> 
>> WARNING: CPU: 0 PID: 13 at kernel/locking/lockdep.c:1120
>> Workqueue: events work_for_cpu_fn
>> Call Trace:
>>  lockdep_register_key+0x68/0x200
>>  wq_init_lockdep+0x40/0xc0
>>  trunc_msg+0x385f9/0x4c30f (unreliable)
>>  wq_init_lockdep+0x40/0xc0
>>  alloc_workqueue+0x1e0/0x620
>>  scsi_host_alloc+0x3d8/0x490
>>  ata_scsi_add_hosts+0xd0/0x220 [libata]
>>  ata_host_register+0x178/0x400 [libata]
>>  ata_host_activate+0x17c/0x210 [libata]
>>  ahci_host_activate+0x84/0x250 [libahci]
>>  ahci_init_one+0xc74/0xdc0 [ahci]
>>  local_pci_probe+0x78/0x100
>>  work_for_cpu_fn+0x40/0x70
>>  process_one_work+0x388/0x750
>>  process_scheduled_works+0x50/0x90
>>  worker_thread+0x3d0/0x570
>>  kthread+0x1b8/0x1e0
>>  ret_from_kernel_thread+0x5c/0x7c
>> 
>> Fixes: 108c14858b9e ("locking/lockdep: Add support for dynamic keys")
>> Signed-off-by: Qian Cai <cai@lca.pw>
>> ---
>> 
>> v2: No need to actually define arch_is_bss_hole() powerpc64 only.
>> 
>> arch/powerpc/include/asm/sections.h | 11 +++++++++++
>> arch/powerpc/kernel/kvm.c           |  5 +++++
>> include/asm-generic/sections.h      |  7 +++++++
>> kernel/locking/lockdep.c            |  3 +++
>> 4 files changed, 26 insertions(+)
>> 
>> diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
>> index 4a1664a8658d..4f5d69c42017 100644
>> --- a/arch/powerpc/include/asm/sections.h
>> +++ b/arch/powerpc/include/asm/sections.h
>> @@ -5,8 +5,19 @@
>> 
>> #include <linux/elf.h>
>> #include <linux/uaccess.h>
>> +
>> +#define arch_is_bss_hole arch_is_bss_hole
>> +
>> #include <asm-generic/sections.h>
>> 
>> +extern void *bss_hole_start, *bss_hole_end;
>> +
>> +static inline int arch_is_bss_hole(unsigned long addr)
>> +{
>> +	return addr >= (unsigned long)bss_hole_start &&
>> +	       addr < (unsigned long)bss_hole_end;
>> +}
>> +
>> extern char __head_end[];
>> 
>> #ifdef __powerpc64__
>> diff --git a/arch/powerpc/kernel/kvm.c b/arch/powerpc/kernel/kvm.c
>> index b7b3a5e4e224..89e0e522e125 100644
>> --- a/arch/powerpc/kernel/kvm.c
>> +++ b/arch/powerpc/kernel/kvm.c
>> @@ -66,6 +66,7 @@
>> static bool kvm_patching_worked = true;
>> char kvm_tmp[1024 * 1024];
>> static int kvm_tmp_index;
>> +void *bss_hole_start, *bss_hole_end;
>> 
>> static inline void kvm_patch_ins(u32 *inst, u32 new_inst)
>> {
>> @@ -707,6 +708,10 @@ static __init void kvm_free_tmp(void)
>> 	 */
>> 	kmemleak_free_part(&kvm_tmp[kvm_tmp_index],
>> 			   ARRAY_SIZE(kvm_tmp) - kvm_tmp_index);
>> +
>> +	bss_hole_start = &kvm_tmp[kvm_tmp_index];
>> +	bss_hole_end = &kvm_tmp[ARRAY_SIZE(kvm_tmp)];
>> +
>> 	free_reserved_area(&kvm_tmp[kvm_tmp_index],
>> 			   &kvm_tmp[ARRAY_SIZE(kvm_tmp)], -1, NULL);
>> }
>> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
>> index d1779d442aa5..4d8b1f2c5fd9 100644
>> --- a/include/asm-generic/sections.h
>> +++ b/include/asm-generic/sections.h
>> @@ -91,6 +91,13 @@ static inline int arch_is_kernel_initmem_freed(unsigned long addr)
>> }
>> #endif
>> 
>> +#ifndef arch_is_bss_hole
>> +static inline int arch_is_bss_hole(unsigned long addr)
>> +{
>> +	return 0;
>> +}
>> +#endif
>> +
>> /**
>>  * memory_contains - checks if an object is contained within a memory region
>>  * @begin: virtual address of the beginning of the memory region
>> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
>> index 4861cf8e274b..cd75b51f15ce 100644
>> --- a/kernel/locking/lockdep.c
>> +++ b/kernel/locking/lockdep.c
>> @@ -675,6 +675,9 @@ static int static_obj(const void *obj)
>> 	if (arch_is_kernel_initmem_freed(addr))
>> 		return 0;
>> 
>> +	if (arch_is_bss_hole(addr))
>> +		return 0;
> 
> arch_is_bss_hole() should use a 'bool' - but other than that, this 
> looks good to me, if the PowerPC maintainers agree too.

I thought about making it a bool in the first place, but since all other similar helpers
(arch_is_kernel_initmem_freed(), arch_is_kernel_text(), arch_is_kernel_data() etc)
could be bool too but are not, I kept arch_is_bss_hole() just to be “int” for consistent.

Although then there is is_kernel_rodata() which is bool. I suppose I’ll change
arch_is_bss_hole() to bool, and then could have a follow-up patch to covert all similar
helpers to return boo instead.
Ingo Molnar Sept. 8, 2019, 8:32 a.m. UTC | #3
* Qian Cai <cai@lca.pw> wrote:

> I thought about making it a bool in the first place, but since all 
> other similar helpers (arch_is_kernel_initmem_freed(), 
> arch_is_kernel_text(), arch_is_kernel_data() etc) could be bool too but 
> are not, I kept arch_is_bss_hole() just to be “int” for consistent.
> 
> Although then there is is_kernel_rodata() which is bool. I suppose I’ll 
> change arch_is_bss_hole() to bool, and then could have a follow-up 
> patch to covert all similar helpers to return boo instead.

Sounds good to me.

Thanks,

	Ingo

Patch
diff mbox series

diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
index 4a1664a8658d..4f5d69c42017 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h
@@ -5,8 +5,19 @@ 
 
 #include <linux/elf.h>
 #include <linux/uaccess.h>
+
+#define arch_is_bss_hole arch_is_bss_hole
+
 #include <asm-generic/sections.h>
 
+extern void *bss_hole_start, *bss_hole_end;
+
+static inline int arch_is_bss_hole(unsigned long addr)
+{
+	return addr >= (unsigned long)bss_hole_start &&
+	       addr < (unsigned long)bss_hole_end;
+}
+
 extern char __head_end[];
 
 #ifdef __powerpc64__
diff --git a/arch/powerpc/kernel/kvm.c b/arch/powerpc/kernel/kvm.c
index b7b3a5e4e224..89e0e522e125 100644
--- a/arch/powerpc/kernel/kvm.c
+++ b/arch/powerpc/kernel/kvm.c
@@ -66,6 +66,7 @@ 
 static bool kvm_patching_worked = true;
 char kvm_tmp[1024 * 1024];
 static int kvm_tmp_index;
+void *bss_hole_start, *bss_hole_end;
 
 static inline void kvm_patch_ins(u32 *inst, u32 new_inst)
 {
@@ -707,6 +708,10 @@  static __init void kvm_free_tmp(void)
 	 */
 	kmemleak_free_part(&kvm_tmp[kvm_tmp_index],
 			   ARRAY_SIZE(kvm_tmp) - kvm_tmp_index);
+
+	bss_hole_start = &kvm_tmp[kvm_tmp_index];
+	bss_hole_end = &kvm_tmp[ARRAY_SIZE(kvm_tmp)];
+
 	free_reserved_area(&kvm_tmp[kvm_tmp_index],
 			   &kvm_tmp[ARRAY_SIZE(kvm_tmp)], -1, NULL);
 }
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index d1779d442aa5..4d8b1f2c5fd9 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -91,6 +91,13 @@  static inline int arch_is_kernel_initmem_freed(unsigned long addr)
 }
 #endif
 
+#ifndef arch_is_bss_hole
+static inline int arch_is_bss_hole(unsigned long addr)
+{
+	return 0;
+}
+#endif
+
 /**
  * memory_contains - checks if an object is contained within a memory region
  * @begin: virtual address of the beginning of the memory region
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 4861cf8e274b..cd75b51f15ce 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -675,6 +675,9 @@  static int static_obj(const void *obj)
 	if (arch_is_kernel_initmem_freed(addr))
 		return 0;
 
+	if (arch_is_bss_hole(addr))
+		return 0;
+
 	/*
 	 * static variable?
 	 */