Re: [PATCH 1/2] Revert "mm/kmemleak: move the initialisation of object to __link_object"

Re: [PATCH 1/2] Revert "mm/kmemleak: move the initialisation of object to __link_object"

[Geert Uytterhoeven]

On Wed, Nov 15, 2023 at 8:23 AM Liu Shixin <liushixin2@huawei.com> wrote:
> Move the initialisation of object back to__alloc_object() because
> set_track_prepare() attempt to acquire zone->lock(spinlocks) while
> __link_object is holding kmemleak_lock(raw_spinlocks). This is not
> right for RT mode.
>
> This reverts commit 245245c2fffd0050772a3f30ba50e2be92537a32.
>
> Signed-off-by: Liu Shixin <liushixin2@huawei.com>

Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 0/2] Fix invalid wait context of set_track_prepare()

[Liu Shixin]

Geert reported an invalid wait context[1] which is resulted by moving
set_track_prepare() inside kmemleak_lock. This is not allowed because
in RT mode, the spinlocks can be preempted but raw_spinlocks can not,
so it is not allowd to acquire spinlocks while holding raw_spinlocks.
The second patch fix same problem in kmemleak_update_trace().

Link: https://lore.kernel.org/linux-mm/CAMuHMdWj0UzwNaxUvcocTfh481qRJpOWwXxsJCTJfu1oCqvgdA@mail.gmail.com/ [1]

Liu Shixin (2):
  Revert "mm/kmemleak: move the initialisation of object to
    __link_object"
  mm/kmemleak: move set_track_prepare() outside raw_spinlocks

 mm/kmemleak.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

-- 
2.25.1

[PATCH 1/2] Revert "mm/kmemleak: move the initialisation of object to __link_object"

[Liu Shixin]

Move the initialisation of object back to__alloc_object() because
set_track_prepare() attempt to acquire zone->lock(spinlocks) while
__link_object is holding kmemleak_lock(raw_spinlocks). This is not
right for RT mode.

This reverts commit 245245c2fffd0050772a3f30ba50e2be92537a32.

Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 mm/kmemleak.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 1eacca03bedd..22bab3738a9e 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -642,32 +642,16 @@ static struct kmemleak_object *__alloc_object(gfp_t gfp)
 	if (!object) {
 		pr_warn("Cannot allocate a kmemleak_object structure\n");
 		kmemleak_disable();
+		return NULL;
 	}
 
-	return object;
-}
-
-static int __link_object(struct kmemleak_object *object, unsigned long ptr,
-			 size_t size, int min_count, bool is_phys)
-{
-
-	struct kmemleak_object *parent;
-	struct rb_node **link, *rb_parent;
-	unsigned long untagged_ptr;
-	unsigned long untagged_objp;
-
 	INIT_LIST_HEAD(&object->object_list);
 	INIT_LIST_HEAD(&object->gray_list);
 	INIT_HLIST_HEAD(&object->area_list);
 	raw_spin_lock_init(&object->lock);
 	atomic_set(&object->use_count, 1);
-	object->flags = OBJECT_ALLOCATED | (is_phys ? OBJECT_PHYS : 0);
-	object->pointer = ptr;
-	object->size = kfence_ksize((void *)ptr) ?: size;
 	object->excess_ref = 0;
-	object->min_count = min_count;
 	object->count = 0;			/* white color initially */
-	object->jiffies = jiffies;
 	object->checksum = 0;
 	object->del_state = 0;
 
@@ -692,6 +676,24 @@ static int __link_object(struct kmemleak_object *object, unsigned long ptr,
 	/* kernel backtrace */
 	object->trace_handle = set_track_prepare();
 
+	return object;
+}
+
+static int __link_object(struct kmemleak_object *object, unsigned long ptr,
+			 size_t size, int min_count, bool is_phys)
+{
+
+	struct kmemleak_object *parent;
+	struct rb_node **link, *rb_parent;
+	unsigned long untagged_ptr;
+	unsigned long untagged_objp;
+
+	object->flags = OBJECT_ALLOCATED | (is_phys ? OBJECT_PHYS : 0);
+	object->pointer = ptr;
+	object->size = kfence_ksize((void *)ptr) ?: size;
+	object->min_count = min_count;
+	object->jiffies = jiffies;
+
 	untagged_ptr = (unsigned long)kasan_reset_tag((void *)ptr);
 	/*
 	 * Only update min_addr and max_addr with object
-- 
2.25.1

[PATCH 2/2] mm/kmemleak: move set_track_prepare() outside raw_spinlocks

[Liu Shixin]

set_track_prepare() will call __alloc_pages() which attempt to acquire
zone->lock(spinlocks), so move it outside object->lock(raw_spinlocks)
because it's not right to acquire spinlocks while holding raw_spinlocks
in RT mode.

Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 mm/kmemleak.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 22bab3738a9e..5501363d6b31 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1152,6 +1152,7 @@ EXPORT_SYMBOL_GPL(kmemleak_free_percpu);
 void __ref kmemleak_update_trace(const void *ptr)
 {
 	struct kmemleak_object *object;
+	depot_stack_handle_t trace_handle;
 	unsigned long flags;
 
 	pr_debug("%s(0x%px)\n", __func__, ptr);
@@ -1168,8 +1169,9 @@ void __ref kmemleak_update_trace(const void *ptr)
 		return;
 	}
 
+	trace_handle = set_track_prepare();
 	raw_spin_lock_irqsave(&object->lock, flags);
-	object->trace_handle = set_track_prepare();
+	object->trace_handle = trace_handle;
 	raw_spin_unlock_irqrestore(&object->lock, flags);
 
 	put_object(object);
-- 
2.25.1

Re: [PATCH 1/2] Revert "mm/kmemleak: move the initialisation of object to __link_object"

[Catalin Marinas]

On Wed, Nov 15, 2023 at 04:21:37PM +0800, Liu Shixin wrote:
> Move the initialisation of object back to__alloc_object() because
> set_track_prepare() attempt to acquire zone->lock(spinlocks) while
> __link_object is holding kmemleak_lock(raw_spinlocks). This is not
> right for RT mode.
> 
> This reverts commit 245245c2fffd0050772a3f30ba50e2be92537a32.
> 
> Signed-off-by: Liu Shixin <liushixin2@huawei.com>

You can also add:

Fixes: 245245c2fffd ("mm/kmemleak: move the initialisation of object to __link_object")

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

I now realised that we update the object allocation stack trace via the
delete_object_part() when we shouldn't. I'd say __alloc_object() can
take a trace_handle as argument and if it's !0, set it directly whithout
calling set_track_prepare() (as a separate patch).

-- 
Catalin

Re: [PATCH 2/2] mm/kmemleak: move set_track_prepare() outside raw_spinlocks

[Catalin Marinas]

On Wed, Nov 15, 2023 at 04:21:38PM +0800, Liu Shixin wrote:
> set_track_prepare() will call __alloc_pages() which attempt to acquire
> zone->lock(spinlocks), so move it outside object->lock(raw_spinlocks)
> because it's not right to acquire spinlocks while holding raw_spinlocks
> in RT mode.
> 
> Signed-off-by: Liu Shixin <liushixin2@huawei.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks for the quick fixes.

-- 
Catalin