1. Patchew
  2. linux
  3. [PATCH 0/2] KHO Fixes
  4. View patch

[PATCH 2/2] KHO: init new_physxa->phys_bits to fix lockdep

Changyuan Lyu posted 2 patches 7 months ago
[PATCH 2/2] KHO: init new_physxa->phys_bits to fix lockdep
Posted by Changyuan Lyu 7 months ago 
From: Pasha Tatashin <pasha.tatashin@soleen.com>

Lockdep shows the following warning:

INFO: trying to register non-static key.
The code is fine but needs lockdep annotation, or maybe
you didn't initialize this object before use?
turning off the locking correctness validator.

[<ffffffff810133a6>] dump_stack_lvl+0x66/0xa0
[<ffffffff8136012c>] assign_lock_key+0x10c/0x120
[<ffffffff81358bb4>] register_lock_class+0xf4/0x2f0
[<ffffffff813597ff>] __lock_acquire+0x7f/0x2c40
[<ffffffff81360cb0>] ? __pfx_hlock_conflict+0x10/0x10
[<ffffffff811707be>] ? native_flush_tlb_global+0x8e/0xa0
[<ffffffff8117096e>] ? __flush_tlb_all+0x4e/0xa0
[<ffffffff81172fc2>] ? __kernel_map_pages+0x112/0x140
[<ffffffff813ec327>] ? xa_load_or_alloc+0x67/0xe0
[<ffffffff81359556>] lock_acquire+0xe6/0x280
[<ffffffff813ec327>] ? xa_load_or_alloc+0x67/0xe0
[<ffffffff8100b9e0>] _raw_spin_lock+0x30/0x40
[<ffffffff813ec327>] ? xa_load_or_alloc+0x67/0xe0
[<ffffffff813ec327>] xa_load_or_alloc+0x67/0xe0
[<ffffffff813eb4c0>] kho_preserve_folio+0x90/0x100
[<ffffffff813ebb7f>] __kho_finalize+0xcf/0x400
[<ffffffff813ebef4>] kho_finalize+0x34/0x70

This is becase xa has its own lock, that is not initialized in
xa_load_or_alloc.

Modifiy __kho_preserve_order(), to properly call
xa_init(&new_physxa->phys_bits);

Fixes: fc33e4b44b27 ("kexec: enable KHO support for memory preservation")
Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
Signed-off-by: Changyuan Lyu <changyuanl@google.com>
---
 kernel/kexec_handover.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/kernel/kexec_handover.c b/kernel/kexec_handover.c
index 69b953551677..f0ac6a9170f8 100644
--- a/kernel/kexec_handover.c
+++ b/kernel/kexec_handover.c
@@ -144,14 +144,35 @@ static int __kho_preserve_order(struct kho_mem_track *track, unsigned long pfn,
 				unsigned int order)
 {
 	struct kho_mem_phys_bits *bits;
-	struct kho_mem_phys *physxa;
+	struct kho_mem_phys *physxa, *new_physxa;
 	const unsigned long pfn_high = pfn >> order;

 	might_sleep();

-	physxa = xa_load_or_alloc(&track->orders, order, sizeof(*physxa));
-	if (IS_ERR(physxa))
-		return PTR_ERR(physxa);
+	physxa = xa_load(&track->orders, order);
+	if (!physxa) {
+		new_physxa = kzalloc(sizeof(*physxa), GFP_KERNEL);
+		if (!new_physxa)
+			return -ENOMEM;
+
+		xa_init(&new_physxa->phys_bits);
+		physxa = xa_cmpxchg(&track->orders, order, NULL, new_physxa,
+				    GFP_KERNEL);
+		if (xa_is_err(physxa)) {
+			int err_ret = xa_err(physxa);
+
+			xa_destroy(&new_physxa->phys_bits);
+			kfree(new_physxa);
+
+			return err_ret;
+		}
+		if (physxa) {
+			xa_destroy(&new_physxa->phys_bits);
+			kfree(new_physxa);
+		} else {
+			physxa = new_physxa;
+		}
+	}

 	bits = xa_load_or_alloc(&physxa->phys_bits, pfn_high / PRESERVE_BITS,
 				sizeof(*bits));
--
2.49.0.1101.gccaa498523-goog
Re: [PATCH 2/2] KHO: init new_physxa->phys_bits to fix lockdep
Posted by Mike Rapoport 7 months ago 
On Sun, May 18, 2025 at 07:23:15AM -0700, Changyuan Lyu wrote:
> From: Pasha Tatashin <pasha.tatashin@soleen.com>
> 
> Lockdep shows the following warning:
> 
> INFO: trying to register non-static key.
> The code is fine but needs lockdep annotation, or maybe
> you didn't initialize this object before use?
> turning off the locking correctness validator.
> 
> [<ffffffff810133a6>] dump_stack_lvl+0x66/0xa0
> [<ffffffff8136012c>] assign_lock_key+0x10c/0x120
> [<ffffffff81358bb4>] register_lock_class+0xf4/0x2f0
> [<ffffffff813597ff>] __lock_acquire+0x7f/0x2c40
> [<ffffffff81360cb0>] ? __pfx_hlock_conflict+0x10/0x10
> [<ffffffff811707be>] ? native_flush_tlb_global+0x8e/0xa0
> [<ffffffff8117096e>] ? __flush_tlb_all+0x4e/0xa0
> [<ffffffff81172fc2>] ? __kernel_map_pages+0x112/0x140
> [<ffffffff813ec327>] ? xa_load_or_alloc+0x67/0xe0
> [<ffffffff81359556>] lock_acquire+0xe6/0x280
> [<ffffffff813ec327>] ? xa_load_or_alloc+0x67/0xe0
> [<ffffffff8100b9e0>] _raw_spin_lock+0x30/0x40
> [<ffffffff813ec327>] ? xa_load_or_alloc+0x67/0xe0
> [<ffffffff813ec327>] xa_load_or_alloc+0x67/0xe0
> [<ffffffff813eb4c0>] kho_preserve_folio+0x90/0x100
> [<ffffffff813ebb7f>] __kho_finalize+0xcf/0x400
> [<ffffffff813ebef4>] kho_finalize+0x34/0x70
> 
> This is becase xa has its own lock, that is not initialized in
> xa_load_or_alloc.
> 
> Modifiy __kho_preserve_order(), to properly call
> xa_init(&new_physxa->phys_bits);
> 
> Fixes: fc33e4b44b27 ("kexec: enable KHO support for memory preservation")
> Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> Signed-off-by: Changyuan Lyu <changyuanl@google.com>
> ---
>  kernel/kexec_handover.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/kexec_handover.c b/kernel/kexec_handover.c
> index 69b953551677..f0ac6a9170f8 100644
> --- a/kernel/kexec_handover.c
> +++ b/kernel/kexec_handover.c
> @@ -144,14 +144,35 @@ static int __kho_preserve_order(struct kho_mem_track *track, unsigned long pfn,
>  				unsigned int order)
>  {
>  	struct kho_mem_phys_bits *bits;
> -	struct kho_mem_phys *physxa;
> +	struct kho_mem_phys *physxa, *new_physxa;
>  	const unsigned long pfn_high = pfn >> order;
> 
>  	might_sleep();
> 
> -	physxa = xa_load_or_alloc(&track->orders, order, sizeof(*physxa));
> -	if (IS_ERR(physxa))
> -		return PTR_ERR(physxa);
> +	physxa = xa_load(&track->orders, order);
> +	if (!physxa) {
> +		new_physxa = kzalloc(sizeof(*physxa), GFP_KERNEL);
> +		if (!new_physxa)
> +			return -ENOMEM;
> +
> +		xa_init(&new_physxa->phys_bits);
> +		physxa = xa_cmpxchg(&track->orders, order, NULL, new_physxa,
> +				    GFP_KERNEL);
> +		if (xa_is_err(physxa)) {
> +			int err_ret = xa_err(physxa);
> +
> +			xa_destroy(&new_physxa->phys_bits);
> +			kfree(new_physxa);
> +
> +			return err_ret;
> +		}
> +		if (physxa) {
> +			xa_destroy(&new_physxa->phys_bits);
> +			kfree(new_physxa);
> +		} else {
> +			physxa = new_physxa;
> +		}
> +	}

You are nearly duplicating xa_load_or_alloc() here. 
Is xa_destroy() is really needed here? In the end we destroying an empty
xarray.

Unless xa_destroy() is a must something like this would be simpler IMHO:

diff --git a/kernel/kexec_handover.c b/kernel/kexec_handover.c
index ef21db6c59d5..4c8303fbf97a 100644
--- a/kernel/kexec_handover.c
+++ b/kernel/kexec_handover.c
@@ -91,10 +91,12 @@ struct kho_serialization {
 	struct khoser_mem_chunk *preserved_mem_map;
 };
 
-static void *xa_load_or_alloc(struct xarray *xa, unsigned long index, size_t sz)
+static void *xa_load_or_alloc(struct xarray *xa, unsigned long index, size_t sz,
+			      bool *new)
 {
 	void *elm, *res;
 
+	*new = false;
 	elm = xa_load(xa, index);
 	if (elm)
 		return elm;
@@ -112,6 +114,7 @@ static void *xa_load_or_alloc(struct xarray *xa, unsigned long index, size_t sz)
 		return res;
 	}
 
+	*new = true;
 	return elm;
 }
 
@@ -146,15 +149,18 @@ static int __kho_preserve_order(struct kho_mem_track *track, unsigned long pfn,
 	struct kho_mem_phys_bits *bits;
 	struct kho_mem_phys *physxa;
 	const unsigned long pfn_high = pfn >> order;
+	bool new;
 
 	might_sleep();
 
-	physxa = xa_load_or_alloc(&track->orders, order, sizeof(*physxa));
+	physxa = xa_load_or_alloc(&track->orders, order, sizeof(*physxa), &new);
 	if (IS_ERR(physxa))
 		return PTR_ERR(physxa);
+	if (new)
+		xa_init(&physxa->phys_bits);
 
 	bits = xa_load_or_alloc(&physxa->phys_bits, pfn_high / PRESERVE_BITS,
-				sizeof(*bits));
+				sizeof(*bits), &new);
 	if (IS_ERR(bits))
 		return PTR_ERR(bits);
 
And if xa_destroy() is actually required, the allocation of new xarray
should be a helper function.

>  	bits = xa_load_or_alloc(&physxa->phys_bits, pfn_high / PRESERVE_BITS,
>  				sizeof(*bits));
> --
> 2.49.0.1101.gccaa498523-goog

-- 
Sincerely yours,
Mike.
Re: [PATCH 2/2] KHO: init new_physxa->phys_bits to fix lockdep
Posted by Pasha Tatashin 7 months ago 
On Sun, May 18, 2025 at 11:51 AM Mike Rapoport <rppt@kernel.org> wrote:
>
> On Sun, May 18, 2025 at 07:23:15AM -0700, Changyuan Lyu wrote:
> > From: Pasha Tatashin <pasha.tatashin@soleen.com>
> >
> > Lockdep shows the following warning:
> >
> > INFO: trying to register non-static key.
> > The code is fine but needs lockdep annotation, or maybe
> > you didn't initialize this object before use?
> > turning off the locking correctness validator.
> >
> > [<ffffffff810133a6>] dump_stack_lvl+0x66/0xa0
> > [<ffffffff8136012c>] assign_lock_key+0x10c/0x120
> > [<ffffffff81358bb4>] register_lock_class+0xf4/0x2f0
> > [<ffffffff813597ff>] __lock_acquire+0x7f/0x2c40
> > [<ffffffff81360cb0>] ? __pfx_hlock_conflict+0x10/0x10
> > [<ffffffff811707be>] ? native_flush_tlb_global+0x8e/0xa0
> > [<ffffffff8117096e>] ? __flush_tlb_all+0x4e/0xa0
> > [<ffffffff81172fc2>] ? __kernel_map_pages+0x112/0x140
> > [<ffffffff813ec327>] ? xa_load_or_alloc+0x67/0xe0
> > [<ffffffff81359556>] lock_acquire+0xe6/0x280
> > [<ffffffff813ec327>] ? xa_load_or_alloc+0x67/0xe0
> > [<ffffffff8100b9e0>] _raw_spin_lock+0x30/0x40
> > [<ffffffff813ec327>] ? xa_load_or_alloc+0x67/0xe0
> > [<ffffffff813ec327>] xa_load_or_alloc+0x67/0xe0
> > [<ffffffff813eb4c0>] kho_preserve_folio+0x90/0x100
> > [<ffffffff813ebb7f>] __kho_finalize+0xcf/0x400
> > [<ffffffff813ebef4>] kho_finalize+0x34/0x70
> >
> > This is becase xa has its own lock, that is not initialized in
> > xa_load_or_alloc.
> >
> > Modifiy __kho_preserve_order(), to properly call
> > xa_init(&new_physxa->phys_bits);
> >
> > Fixes: fc33e4b44b27 ("kexec: enable KHO support for memory preservation")
> > Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> > Signed-off-by: Changyuan Lyu <changyuanl@google.com>
> > ---
> >  kernel/kexec_handover.c | 29 +++++++++++++++++++++++++----
> >  1 file changed, 25 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/kexec_handover.c b/kernel/kexec_handover.c
> > index 69b953551677..f0ac6a9170f8 100644
> > --- a/kernel/kexec_handover.c
> > +++ b/kernel/kexec_handover.c
> > @@ -144,14 +144,35 @@ static int __kho_preserve_order(struct kho_mem_track *track, unsigned long pfn,
> >                               unsigned int order)
> >  {
> >       struct kho_mem_phys_bits *bits;
> > -     struct kho_mem_phys *physxa;
> > +     struct kho_mem_phys *physxa, *new_physxa;
> >       const unsigned long pfn_high = pfn >> order;
> >
> >       might_sleep();
> >
> > -     physxa = xa_load_or_alloc(&track->orders, order, sizeof(*physxa));
> > -     if (IS_ERR(physxa))
> > -             return PTR_ERR(physxa);
> > +     physxa = xa_load(&track->orders, order);
> > +     if (!physxa) {
> > +             new_physxa = kzalloc(sizeof(*physxa), GFP_KERNEL);
> > +             if (!new_physxa)
> > +                     return -ENOMEM;
> > +
> > +             xa_init(&new_physxa->phys_bits);
> > +             physxa = xa_cmpxchg(&track->orders, order, NULL, new_physxa,
> > +                                 GFP_KERNEL);
> > +             if (xa_is_err(physxa)) {
> > +                     int err_ret = xa_err(physxa);
> > +
> > +                     xa_destroy(&new_physxa->phys_bits);
> > +                     kfree(new_physxa);
> > +
> > +                     return err_ret;
> > +             }
> > +             if (physxa) {
> > +                     xa_destroy(&new_physxa->phys_bits);
> > +                     kfree(new_physxa);
> > +             } else {
> > +                     physxa = new_physxa;
> > +             }
> > +     }
>
> You are nearly duplicating xa_load_or_alloc() here.
> Is xa_destroy() is really needed here? In the end we destroying an empty
> xarray.
>
> Unless xa_destroy() is a must something like this would be simpler IMHO:

I wanted to do proper xa_destroy(), as the whole point of this patch
is to satisfy lockdep, and do a proper xa_init(). The patch fixes a
warning in linux-next, and I think should be taken as is. We can do a
separate clean-up once the series lands, where xa_load_or_alloc()
could either take another argument, or split into two functions.

Pasha

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.