[PATCH v3 0/7] Some bugfix about kmemleak

[PATCH v3 0/7] Some bugfix about kmemleak

[Liu Shixin]

Some bugfix about kmemleak and the print info of debug mode.

v2->v3: Split __create_object into two function[4] and use kmemleak_lock to
	fix partially freeing unknown object warning[7]. And add a new patch
	to fix an inccorrect use of kmem_cache_free[5].
v1->v2: Split the first patch suggested by Andrew. And fix the format of
	the last two patches suggested by Catalin. Use mutex in patch[3]
	because there are memory allocation with flag GFP_KERNEL.

Liu Shixin (7):
  bootmem: use kmemleak_free_part_phys in put_page_bootmem
  bootmem: use kmemleak_free_part_phys in free_bootmem_page
  mm/kmemleak: fix print format of pointer in pr_debug()
  mm: kmemleak: split __create_object into two functions
  mm: kmemleak: use mem_pool_free() to free object
  mm: kmemleak: add __find_and_remove_object()
  mm/kmemleak: fix partially freeing unknown object warning

 include/linux/bootmem_info.h |   2 +-
 mm/bootmem_info.c            |   2 +-
 mm/kmemleak.c                | 154 +++++++++++++++++++++++------------
 3 files changed, 106 insertions(+), 52 deletions(-)

-- 
2.25.1

[PATCH v3 1/7] bootmem: use kmemleak_free_part_phys in put_page_bootmem

[Liu Shixin]

Since kmemleak_alloc_phys() rather than kmemleak_alloc() was called from
memblock_alloc_range_nid(), kmemleak_free_part_phys() should be used to
delete kmemleak object in put_page_bootmem(). In debug mode, there are
following warning:

 kmemleak: Partially freeing unknown object at 0xffff97345aff7000 (size 4096)

Fixes: dd0ff4d12dd2 ("bootmem: remove the vmemmap pages from kmemleak in put_page_bootmem")
Signed-off-by: Liu Shixin <liushixin2@huawei.com>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
---
 mm/bootmem_info.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/bootmem_info.c b/mm/bootmem_info.c
index b1efebfcf94b..fa7cb0c87c03 100644
--- a/mm/bootmem_info.c
+++ b/mm/bootmem_info.c
@@ -34,7 +34,7 @@ void put_page_bootmem(struct page *page)
 		ClearPagePrivate(page);
 		set_page_private(page, 0);
 		INIT_LIST_HEAD(&page->lru);
-		kmemleak_free_part(page_to_virt(page), PAGE_SIZE);
+		kmemleak_free_part_phys(PFN_PHYS(page_to_pfn(page)), PAGE_SIZE);
 		free_reserved_page(page);
 	}
 }
-- 
2.25.1

[PATCH v3 2/7] bootmem: use kmemleak_free_part_phys in free_bootmem_page

[Liu Shixin]

Since kmemleak_alloc_phys() rather than kmemleak_alloc() was called from
memblock_alloc_range_nid(), kmemleak_free_part_phys() should be used to
delete kmemleak object in free_bootmem_page(). In debug mode, there are
following warning:

 kmemleak: Partially freeing unknown object at 0xffff97345aff7000 (size 4096)

Fixes: 028725e73375 ("bootmem: remove the vmemmap pages from kmemleak in free_bootmem_page")
Signed-off-by: Liu Shixin <liushixin2@huawei.com>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
---
 include/linux/bootmem_info.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/bootmem_info.h b/include/linux/bootmem_info.h
index e1a3c9c9754c..cffa38a73618 100644
--- a/include/linux/bootmem_info.h
+++ b/include/linux/bootmem_info.h
@@ -60,7 +60,7 @@ static inline void get_page_bootmem(unsigned long info, struct page *page,
 
 static inline void free_bootmem_page(struct page *page)
 {
-	kmemleak_free_part(page_to_virt(page), PAGE_SIZE);
+	kmemleak_free_part_phys(PFN_PHYS(page_to_pfn(page)), PAGE_SIZE);
 	free_reserved_page(page);
 }
 #endif
-- 
2.25.1

[PATCH v3 3/7] mm/kmemleak: fix print format of pointer in pr_debug()

[Liu Shixin]

With 0x%p, the pointer will be hashed and print (____ptrval____) instead.
And with 0x%pa, the pointer can be successfully printed but with duplicate
prefixes, which looks like:

 kmemleak: kmemleak_free(0x(____ptrval____))
 kmemleak: kmemleak_free_percpu(0x(____ptrval____))
 kmemleak: kmemleak_free_part_phys(0x0x0000000a1af86000)

Use 0x%px instead of 0x%p or 0x%pa to print the pointer. Then the print
will be like:

 kmemleak: kmemleak_free(0xffff9111c145b020)
 kmemleak: kmemleak_free_percpu(0x00000000000333b0)
 kmemleak: kmemleak_free_part_phys(0x0000000a1af80000)

Signed-off-by: Liu Shixin <liushixin2@huawei.com>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
---
 mm/kmemleak.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 54c2c90d3abc..289b3be5ee6e 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -975,7 +975,7 @@ static void object_no_scan(unsigned long ptr)
 void __ref kmemleak_alloc(const void *ptr, size_t size, int min_count,
 			  gfp_t gfp)
 {
-	pr_debug("%s(0x%p, %zu, %d)\n", __func__, ptr, size, min_count);
+	pr_debug("%s(0x%px, %zu, %d)\n", __func__, ptr, size, min_count);
 
 	if (kmemleak_enabled && ptr && !IS_ERR(ptr))
 		create_object((unsigned long)ptr, size, min_count, gfp);
@@ -996,7 +996,7 @@ void __ref kmemleak_alloc_percpu(const void __percpu *ptr, size_t size,
 {
 	unsigned int cpu;
 
-	pr_debug("%s(0x%p, %zu)\n", __func__, ptr, size);
+	pr_debug("%s(0x%px, %zu)\n", __func__, ptr, size);
 
 	/*
 	 * Percpu allocations are only scanned and not reported as leaks
@@ -1020,7 +1020,7 @@ EXPORT_SYMBOL_GPL(kmemleak_alloc_percpu);
  */
 void __ref kmemleak_vmalloc(const struct vm_struct *area, size_t size, gfp_t gfp)
 {
-	pr_debug("%s(0x%p, %zu)\n", __func__, area, size);
+	pr_debug("%s(0x%px, %zu)\n", __func__, area, size);
 
 	/*
 	 * A min_count = 2 is needed because vm_struct contains a reference to
@@ -1043,7 +1043,7 @@ EXPORT_SYMBOL_GPL(kmemleak_vmalloc);
  */
 void __ref kmemleak_free(const void *ptr)
 {
-	pr_debug("%s(0x%p)\n", __func__, ptr);
+	pr_debug("%s(0x%px)\n", __func__, ptr);
 
 	if (kmemleak_free_enabled && ptr && !IS_ERR(ptr))
 		delete_object_full((unsigned long)ptr);
@@ -1061,7 +1061,7 @@ EXPORT_SYMBOL_GPL(kmemleak_free);
  */
 void __ref kmemleak_free_part(const void *ptr, size_t size)
 {
-	pr_debug("%s(0x%p)\n", __func__, ptr);
+	pr_debug("%s(0x%px)\n", __func__, ptr);
 
 	if (kmemleak_enabled && ptr && !IS_ERR(ptr))
 		delete_object_part((unsigned long)ptr, size, false);
@@ -1079,7 +1079,7 @@ void __ref kmemleak_free_percpu(const void __percpu *ptr)
 {
 	unsigned int cpu;
 
-	pr_debug("%s(0x%p)\n", __func__, ptr);
+	pr_debug("%s(0x%px)\n", __func__, ptr);
 
 	if (kmemleak_free_enabled && ptr && !IS_ERR(ptr))
 		for_each_possible_cpu(cpu)
@@ -1100,7 +1100,7 @@ void __ref kmemleak_update_trace(const void *ptr)
 	struct kmemleak_object *object;
 	unsigned long flags;
 
-	pr_debug("%s(0x%p)\n", __func__, ptr);
+	pr_debug("%s(0x%px)\n", __func__, ptr);
 
 	if (!kmemleak_enabled || IS_ERR_OR_NULL(ptr))
 		return;
@@ -1131,7 +1131,7 @@ EXPORT_SYMBOL(kmemleak_update_trace);
  */
 void __ref kmemleak_not_leak(const void *ptr)
 {
-	pr_debug("%s(0x%p)\n", __func__, ptr);
+	pr_debug("%s(0x%px)\n", __func__, ptr);
 
 	if (kmemleak_enabled && ptr && !IS_ERR(ptr))
 		make_gray_object((unsigned long)ptr);
@@ -1149,7 +1149,7 @@ EXPORT_SYMBOL(kmemleak_not_leak);
  */
 void __ref kmemleak_ignore(const void *ptr)
 {
-	pr_debug("%s(0x%p)\n", __func__, ptr);
+	pr_debug("%s(0x%px)\n", __func__, ptr);
 
 	if (kmemleak_enabled && ptr && !IS_ERR(ptr))
 		make_black_object((unsigned long)ptr, false);
@@ -1169,7 +1169,7 @@ EXPORT_SYMBOL(kmemleak_ignore);
  */
 void __ref kmemleak_scan_area(const void *ptr, size_t size, gfp_t gfp)
 {
-	pr_debug("%s(0x%p)\n", __func__, ptr);
+	pr_debug("%s(0x%px)\n", __func__, ptr);
 
 	if (kmemleak_enabled && ptr && size && !IS_ERR(ptr))
 		add_scan_area((unsigned long)ptr, size, gfp);
@@ -1187,7 +1187,7 @@ EXPORT_SYMBOL(kmemleak_scan_area);
  */
 void __ref kmemleak_no_scan(const void *ptr)
 {
-	pr_debug("%s(0x%p)\n", __func__, ptr);
+	pr_debug("%s(0x%px)\n", __func__, ptr);
 
 	if (kmemleak_enabled && ptr && !IS_ERR(ptr))
 		object_no_scan((unsigned long)ptr);
@@ -1203,7 +1203,7 @@ EXPORT_SYMBOL(kmemleak_no_scan);
  */
 void __ref kmemleak_alloc_phys(phys_addr_t phys, size_t size, gfp_t gfp)
 {
-	pr_debug("%s(0x%pa, %zu)\n", __func__, &phys, size);
+	pr_debug("%s(0x%px, %zu)\n", __func__, &phys, size);
 
 	if (kmemleak_enabled)
 		/*
@@ -1223,7 +1223,7 @@ EXPORT_SYMBOL(kmemleak_alloc_phys);
  */
 void __ref kmemleak_free_part_phys(phys_addr_t phys, size_t size)
 {
-	pr_debug("%s(0x%pa)\n", __func__, &phys);
+	pr_debug("%s(0x%px)\n", __func__, &phys);
 
 	if (kmemleak_enabled)
 		delete_object_part((unsigned long)phys, size, true);
@@ -1237,7 +1237,7 @@ EXPORT_SYMBOL(kmemleak_free_part_phys);
  */
 void __ref kmemleak_ignore_phys(phys_addr_t phys)
 {
-	pr_debug("%s(0x%pa)\n", __func__, &phys);
+	pr_debug("%s(0x%px)\n", __func__, &phys);
 
 	if (kmemleak_enabled)
 		make_black_object((unsigned long)phys, true);
-- 
2.25.1

[PATCH v3 4/7] mm: kmemleak: split __create_object into two functions

[Liu Shixin]

__create_object() consists of two part, the first part allocate a
kmemleak object and initialize it, the second part insert it into
object tree. This function need kmemleak_lock but actually only the
second part need lock.

Split it into two functions, the first function __alloc_object only
allocate a kmemleak object, and the second function __link_object()
will initialize the object and insert it into object tree, use the
kmemleak_lock to protect __link_object() only.

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

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 289b3be5ee6e..064fc3695c6b 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -623,25 +623,15 @@ static noinline depot_stack_handle_t set_track_prepare(void)
 	return trace_handle;
 }
 
-/*
- * Create the metadata (struct kmemleak_object) corresponding to an allocated
- * memory block and add it to the object_list and object_tree_root (or
- * object_phys_tree_root).
- */
-static void __create_object(unsigned long ptr, size_t size,
-			    int min_count, gfp_t gfp, bool is_phys)
+static struct kmemleak_object * __alloc_object(gfp_t gfp)
 {
-	unsigned long flags;
-	struct kmemleak_object *object, *parent;
-	struct rb_node **link, *rb_parent;
-	unsigned long untagged_ptr;
-	unsigned long untagged_objp;
+	struct kmemleak_object *object;
 
 	object = mem_pool_alloc(gfp);
 	if (!object) {
 		pr_warn("Cannot allocate a kmemleak_object structure\n");
 		kmemleak_disable();
-		return;
+		return NULL;
 	}
 
 	INIT_LIST_HEAD(&object->object_list);
@@ -649,13 +639,8 @@ static void __create_object(unsigned long ptr, size_t size,
 	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;
 
@@ -680,7 +665,23 @@ static void __create_object(unsigned long ptr, size_t size,
 	/* kernel backtrace */
 	object->trace_handle = set_track_prepare();
 
-	raw_spin_lock_irqsave(&kmemleak_lock, flags);
+	return object;
+}
+
+static void __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);
 	/*
@@ -711,14 +712,32 @@ static void __create_object(unsigned long ptr, size_t size,
 			 */
 			dump_object_info(parent);
 			kmem_cache_free(object_cache, object);
-			goto out;
+			return;
 		}
 	}
 	rb_link_node(&object->rb_node, rb_parent, link);
 	rb_insert_color(&object->rb_node, is_phys ? &object_phys_tree_root :
 					  &object_tree_root);
 	list_add_tail_rcu(&object->object_list, &object_list);
-out:
+}
+
+/*
+ * Create the metadata (struct kmemleak_object) corresponding to an allocated
+ * memory block and add it to the object_list and object_tree_root (or
+ * object_phys_tree_root).
+ */
+static void __create_object(unsigned long ptr, size_t size,
+				int min_count, gfp_t gfp, bool is_phys)
+{
+	struct kmemleak_object *object;
+	unsigned long flags;
+
+	object = __alloc_object(gfp);
+	if (!object)
+		return;
+
+	raw_spin_lock_irqsave(&kmemleak_lock, flags);
+	__link_object(object, ptr, size, min_count, is_phys);
 	raw_spin_unlock_irqrestore(&kmemleak_lock, flags);
 }
 
-- 
2.25.1

Re: [PATCH v3 4/7] mm: kmemleak: split __create_object into two functions

[Catalin Marinas]

Thanks for this, it looks better.

On Wed, Oct 18, 2023 at 06:29:49PM +0800, Liu Shixin wrote:
> -/*
> - * Create the metadata (struct kmemleak_object) corresponding to an allocated
> - * memory block and add it to the object_list and object_tree_root (or
> - * object_phys_tree_root).
> - */
> -static void __create_object(unsigned long ptr, size_t size,
> -			    int min_count, gfp_t gfp, bool is_phys)
> +static struct kmemleak_object * __alloc_object(gfp_t gfp)
>  {
> -	unsigned long flags;
> -	struct kmemleak_object *object, *parent;
> -	struct rb_node **link, *rb_parent;
> -	unsigned long untagged_ptr;
> -	unsigned long untagged_objp;
> +	struct kmemleak_object *object;
>  
>  	object = mem_pool_alloc(gfp);
>  	if (!object) {
>  		pr_warn("Cannot allocate a kmemleak_object structure\n");
>  		kmemleak_disable();
> -		return;
> +		return NULL;
>  	}
>  
>  	INIT_LIST_HEAD(&object->object_list);
> @@ -649,13 +639,8 @@ static void __create_object(unsigned long ptr, size_t size,
>  	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;

I'd keep all the initialisation in one place even if it means passing
more arguments to __alloc_object(). It feels a bit weird and error prone
to split the initialisation in two places. Otherwise I'm fine with the
split.

-- 
Catalin

[PATCH v3 5/7] mm: kmemleak: use mem_pool_free() to free object

[Liu Shixin]

The kmemleak object is allocated by mem_pool_alloc(), which
could be from slab or mem_pool[], so it's not suitable using
__kmem_cache_free() to free the object, use __mem_pool_free()
instead.

Fixes: 0647398a8c7b ("mm: kmemleak: simple memory allocation pool for kmemleak objects")
Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 mm/kmemleak.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 064fc3695c6b..ea34986c02b4 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -668,8 +668,8 @@ static struct kmemleak_object * __alloc_object(gfp_t gfp)
 	return object;
 }
 
-static void __link_object(struct kmemleak_object *object, unsigned long ptr,
-			  size_t size, int min_count, bool is_phys)
+static int __link_object(struct kmemleak_object *object, unsigned long ptr,
+			 size_t size, int min_count, bool is_phys)
 {
 
 	struct kmemleak_object *parent;
@@ -711,14 +711,15 @@ static void __link_object(struct kmemleak_object *object, unsigned long ptr,
 			 * be freed while the kmemleak_lock is held.
 			 */
 			dump_object_info(parent);
-			kmem_cache_free(object_cache, object);
-			return;
+			return -EEXIST;
 		}
 	}
 	rb_link_node(&object->rb_node, rb_parent, link);
 	rb_insert_color(&object->rb_node, is_phys ? &object_phys_tree_root :
 					  &object_tree_root);
 	list_add_tail_rcu(&object->object_list, &object_list);
+
+	return 0;
 }
 
 /*
@@ -731,14 +732,17 @@ static void __create_object(unsigned long ptr, size_t size,
 {
 	struct kmemleak_object *object;
 	unsigned long flags;
+	int ret;
 
 	object = __alloc_object(gfp);
 	if (!object)
 		return;
 
 	raw_spin_lock_irqsave(&kmemleak_lock, flags);
-	__link_object(object, ptr, size, min_count, is_phys);
+	ret = __link_object(object, ptr, size, min_count, is_phys);
 	raw_spin_unlock_irqrestore(&kmemleak_lock, flags);
+	if (ret)
+		mem_pool_free(object);
 }
 
 /* Create kmemleak object which allocated with virtual address. */
-- 
2.25.1

Re: [PATCH v3 5/7] mm: kmemleak: use mem_pool_free() to free object

[Catalin Marinas]

On Wed, Oct 18, 2023 at 06:29:50PM +0800, Liu Shixin wrote:
> The kmemleak object is allocated by mem_pool_alloc(), which
> could be from slab or mem_pool[], so it's not suitable using
> __kmem_cache_free() to free the object, use __mem_pool_free()
> instead.
> 
> Fixes: 0647398a8c7b ("mm: kmemleak: simple memory allocation pool for kmemleak objects")
> Signed-off-by: Liu Shixin <liushixin2@huawei.com>

Could you please reorder this patch before the previous one? If you
added a Fixes tag, we may want a cc stable as well (as for the other
patches with a Fixes tag) and it makes more sense to backport it on its
own without the __create_object() split. Otherwise:

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

Re: [PATCH v3 5/7] mm: kmemleak: use mem_pool_free() to free object

[Catalin Marinas]

On Wed, Oct 18, 2023 at 04:48:06PM +0100, Catalin Marinas wrote:
> On Wed, Oct 18, 2023 at 06:29:50PM +0800, Liu Shixin wrote:
> > The kmemleak object is allocated by mem_pool_alloc(), which
> > could be from slab or mem_pool[], so it's not suitable using
> > __kmem_cache_free() to free the object, use __mem_pool_free()
> > instead.
> > 
> > Fixes: 0647398a8c7b ("mm: kmemleak: simple memory allocation pool for kmemleak objects")
> > Signed-off-by: Liu Shixin <liushixin2@huawei.com>
> 
> Could you please reorder this patch before the previous one? If you
> added a Fixes tag, we may want a cc stable as well (as for the other
> patches with a Fixes tag) and it makes more sense to backport it on its
> own without the __create_object() split. Otherwise:

Ah, ignore this. If we want a cc stable, the whole thing needs
backporting, including the split which is essential for the subsequent
fix.

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

-- 
Catalin

Re: [PATCH v3 5/7] mm: kmemleak: use mem_pool_free() to free object

[Andrew Morton]

On Wed, 18 Oct 2023 16:57:50 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:

> > Could you please reorder this patch before the previous one? If you
> > added a Fixes tag, we may want a cc stable as well (as for the other
> > patches with a Fixes tag) and it makes more sense to backport it on its
> > own without the __create_object() split. Otherwise:
> 
> Ah, ignore this. If we want a cc stable, the whole thing needs
> backporting, including the split which is essential for the subsequent
> fix.

Do we want a cc:stable?  That tag wasn't originally included.

If so, all seven patches?

If "not all seven" then can we please have two series, one for the
backport patches, one for next merge window.

Re: [PATCH v3 5/7] mm: kmemleak: use mem_pool_free() to free object

[Catalin Marinas]

On Wed, Oct 18, 2023 at 09:22:53AM -0700, Andrew Morton wrote:
> On Wed, 18 Oct 2023 16:57:50 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > Could you please reorder this patch before the previous one? If you
> > > added a Fixes tag, we may want a cc stable as well (as for the other
> > > patches with a Fixes tag) and it makes more sense to backport it on its
> > > own without the __create_object() split. Otherwise:
> > 
> > Ah, ignore this. If we want a cc stable, the whole thing needs
> > backporting, including the split which is essential for the subsequent
> > fix.
> 
> Do we want a cc:stable?  That tag wasn't originally included.
> 
> If so, all seven patches?
> 
> If "not all seven" then can we please have two series, one for the
> backport patches, one for next merge window.

I think we need all 7 if we are to backport them. But we don't need to
cc stable explicitly, we can send them to stable@kernel.org separately
once tested on those stable versions. So, for the mm tree, don't bother
with cc stable.

-- 
Catalin

[PATCH v3 6/7] mm: kmemleak: add __find_and_remove_object()

[Liu Shixin]

Add new __find_and_remove_object() without kmemleak_lock protect, it is
in preparation for the next patch.

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

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index ea34986c02b4..7c9125c18956 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -583,6 +583,19 @@ static void __remove_object(struct kmemleak_object *object)
 	object->del_state |= DELSTATE_REMOVED;
 }
 
+static struct kmemleak_object *__find_and_remove_object(unsigned long ptr,
+							int alias,
+							bool is_phys)
+{
+	struct kmemleak_object *object;
+
+	object = __lookup_object(ptr, alias, is_phys);
+	if (object)
+		__remove_object(object);
+
+	return object;
+}
+
 /*
  * Look up an object in the object search tree and remove it from both
  * object_tree_root (or object_phys_tree_root) and object_list. The
@@ -596,9 +609,7 @@ static struct kmemleak_object *find_and_remove_object(unsigned long ptr, int ali
 	struct kmemleak_object *object;
 
 	raw_spin_lock_irqsave(&kmemleak_lock, flags);
-	object = __lookup_object(ptr, alias, is_phys);
-	if (object)
-		__remove_object(object);
+	object = __find_and_remove_object(ptr, alias, is_phys);
 	raw_spin_unlock_irqrestore(&kmemleak_lock, flags);
 
 	return object;
-- 
2.25.1

Re: [PATCH v3 6/7] mm: kmemleak: add __find_and_remove_object()

[Catalin Marinas]

On Wed, Oct 18, 2023 at 06:29:51PM +0800, Liu Shixin wrote:
> Add new __find_and_remove_object() without kmemleak_lock protect, it is
> in preparation for the next patch.
> 
> Signed-off-by: Liu Shixin <liushixin2@huawei.com>

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

[PATCH v3 7/7] mm/kmemleak: fix partially freeing unknown object warning

[Liu Shixin]

delete_object_part() can be called by multiple callers in the
same time. If an object is found and removed by a caller, and
then another caller try to find it too, it failed and return
directly. It still be recorded by kmemleak even if it has already
been freed to buddy. With DEBUG on, kmemleak will report the
following warning,

 kmemleak: Partially freeing unknown object at 0xa1af86000 (size 4096)
 CPU: 0 PID: 742 Comm: test_huge Not tainted 6.6.0-rc3kmemleak+ #54
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
 Call Trace:
  <TASK>
  dump_stack_lvl+0x37/0x50
  kmemleak_free_part_phys+0x50/0x60
  hugetlb_vmemmap_optimize+0x172/0x290
  ? __pfx_vmemmap_remap_pte+0x10/0x10
  __prep_new_hugetlb_folio+0xe/0x30
  prep_new_hugetlb_folio.isra.0+0xe/0x40
  alloc_fresh_hugetlb_folio+0xc3/0xd0
  alloc_surplus_hugetlb_folio.constprop.0+0x6e/0xd0
  hugetlb_acct_memory.part.0+0xe6/0x2a0
  hugetlb_reserve_pages+0x110/0x2c0
  hugetlbfs_file_mmap+0x11d/0x1b0
  mmap_region+0x248/0x9a0
  ? hugetlb_get_unmapped_area+0x15c/0x2d0
  do_mmap+0x38b/0x580
  vm_mmap_pgoff+0xe6/0x190
  ksys_mmap_pgoff+0x18a/0x1f0
  do_syscall_64+0x3f/0x90
  entry_SYSCALL_64_after_hwframe+0x6e/0xd8

Expand __create_object() and move __alloc_object() to the beginning.
Then use kmemleak_lock to protect __find_and_remove_object() and
__link_object() as a whole, which can guarantee all objects are
processed sequentialally.

Fixes: 53238a60dd4a ("kmemleak: Allow partial freeing of memory blocks")
Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 mm/kmemleak.c | 42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 7c9125c18956..a956b2734324 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -816,16 +816,25 @@ static void delete_object_full(unsigned long ptr)
  */
 static void delete_object_part(unsigned long ptr, size_t size, bool is_phys)
 {
-	struct kmemleak_object *object;
-	unsigned long start, end;
+	struct kmemleak_object *object, *object_l, *object_r;
+	unsigned long start, end, flags;
+
+	object_l = __alloc_object(GFP_KERNEL);
+	if (!object_l)
+		return;
 
-	object = find_and_remove_object(ptr, 1, is_phys);
+	object_r = __alloc_object(GFP_KERNEL);
+	if (!object_r)
+		goto out;
+
+	raw_spin_lock_irqsave(&kmemleak_lock, flags);
+	object = __find_and_remove_object(ptr, 1, is_phys);
 	if (!object) {
 #ifdef DEBUG
 		kmemleak_warn("Partially freeing unknown object at 0x%08lx (size %zu)\n",
 			      ptr, size);
 #endif
-		return;
+		goto unlock;
 	}
 
 	/*
@@ -835,14 +844,25 @@ static void delete_object_part(unsigned long ptr, size_t size, bool is_phys)
 	 */
 	start = object->pointer;
 	end = object->pointer + object->size;
-	if (ptr > start)
-		__create_object(start, ptr - start, object->min_count,
-			      GFP_KERNEL, is_phys);
-	if (ptr + size < end)
-		__create_object(ptr + size, end - ptr - size, object->min_count,
-			      GFP_KERNEL, is_phys);
+	if ((ptr > start) &&
+	    !__link_object(object_l, start, ptr - start,
+			   object->min_count, is_phys))
+		object_l = NULL;
+	if ((ptr + size < end) &&
+	    !__link_object(object_r, ptr + size, end - ptr - size,
+			   object->min_count, is_phys))
+		object_r = NULL;
+
+unlock:
+	raw_spin_unlock_irqrestore(&kmemleak_lock, flags);
+	if (object)
+		__delete_object(object);
 
-	__delete_object(object);
+out:
+	if (object_l)
+		mem_pool_free(object_l);
+	if (object_r)
+		mem_pool_free(object_r);
 }
 
 static void __paint_it(struct kmemleak_object *object, int color)
-- 
2.25.1

Re: [PATCH v3 7/7] mm/kmemleak: fix partially freeing unknown object warning

[Catalin Marinas]

On Wed, Oct 18, 2023 at 06:29:52PM +0800, Liu Shixin wrote:
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 7c9125c18956..a956b2734324 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -816,16 +816,25 @@ static void delete_object_full(unsigned long ptr)
>   */
>  static void delete_object_part(unsigned long ptr, size_t size, bool is_phys)
>  {
> -	struct kmemleak_object *object;
> -	unsigned long start, end;
> +	struct kmemleak_object *object, *object_l, *object_r;
> +	unsigned long start, end, flags;
> +
> +	object_l = __alloc_object(GFP_KERNEL);
> +	if (!object_l)
> +		return;
>  
> -	object = find_and_remove_object(ptr, 1, is_phys);
> +	object_r = __alloc_object(GFP_KERNEL);
> +	if (!object_r)
> +		goto out;
> +
> +	raw_spin_lock_irqsave(&kmemleak_lock, flags);
> +	object = __find_and_remove_object(ptr, 1, is_phys);
>  	if (!object) {
>  #ifdef DEBUG
>  		kmemleak_warn("Partially freeing unknown object at 0x%08lx (size %zu)\n",
>  			      ptr, size);
>  #endif
> -		return;
> +		goto unlock;
>  	}
>  
>  	/*
> @@ -835,14 +844,25 @@ static void delete_object_part(unsigned long ptr, size_t size, bool is_phys)
>  	 */
>  	start = object->pointer;
>  	end = object->pointer + object->size;
> -	if (ptr > start)
> -		__create_object(start, ptr - start, object->min_count,
> -			      GFP_KERNEL, is_phys);
> -	if (ptr + size < end)
> -		__create_object(ptr + size, end - ptr - size, object->min_count,
> -			      GFP_KERNEL, is_phys);
> +	if ((ptr > start) &&
> +	    !__link_object(object_l, start, ptr - start,
> +			   object->min_count, is_phys))
> +		object_l = NULL;
> +	if ((ptr + size < end) &&
> +	    !__link_object(object_r, ptr + size, end - ptr - size,
> +			   object->min_count, is_phys))
> +		object_r = NULL;
> +
> +unlock:
> +	raw_spin_unlock_irqrestore(&kmemleak_lock, flags);
> +	if (object)
> +		__delete_object(object);
>  
> -	__delete_object(object);
> +out:
> +	if (object_l)
> +		mem_pool_free(object_l);
> +	if (object_r)
> +		mem_pool_free(object_r);
>  }

Ah, I see now why __link_object() needs to do further object
initialisation under the lock as prior to find_and_remove_object() we
don't have all the information needed for object_l and object_r.

Maybe in the __create_object splitting patch you can leave
__alloc_object() just do the actual allocation and __link_object() do
the full initialisation. Up to you, you can send a separate patch on top
as I can see Andrew picked them up already.

Otherwise the series looks fine.

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