[PATCH v2 0/4] Some bugfix about kmemleak

[PATCH v2 0/4] Some bugfix about kmemleak

[Liu Shixin]

Some bugfix to improve the accuracy of detection and the print info of
debug mode.

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 (4):
  bootmem: use kmemleak_free_part_phys in put_page_bootmem
  bootmem: use kmemleak_free_part_phys in free_bootmem_page
  mm/kmemleak: fix partially freeing unknown object warning
  mm/kmemleak: fix print format of pointer in pr_debug()

 include/linux/bootmem_info.h |  2 +-
 mm/bootmem_info.c            |  2 +-
 mm/kmemleak.c                | 37 +++++++++++++++++++++---------------
 3 files changed, 24 insertions(+), 17 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] 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 v2 2/4] 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 v2 3/4] 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. The secound part still
be recorded by kmemleak even if it has alreadly 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

Fix the problem by adding a new mutex lock to make sure all objects are
deleted sequentially in delete_object_part(). The kmemleak_lock is not
suitable here because there is a memory allocation with flag GFP_KERNEL.

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

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 54c2c90d3abc..ed497866361a 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -208,6 +208,8 @@ static struct rb_root object_tree_root = RB_ROOT;
 static struct rb_root object_phys_tree_root = RB_ROOT;
 /* protecting the access to object_list, object_tree_root (or object_phys_tree_root) */
 static DEFINE_RAW_SPINLOCK(kmemleak_lock);
+/* Serial delete_object_part() to ensure all objects are deleted correctly */
+static DEFINE_MUTEX(delete_object_part_mutex);
 
 /* allocation caches for kmemleak internal data */
 static struct kmem_cache *object_cache;
@@ -785,13 +787,15 @@ static void delete_object_part(unsigned long ptr, size_t size, bool is_phys)
 	struct kmemleak_object *object;
 	unsigned long start, end;
 
+	mutex_lock(&delete_object_part_mutex);
+
 	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;
 	}
 
 	/*
@@ -809,6 +813,9 @@ static void delete_object_part(unsigned long ptr, size_t size, bool is_phys)
 			      GFP_KERNEL, is_phys);
 
 	__delete_object(object);
+
+unlock:
+	mutex_unlock(&delete_object_part_mutex);
 }
 
 static void __paint_it(struct kmemleak_object *object, int color)
-- 
2.25.1

[PATCH v2 4/4] 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>
---
 mm/kmemleak.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index ed497866361a..042cb0d18c7a 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -982,7 +982,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);
@@ -1003,7 +1003,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
@@ -1027,7 +1027,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
@@ -1050,7 +1050,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);
@@ -1068,7 +1068,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);
@@ -1086,7 +1086,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)
@@ -1107,7 +1107,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;
@@ -1138,7 +1138,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);
@@ -1156,7 +1156,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);
@@ -1176,7 +1176,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);
@@ -1194,7 +1194,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);
@@ -1210,7 +1210,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)
 		/*
@@ -1230,7 +1230,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);
@@ -1244,7 +1244,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

Re: [PATCH v2 4/4] mm/kmemleak: fix print format of pointer in pr_debug()

[Catalin Marinas]

On Sun, Oct 08, 2023 at 10:33:17AM +0800, Liu Shixin wrote:
> 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>

Re: [PATCH v2 3/4] mm/kmemleak: fix partially freeing unknown object warning

[Catalin Marinas]

On Sun, Oct 08, 2023 at 10:33:16AM +0800, Liu Shixin wrote:
> 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. The secound part still
> be recorded by kmemleak even if it has alreadly 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
> 
> Fix the problem by adding a new mutex lock to make sure all objects are
> deleted sequentially in delete_object_part(). The kmemleak_lock is not
> suitable here because there is a memory allocation with flag GFP_KERNEL.
> 
> Fixes: 53238a60dd4a ("kmemleak: Allow partial freeing of memory blocks")
> Signed-off-by: Liu Shixin <liushixin2@huawei.com>
> ---
>  mm/kmemleak.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 54c2c90d3abc..ed497866361a 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -208,6 +208,8 @@ static struct rb_root object_tree_root = RB_ROOT;
>  static struct rb_root object_phys_tree_root = RB_ROOT;
>  /* protecting the access to object_list, object_tree_root (or object_phys_tree_root) */
>  static DEFINE_RAW_SPINLOCK(kmemleak_lock);
> +/* Serial delete_object_part() to ensure all objects are deleted correctly */
> +static DEFINE_MUTEX(delete_object_part_mutex);

I mentioned in my reply on v1, do we actually need a lock at all?

https://lore.kernel.org/r/ZRRhDgR/SIxbOCDk@arm.com

I think we can rework the core a bit to only use kmemleak_lock. We could
use GFP_ATOMIC when invoking __create_object(), it won't matter much as
partial freeing is used rarely and only during boot. Alternatively, we
could allocate two objects outside the lock and just pass them to a new,
modified __create_object() function which would avoid re-allocation.

The mutex looks simpler but then we need to be careful we only calls
these functions in a sleepable context.

-- 
Catalin