[PATCH 0/2] KUnit-Kmemleak Integration

[PATCH 0/2] KUnit-Kmemleak Integration

[Uriel Guajardo]

From: Uriel Guajardo <urielguajardo@google.com>

With these patches, KUnit can access and manually run kmemleak in every test
case. Any errors caught by kmemleak will cause the KUnit test to fail.

This patchset relies on "kunit: KASAN integration", which places the
currently running kunit test in task_struct. [1]

[1] https://lore.kernel.org/linux-kselftest/20200606040349.246780-2-davidgow@google.com

Uriel Guajardo (2):
  kunit: support kunit failures from debugging tools
  kunit: kmemleak integration

 include/kunit/test-bug.h | 15 +++++++++++++
 include/kunit/test.h     |  1 +
 include/linux/kmemleak.h | 11 ++++++++++
 lib/Kconfig.debug        | 26 +++++++++++++++++++++++
 lib/kunit/test.c         | 46 +++++++++++++++++++++++++++++++++++-----
 mm/kmemleak.c            | 27 +++++++++++++++++------
 6 files changed, 115 insertions(+), 11 deletions(-)
 create mode 100644 include/kunit/test-bug.h

-- 
2.27.0.212.ge8ba1cc988-goog

[PATCH 1/2] kunit: support kunit failures from debugging tools

[Uriel Guajardo]

From: Uriel Guajardo <urielguajardo@google.com>

Dynamic analyis tools can now fail the currently running kunit
test case by calling kunit_fail_current_test().

A new header file is created so that no circular dependencies are
created with external tools.

This patch requires "kunit: KASAN integration", which places the
currently running kunit test in task_struct. [1]

[1] https://lore.kernel.org/linux-kselftest/20200606040349.246780-2-davidgow@google.com

Signed-off-by: Uriel Guajardo <urielguajardo@google.com>
---
 include/kunit/test-bug.h | 15 +++++++++++++++
 include/kunit/test.h     |  1 +
 lib/kunit/test.c         | 10 ++++++----
 3 files changed, 22 insertions(+), 4 deletions(-)
 create mode 100644 include/kunit/test-bug.h

diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h
new file mode 100644
index 000000000000..69ec3dd91c52
--- /dev/null
+++ b/include/kunit/test-bug.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Base unit test (KUnit) API.
+ *
+ * Copyright (C) 2020, Google LLC.
+ * Author: Uriel Guajardo <urielguajardo@google.com>
+ */
+
+#if IS_ENABLED(CONFIG_KUNIT)
+extern void kunit_fail_current_test(void);
+#else
+static inline void kunit_fail_current_test(void)
+{
+}
+#endif
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 1dc3d118f64b..c46715c739b5 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -11,6 +11,7 @@
 
 #include <kunit/assert.h>
 #include <kunit/try-catch.h>
+#include <kunit/test-bug.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/slab.h>
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index ef3bfb9fae48..8580ed831a8f 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -16,6 +16,12 @@
 #include "string-stream.h"
 #include "try-catch-impl.h"
 
+void kunit_fail_current_test(void)
+{
+	if (current->kunit_test)
+		kunit_set_failure(current->kunit_test);
+}
+
 static void kunit_print_tap_version(void)
 {
 	static bool kunit_has_printed_tap_version;
@@ -284,9 +290,7 @@ static void kunit_try_run_case(void *data)
 	struct kunit_suite *suite = ctx->suite;
 	struct kunit_case *test_case = ctx->test_case;
 
-#if (IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT))
 	current->kunit_test = test;
-#endif /* IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT) */
 
 	/*
 	 * kunit_run_case_internal may encounter a fatal error; if it does,
@@ -603,9 +607,7 @@ void kunit_cleanup(struct kunit *test)
 		spin_unlock(&test->lock);
 		kunit_remove_resource(test, res);
 	}
-#if (IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT))
 	current->kunit_test = NULL;
-#endif /* IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)*/
 }
 EXPORT_SYMBOL_GPL(kunit_cleanup);
 
-- 
2.27.0.212.ge8ba1cc988-goog

[PATCH 2/2] kunit: kmemleak integration

[Uriel Guajardo]

From: Uriel Guajardo <urielguajardo@google.com>

Integrate kmemleak into the KUnit testing framework.

Kmemleak will now fail the currently running KUnit test case if it finds
any memory leaks.

The minimum object age for reporting is set to 0 msecs so that leaks are
not ignored if the test case finishes too quickly.

Signed-off-by: Uriel Guajardo <urielguajardo@google.com>
---
 include/linux/kmemleak.h | 11 +++++++++++
 lib/Kconfig.debug        | 26 ++++++++++++++++++++++++++
 lib/kunit/test.c         | 36 +++++++++++++++++++++++++++++++++++-
 mm/kmemleak.c            | 27 +++++++++++++++++++++------
 4 files changed, 93 insertions(+), 7 deletions(-)

diff --git a/include/linux/kmemleak.h b/include/linux/kmemleak.h
index 34684b2026ab..0da427934462 100644
--- a/include/linux/kmemleak.h
+++ b/include/linux/kmemleak.h
@@ -35,6 +35,10 @@ extern void kmemleak_free_part_phys(phys_addr_t phys, size_t size) __ref;
 extern void kmemleak_not_leak_phys(phys_addr_t phys) __ref;
 extern void kmemleak_ignore_phys(phys_addr_t phys) __ref;
 
+extern ssize_t kmemleak_write(struct file *file,
+			      const char __user *user_buf,
+			      size_t size, loff_t *ppos);
+
 static inline void kmemleak_alloc_recursive(const void *ptr, size_t size,
 					    int min_count, slab_flags_t flags,
 					    gfp_t gfp)
@@ -120,6 +124,13 @@ static inline void kmemleak_ignore_phys(phys_addr_t phys)
 {
 }
 
+static inline ssize_t kmemleak_write(struct file *file,
+				     const char __user *user_buf,
+				     size_t size, loff_t *ppos)
+{
+	return -1;
+}
+
 #endif	/* CONFIG_DEBUG_KMEMLEAK */
 
 #endif	/* __KMEMLEAK_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 21d9c5f6e7ec..e9c492cb3f4d 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -602,6 +602,32 @@ config DEBUG_KMEMLEAK_MEM_POOL_SIZE
 	  fully initialised, this memory pool acts as an emergency one
 	  if slab allocations fail.
 
+config DEBUG_KMEMLEAK_MAX_TRACE
+	int "Kmemleak stack trace length"
+	depends on DEBUG_KMEMLEAK
+	default 16
+
+config DEBUG_KMEMLEAK_MSECS_MIN_AGE
+	int "Minimum object age before reporting in msecs"
+	depends on DEBUG_KMEMLEAK
+	default 0 if KUNIT
+	default 5000
+
+config DEBUG_KMEMLEAK_SECS_FIRST_SCAN
+	int "Delay before first scan in secs"
+	depends on DEBUG_KMEMLEAK
+	default 60
+
+config DEBUG_KMEMLEAK_SECS_SCAN_WAIT
+	int "Delay before subsequent auto scans in secs"
+	depends on DEBUG_KMEMLEAK
+	default 600
+
+config DEBUG_KMEMLEAK_MAX_SCAN_SIZE
+	int "Maximum size of scanned block"
+	depends on DEBUG_KMEMLEAK
+	default 4096
+
 config DEBUG_KMEMLEAK_TEST
 	tristate "Simple test for the kernel memory leak detector"
 	depends on DEBUG_KMEMLEAK && m
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 8580ed831a8f..8d113a6a214b 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -11,6 +11,7 @@
 #include <linux/kref.h>
 #include <linux/sched/debug.h>
 #include <linux/sched.h>
+#include <linux/kmemleak.h>
 
 #include "debugfs.h"
 #include "string-stream.h"
@@ -277,6 +278,27 @@ static void kunit_run_case_cleanup(struct kunit *test,
 	kunit_case_internal_cleanup(test);
 }
 
+/*
+ * Manually scans for memory leaks using the kmemleak tool.
+ *
+ * Any leaks that occurred since the previous scan will be
+ * reported and will cause the currently running test to fail.
+ */
+static inline void kmemleak_scan(void)
+{
+	loff_t pos;
+	kmemleak_write(NULL, "scan", 5, &pos);
+}
+
+/*
+ * Turn off the background automatic scan that kmemleak performs upon starting
+ */
+static inline void kmemleak_automatic_scan_off(void)
+{
+	loff_t pos;
+	kmemleak_write(NULL, "scan=off", 9, &pos);
+}
+
 struct kunit_try_catch_context {
 	struct kunit *test;
 	struct kunit_suite *suite;
@@ -290,6 +312,12 @@ static void kunit_try_run_case(void *data)
 	struct kunit_suite *suite = ctx->suite;
 	struct kunit_case *test_case = ctx->test_case;
 
+	/*
+	 * Clear any reported memory leaks since last scan, so that only the
+	 * leaks pertaining to the test case remain afterwards.
+	 */
+	kmemleak_scan();
+
 	current->kunit_test = test;
 
 	/*
@@ -298,7 +326,12 @@ static void kunit_try_run_case(void *data)
 	 * thread will resume control and handle any necessary clean up.
 	 */
 	kunit_run_case_internal(test, suite, test_case);
-	/* This line may never be reached. */
+
+	/* These lines may never be reached. */
+
+	/* Report any detected memory leaks that occurred in the test case */
+	kmemleak_scan();
+
 	kunit_run_case_cleanup(test, suite);
 }
 
@@ -388,6 +421,7 @@ static void kunit_init_suite(struct kunit_suite *suite)
 int __kunit_test_suites_init(struct kunit_suite **suites)
 {
 	unsigned int i;
+	kmemleak_automatic_scan_off();
 
 	for (i = 0; suites[i] != NULL; i++) {
 		kunit_init_suite(suites[i]);
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index e362dc3d2028..ad046c77e00c 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -99,15 +99,26 @@
 #include <linux/kasan.h>
 #include <linux/kmemleak.h>
 #include <linux/memory_hotplug.h>
+#include <kunit/test.h>
 
 /*
  * Kmemleak configuration and common defines.
  */
-#define MAX_TRACE		16	/* stack trace length */
-#define MSECS_MIN_AGE		5000	/* minimum object age for reporting */
-#define SECS_FIRST_SCAN		60	/* delay before the first scan */
-#define SECS_SCAN_WAIT		600	/* subsequent auto scanning delay */
-#define MAX_SCAN_SIZE		4096	/* maximum size of a scanned block */
+
+/* stack trace length */
+#define MAX_TRACE		CONFIG_DEBUG_KMEMLEAK_MAX_TRACE
+
+/* minimum object age for reporting */
+#define MSECS_MIN_AGE		CONFIG_DEBUG_KMEMLEAK_MSECS_MIN_AGE
+
+/* delay before the first scan */
+#define SECS_FIRST_SCAN		CONFIG_DEBUG_KMEMLEAK_SECS_FIRST_SCAN
+
+/* subsequent auto scanning delay */
+#define SECS_SCAN_WAIT		CONFIG_DEBUG_KMEMLEAK_SECS_SCAN_WAIT
+
+/* maximum size of a scanned lock */
+#define MAX_SCAN_SIZE		CONFIG_DEBUG_KMEMLEAK_MAX_SCAN_SIZE
 
 #define BYTES_PER_POINTER	sizeof(void *)
 
@@ -1490,6 +1501,7 @@ static void kmemleak_scan(void)
 	 * Check for new or unreferenced objects modified since the previous
 	 * scan and color them gray until the next scan.
 	 */
+#if (!IS_ENABLED(CONFIG_KUNIT))
 	rcu_read_lock();
 	list_for_each_entry_rcu(object, &object_list, object_list) {
 		raw_spin_lock_irqsave(&object->lock, flags);
@@ -1502,6 +1514,7 @@ static void kmemleak_scan(void)
 		raw_spin_unlock_irqrestore(&object->lock, flags);
 	}
 	rcu_read_unlock();
+#endif
 
 	/*
 	 * Re-scan the gray list for modified unreferenced objects.
@@ -1534,6 +1547,8 @@ static void kmemleak_scan(void)
 	rcu_read_unlock();
 
 	if (new_leaks) {
+		kunit_fail_current_test();
+
 		kmemleak_found_leaks = true;
 
 		pr_info("%d new suspected memory leaks (see /sys/kernel/debug/kmemleak)\n",
@@ -1764,7 +1779,7 @@ static void __kmemleak_do_cleanup(void);
  *		  if kmemleak has been disabled.
  *   dump=...	- dump information about the object found at the given address
  */
-static ssize_t kmemleak_write(struct file *file, const char __user *user_buf,
+ssize_t kmemleak_write(struct file *file, const char __user *user_buf,
 			      size_t size, loff_t *ppos)
 {
 	char buf[64];
-- 
2.27.0.212.ge8ba1cc988-goog

Re: [PATCH 2/2] kunit: kmemleak integration

[Qian Cai]

On Mon, Jul 06, 2020 at 09:13:09PM +0000, Uriel Guajardo wrote:
> From: Uriel Guajardo <urielguajardo@google.com>
> 
> Integrate kmemleak into the KUnit testing framework.
> 
> Kmemleak will now fail the currently running KUnit test case if it finds
> any memory leaks.
> 
> The minimum object age for reporting is set to 0 msecs so that leaks are
> not ignored if the test case finishes too quickly.
> 
> Signed-off-by: Uriel Guajardo <urielguajardo@google.com>
> ---
>  include/linux/kmemleak.h | 11 +++++++++++
>  lib/Kconfig.debug        | 26 ++++++++++++++++++++++++++
>  lib/kunit/test.c         | 36 +++++++++++++++++++++++++++++++++++-
>  mm/kmemleak.c            | 27 +++++++++++++++++++++------
>  4 files changed, 93 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/kmemleak.h b/include/linux/kmemleak.h
> index 34684b2026ab..0da427934462 100644
> --- a/include/linux/kmemleak.h
> +++ b/include/linux/kmemleak.h
> @@ -35,6 +35,10 @@ extern void kmemleak_free_part_phys(phys_addr_t phys, size_t size) __ref;
>  extern void kmemleak_not_leak_phys(phys_addr_t phys) __ref;
>  extern void kmemleak_ignore_phys(phys_addr_t phys) __ref;
>  
> +extern ssize_t kmemleak_write(struct file *file,
> +			      const char __user *user_buf,
> +			      size_t size, loff_t *ppos);
> +
>  static inline void kmemleak_alloc_recursive(const void *ptr, size_t size,
>  					    int min_count, slab_flags_t flags,
>  					    gfp_t gfp)
> @@ -120,6 +124,13 @@ static inline void kmemleak_ignore_phys(phys_addr_t phys)
>  {
>  }
>  
> +static inline ssize_t kmemleak_write(struct file *file,
> +				     const char __user *user_buf,
> +				     size_t size, loff_t *ppos)
> +{
> +	return -1;
> +}
> +
>  #endif	/* CONFIG_DEBUG_KMEMLEAK */
>  
>  #endif	/* __KMEMLEAK_H */
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 21d9c5f6e7ec..e9c492cb3f4d 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -602,6 +602,32 @@ config DEBUG_KMEMLEAK_MEM_POOL_SIZE
>  	  fully initialised, this memory pool acts as an emergency one
>  	  if slab allocations fail.
>  
> +config DEBUG_KMEMLEAK_MAX_TRACE
> +	int "Kmemleak stack trace length"
> +	depends on DEBUG_KMEMLEAK
> +	default 16
> +
> +config DEBUG_KMEMLEAK_MSECS_MIN_AGE
> +	int "Minimum object age before reporting in msecs"
> +	depends on DEBUG_KMEMLEAK
> +	default 0 if KUNIT
> +	default 5000
> +
> +config DEBUG_KMEMLEAK_SECS_FIRST_SCAN
> +	int "Delay before first scan in secs"
> +	depends on DEBUG_KMEMLEAK
> +	default 60
> +
> +config DEBUG_KMEMLEAK_SECS_SCAN_WAIT
> +	int "Delay before subsequent auto scans in secs"
> +	depends on DEBUG_KMEMLEAK
> +	default 600
> +
> +config DEBUG_KMEMLEAK_MAX_SCAN_SIZE
> +	int "Maximum size of scanned block"
> +	depends on DEBUG_KMEMLEAK
> +	default 4096
> +

Why do you make those configurable? I don't see anywhere you make use of
them except DEBUG_KMEMLEAK_MSECS_MIN_AGE?

Even then, how setting DEBUG_KMEMLEAK_MSECS_MIN_AGE=0 not giving too
many false positives? Kmemleak simply does not work that instantly.

>  config DEBUG_KMEMLEAK_TEST
>  	tristate "Simple test for the kernel memory leak detector"
>  	depends on DEBUG_KMEMLEAK && m
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 8580ed831a8f..8d113a6a214b 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -11,6 +11,7 @@
>  #include <linux/kref.h>
>  #include <linux/sched/debug.h>
>  #include <linux/sched.h>
> +#include <linux/kmemleak.h>
>  
>  #include "debugfs.h"
>  #include "string-stream.h"
> @@ -277,6 +278,27 @@ static void kunit_run_case_cleanup(struct kunit *test,
>  	kunit_case_internal_cleanup(test);
>  }
>  
> +/*
> + * Manually scans for memory leaks using the kmemleak tool.
> + *
> + * Any leaks that occurred since the previous scan will be
> + * reported and will cause the currently running test to fail.
> + */
> +static inline void kmemleak_scan(void)
> +{
> +	loff_t pos;
> +	kmemleak_write(NULL, "scan", 5, &pos);
> +}
> +
> +/*
> + * Turn off the background automatic scan that kmemleak performs upon starting
> + */
> +static inline void kmemleak_automatic_scan_off(void)
> +{
> +	loff_t pos;
> +	kmemleak_write(NULL, "scan=off", 9, &pos);
> +}
> +
>  struct kunit_try_catch_context {
>  	struct kunit *test;
>  	struct kunit_suite *suite;
> @@ -290,6 +312,12 @@ static void kunit_try_run_case(void *data)
>  	struct kunit_suite *suite = ctx->suite;
>  	struct kunit_case *test_case = ctx->test_case;
>  
> +	/*
> +	 * Clear any reported memory leaks since last scan, so that only the
> +	 * leaks pertaining to the test case remain afterwards.
> +	 */
> +	kmemleak_scan();
> +
>  	current->kunit_test = test;
>  
>  	/*
> @@ -298,7 +326,12 @@ static void kunit_try_run_case(void *data)
>  	 * thread will resume control and handle any necessary clean up.
>  	 */
>  	kunit_run_case_internal(test, suite, test_case);
> -	/* This line may never be reached. */
> +
> +	/* These lines may never be reached. */
> +
> +	/* Report any detected memory leaks that occurred in the test case */
> +	kmemleak_scan();
> +
>  	kunit_run_case_cleanup(test, suite);
>  }
>  
> @@ -388,6 +421,7 @@ static void kunit_init_suite(struct kunit_suite *suite)
>  int __kunit_test_suites_init(struct kunit_suite **suites)
>  {
>  	unsigned int i;
> +	kmemleak_automatic_scan_off();
>  
>  	for (i = 0; suites[i] != NULL; i++) {
>  		kunit_init_suite(suites[i]);
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index e362dc3d2028..ad046c77e00c 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -99,15 +99,26 @@
>  #include <linux/kasan.h>
>  #include <linux/kmemleak.h>
>  #include <linux/memory_hotplug.h>
> +#include <kunit/test.h>
>  
>  /*
>   * Kmemleak configuration and common defines.
>   */
> -#define MAX_TRACE		16	/* stack trace length */
> -#define MSECS_MIN_AGE		5000	/* minimum object age for reporting */
> -#define SECS_FIRST_SCAN		60	/* delay before the first scan */
> -#define SECS_SCAN_WAIT		600	/* subsequent auto scanning delay */
> -#define MAX_SCAN_SIZE		4096	/* maximum size of a scanned block */
> +
> +/* stack trace length */
> +#define MAX_TRACE		CONFIG_DEBUG_KMEMLEAK_MAX_TRACE
> +
> +/* minimum object age for reporting */
> +#define MSECS_MIN_AGE		CONFIG_DEBUG_KMEMLEAK_MSECS_MIN_AGE
> +
> +/* delay before the first scan */
> +#define SECS_FIRST_SCAN		CONFIG_DEBUG_KMEMLEAK_SECS_FIRST_SCAN
> +
> +/* subsequent auto scanning delay */
> +#define SECS_SCAN_WAIT		CONFIG_DEBUG_KMEMLEAK_SECS_SCAN_WAIT
> +
> +/* maximum size of a scanned lock */
> +#define MAX_SCAN_SIZE		CONFIG_DEBUG_KMEMLEAK_MAX_SCAN_SIZE
>  
>  #define BYTES_PER_POINTER	sizeof(void *)
>  
> @@ -1490,6 +1501,7 @@ static void kmemleak_scan(void)
>  	 * Check for new or unreferenced objects modified since the previous
>  	 * scan and color them gray until the next scan.
>  	 */
> +#if (!IS_ENABLED(CONFIG_KUNIT))
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(object, &object_list, object_list) {
>  		raw_spin_lock_irqsave(&object->lock, flags);
> @@ -1502,6 +1514,7 @@ static void kmemleak_scan(void)
>  		raw_spin_unlock_irqrestore(&object->lock, flags);
>  	}
>  	rcu_read_unlock();
> +#endif
>  
>  	/*
>  	 * Re-scan the gray list for modified unreferenced objects.
> @@ -1534,6 +1547,8 @@ static void kmemleak_scan(void)
>  	rcu_read_unlock();
>  
>  	if (new_leaks) {
> +		kunit_fail_current_test();
> +
>  		kmemleak_found_leaks = true;
>  
>  		pr_info("%d new suspected memory leaks (see /sys/kernel/debug/kmemleak)\n",
> @@ -1764,7 +1779,7 @@ static void __kmemleak_do_cleanup(void);
>   *		  if kmemleak has been disabled.
>   *   dump=...	- dump information about the object found at the given address
>   */
> -static ssize_t kmemleak_write(struct file *file, const char __user *user_buf,
> +ssize_t kmemleak_write(struct file *file, const char __user *user_buf,
>  			      size_t size, loff_t *ppos)
>  {
>  	char buf[64];
> -- 
> 2.27.0.212.ge8ba1cc988-goog
> 
> 

Re: [PATCH 2/2] kunit: kmemleak integration

[Uriel Guajardo]

On Mon, Jul 6, 2020 at 4:39 PM Qian Cai <cai@lca.pw> wrote:
>
> On Mon, Jul 06, 2020 at 09:13:09PM +0000, Uriel Guajardo wrote:
> > From: Uriel Guajardo <urielguajardo@google.com>
> >
> > Integrate kmemleak into the KUnit testing framework.
> >
> > Kmemleak will now fail the currently running KUnit test case if it finds
> > any memory leaks.
> >
> > The minimum object age for reporting is set to 0 msecs so that leaks are
> > not ignored if the test case finishes too quickly.
> >
> > Signed-off-by: Uriel Guajardo <urielguajardo@google.com>
> > ---
> >  include/linux/kmemleak.h | 11 +++++++++++
> >  lib/Kconfig.debug        | 26 ++++++++++++++++++++++++++
> >  lib/kunit/test.c         | 36 +++++++++++++++++++++++++++++++++++-
> >  mm/kmemleak.c            | 27 +++++++++++++++++++++------
> >  4 files changed, 93 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/linux/kmemleak.h b/include/linux/kmemleak.h
> > index 34684b2026ab..0da427934462 100644
> > --- a/include/linux/kmemleak.h
> > +++ b/include/linux/kmemleak.h
> > @@ -35,6 +35,10 @@ extern void kmemleak_free_part_phys(phys_addr_t phys, size_t size) __ref;
> >  extern void kmemleak_not_leak_phys(phys_addr_t phys) __ref;
> >  extern void kmemleak_ignore_phys(phys_addr_t phys) __ref;
> >
> > +extern ssize_t kmemleak_write(struct file *file,
> > +                           const char __user *user_buf,
> > +                           size_t size, loff_t *ppos);
> > +
> >  static inline void kmemleak_alloc_recursive(const void *ptr, size_t size,
> >                                           int min_count, slab_flags_t flags,
> >                                           gfp_t gfp)
> > @@ -120,6 +124,13 @@ static inline void kmemleak_ignore_phys(phys_addr_t phys)
> >  {
> >  }
> >
> > +static inline ssize_t kmemleak_write(struct file *file,
> > +                                  const char __user *user_buf,
> > +                                  size_t size, loff_t *ppos)
> > +{
> > +     return -1;
> > +}
> > +
> >  #endif       /* CONFIG_DEBUG_KMEMLEAK */
> >
> >  #endif       /* __KMEMLEAK_H */
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 21d9c5f6e7ec..e9c492cb3f4d 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -602,6 +602,32 @@ config DEBUG_KMEMLEAK_MEM_POOL_SIZE
> >         fully initialised, this memory pool acts as an emergency one
> >         if slab allocations fail.
> >
> > +config DEBUG_KMEMLEAK_MAX_TRACE
> > +     int "Kmemleak stack trace length"
> > +     depends on DEBUG_KMEMLEAK
> > +     default 16
> > +
> > +config DEBUG_KMEMLEAK_MSECS_MIN_AGE
> > +     int "Minimum object age before reporting in msecs"
> > +     depends on DEBUG_KMEMLEAK
> > +     default 0 if KUNIT
> > +     default 5000
> > +
> > +config DEBUG_KMEMLEAK_SECS_FIRST_SCAN
> > +     int "Delay before first scan in secs"
> > +     depends on DEBUG_KMEMLEAK
> > +     default 60
> > +
> > +config DEBUG_KMEMLEAK_SECS_SCAN_WAIT
> > +     int "Delay before subsequent auto scans in secs"
> > +     depends on DEBUG_KMEMLEAK
> > +     default 600
> > +
> > +config DEBUG_KMEMLEAK_MAX_SCAN_SIZE
> > +     int "Maximum size of scanned block"
> > +     depends on DEBUG_KMEMLEAK
> > +     default 4096
> > +
>
> Why do you make those configurable? I don't see anywhere you make use of
> them except DEBUG_KMEMLEAK_MSECS_MIN_AGE?
>

That's correct. Strictly speaking, only DEBUG_KMEMLEAK_MSECS_MIN_AGE
is used to set a default when KUnit is configured.

There is no concrete reason why these other variables need to be
configurable. At the time of writing this, it seemed to make the most
sense to configure the other configuration options, given that I was
already going to make MSECS_MIN_AGE configurable. It can definitely be
taken out.

> Even then, how setting DEBUG_KMEMLEAK_MSECS_MIN_AGE=0 not giving too
> many false positives? Kmemleak simply does not work that instantly.
>

I did not experience this issue, but I see your point.

An alternative that I was thinking about -- and one that is not in
this patch -- is to wait DEBUG_KMEMLEAK_MSECS_MIN_AGE after each test
case in a test suite, while leaving kmemleak's default value as is. I
was hesitant to do this initially because many KUnit test cases run
quick, so this may result in a lot of time just waiting. But if we
leave it configurable, the user can change this as needed and deal
with the possible false positives.

> >  config DEBUG_KMEMLEAK_TEST
> >       tristate "Simple test for the kernel memory leak detector"
> >       depends on DEBUG_KMEMLEAK && m
> > diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> > index 8580ed831a8f..8d113a6a214b 100644
> > --- a/lib/kunit/test.c
> > +++ b/lib/kunit/test.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/kref.h>
> >  #include <linux/sched/debug.h>
> >  #include <linux/sched.h>
> > +#include <linux/kmemleak.h>
> >
> >  #include "debugfs.h"
> >  #include "string-stream.h"
> > @@ -277,6 +278,27 @@ static void kunit_run_case_cleanup(struct kunit *test,
> >       kunit_case_internal_cleanup(test);
> >  }
> >
> > +/*
> > + * Manually scans for memory leaks using the kmemleak tool.
> > + *
> > + * Any leaks that occurred since the previous scan will be
> > + * reported and will cause the currently running test to fail.
> > + */
> > +static inline void kmemleak_scan(void)
> > +{
> > +     loff_t pos;
> > +     kmemleak_write(NULL, "scan", 5, &pos);
> > +}
> > +
> > +/*
> > + * Turn off the background automatic scan that kmemleak performs upon starting
> > + */
> > +static inline void kmemleak_automatic_scan_off(void)
> > +{
> > +     loff_t pos;
> > +     kmemleak_write(NULL, "scan=off", 9, &pos);
> > +}
> > +
> >  struct kunit_try_catch_context {
> >       struct kunit *test;
> >       struct kunit_suite *suite;
> > @@ -290,6 +312,12 @@ static void kunit_try_run_case(void *data)
> >       struct kunit_suite *suite = ctx->suite;
> >       struct kunit_case *test_case = ctx->test_case;
> >
> > +     /*
> > +      * Clear any reported memory leaks since last scan, so that only the
> > +      * leaks pertaining to the test case remain afterwards.
> > +      */
> > +     kmemleak_scan();
> > +
> >       current->kunit_test = test;
> >
> >       /*
> > @@ -298,7 +326,12 @@ static void kunit_try_run_case(void *data)
> >        * thread will resume control and handle any necessary clean up.
> >        */
> >       kunit_run_case_internal(test, suite, test_case);
> > -     /* This line may never be reached. */
> > +
> > +     /* These lines may never be reached. */
> > +
> > +     /* Report any detected memory leaks that occurred in the test case */
> > +     kmemleak_scan();
> > +
> >       kunit_run_case_cleanup(test, suite);
> >  }
> >
> > @@ -388,6 +421,7 @@ static void kunit_init_suite(struct kunit_suite *suite)
> >  int __kunit_test_suites_init(struct kunit_suite **suites)
> >  {
> >       unsigned int i;
> > +     kmemleak_automatic_scan_off();
> >
> >       for (i = 0; suites[i] != NULL; i++) {
> >               kunit_init_suite(suites[i]);
> > diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> > index e362dc3d2028..ad046c77e00c 100644
> > --- a/mm/kmemleak.c
> > +++ b/mm/kmemleak.c
> > @@ -99,15 +99,26 @@
> >  #include <linux/kasan.h>
> >  #include <linux/kmemleak.h>
> >  #include <linux/memory_hotplug.h>
> > +#include <kunit/test.h>
> >
> >  /*
> >   * Kmemleak configuration and common defines.
> >   */
> > -#define MAX_TRACE            16      /* stack trace length */
> > -#define MSECS_MIN_AGE                5000    /* minimum object age for reporting */
> > -#define SECS_FIRST_SCAN              60      /* delay before the first scan */
> > -#define SECS_SCAN_WAIT               600     /* subsequent auto scanning delay */
> > -#define MAX_SCAN_SIZE                4096    /* maximum size of a scanned block */
> > +
> > +/* stack trace length */
> > +#define MAX_TRACE            CONFIG_DEBUG_KMEMLEAK_MAX_TRACE
> > +
> > +/* minimum object age for reporting */
> > +#define MSECS_MIN_AGE                CONFIG_DEBUG_KMEMLEAK_MSECS_MIN_AGE
> > +
> > +/* delay before the first scan */
> > +#define SECS_FIRST_SCAN              CONFIG_DEBUG_KMEMLEAK_SECS_FIRST_SCAN
> > +
> > +/* subsequent auto scanning delay */
> > +#define SECS_SCAN_WAIT               CONFIG_DEBUG_KMEMLEAK_SECS_SCAN_WAIT
> > +
> > +/* maximum size of a scanned lock */
> > +#define MAX_SCAN_SIZE                CONFIG_DEBUG_KMEMLEAK_MAX_SCAN_SIZE
> >
> >  #define BYTES_PER_POINTER    sizeof(void *)
> >
> > @@ -1490,6 +1501,7 @@ static void kmemleak_scan(void)
> >        * Check for new or unreferenced objects modified since the previous
> >        * scan and color them gray until the next scan.
> >        */
> > +#if (!IS_ENABLED(CONFIG_KUNIT))
> >       rcu_read_lock();
> >       list_for_each_entry_rcu(object, &object_list, object_list) {
> >               raw_spin_lock_irqsave(&object->lock, flags);
> > @@ -1502,6 +1514,7 @@ static void kmemleak_scan(void)
> >               raw_spin_unlock_irqrestore(&object->lock, flags);
> >       }
> >       rcu_read_unlock();
> > +#endif
> >
> >       /*
> >        * Re-scan the gray list for modified unreferenced objects.
> > @@ -1534,6 +1547,8 @@ static void kmemleak_scan(void)
> >       rcu_read_unlock();
> >
> >       if (new_leaks) {
> > +             kunit_fail_current_test();
> > +
> >               kmemleak_found_leaks = true;
> >
> >               pr_info("%d new suspected memory leaks (see /sys/kernel/debug/kmemleak)\n",
> > @@ -1764,7 +1779,7 @@ static void __kmemleak_do_cleanup(void);
> >   *             if kmemleak has been disabled.
> >   *   dump=...        - dump information about the object found at the given address
> >   */
> > -static ssize_t kmemleak_write(struct file *file, const char __user *user_buf,
> > +ssize_t kmemleak_write(struct file *file, const char __user *user_buf,
> >                             size_t size, loff_t *ppos)
> >  {
> >       char buf[64];
> > --
> > 2.27.0.212.ge8ba1cc988-goog
> >
> >

Re: [PATCH 2/2] kunit: kmemleak integration

[Qian Cai]

On Mon, Jul 06, 2020 at 05:48:21PM -0500, Uriel Guajardo wrote:
> On Mon, Jul 6, 2020 at 4:39 PM Qian Cai <cai@lca.pw> wrote:
> >
> > On Mon, Jul 06, 2020 at 09:13:09PM +0000, Uriel Guajardo wrote:
> > > From: Uriel Guajardo <urielguajardo@google.com>
> > >
> > > Integrate kmemleak into the KUnit testing framework.
> > >
> > > Kmemleak will now fail the currently running KUnit test case if it finds
> > > any memory leaks.
> > >
> > > The minimum object age for reporting is set to 0 msecs so that leaks are
> > > not ignored if the test case finishes too quickly.
> > >
> > > Signed-off-by: Uriel Guajardo <urielguajardo@google.com>
> > > ---
> > >  include/linux/kmemleak.h | 11 +++++++++++
> > >  lib/Kconfig.debug        | 26 ++++++++++++++++++++++++++
> > >  lib/kunit/test.c         | 36 +++++++++++++++++++++++++++++++++++-
> > >  mm/kmemleak.c            | 27 +++++++++++++++++++++------
> > >  4 files changed, 93 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/linux/kmemleak.h b/include/linux/kmemleak.h
> > > index 34684b2026ab..0da427934462 100644
> > > --- a/include/linux/kmemleak.h
> > > +++ b/include/linux/kmemleak.h
> > > @@ -35,6 +35,10 @@ extern void kmemleak_free_part_phys(phys_addr_t phys, size_t size) __ref;
> > >  extern void kmemleak_not_leak_phys(phys_addr_t phys) __ref;
> > >  extern void kmemleak_ignore_phys(phys_addr_t phys) __ref;
> > >
> > > +extern ssize_t kmemleak_write(struct file *file,
> > > +                           const char __user *user_buf,
> > > +                           size_t size, loff_t *ppos);
> > > +
> > >  static inline void kmemleak_alloc_recursive(const void *ptr, size_t size,
> > >                                           int min_count, slab_flags_t flags,
> > >                                           gfp_t gfp)
> > > @@ -120,6 +124,13 @@ static inline void kmemleak_ignore_phys(phys_addr_t phys)
> > >  {
> > >  }
> > >
> > > +static inline ssize_t kmemleak_write(struct file *file,
> > > +                                  const char __user *user_buf,
> > > +                                  size_t size, loff_t *ppos)
> > > +{
> > > +     return -1;
> > > +}
> > > +
> > >  #endif       /* CONFIG_DEBUG_KMEMLEAK */
> > >
> > >  #endif       /* __KMEMLEAK_H */
> > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > > index 21d9c5f6e7ec..e9c492cb3f4d 100644
> > > --- a/lib/Kconfig.debug
> > > +++ b/lib/Kconfig.debug
> > > @@ -602,6 +602,32 @@ config DEBUG_KMEMLEAK_MEM_POOL_SIZE
> > >         fully initialised, this memory pool acts as an emergency one
> > >         if slab allocations fail.
> > >
> > > +config DEBUG_KMEMLEAK_MAX_TRACE
> > > +     int "Kmemleak stack trace length"
> > > +     depends on DEBUG_KMEMLEAK
> > > +     default 16
> > > +
> > > +config DEBUG_KMEMLEAK_MSECS_MIN_AGE
> > > +     int "Minimum object age before reporting in msecs"
> > > +     depends on DEBUG_KMEMLEAK
> > > +     default 0 if KUNIT
> > > +     default 5000
> > > +
> > > +config DEBUG_KMEMLEAK_SECS_FIRST_SCAN
> > > +     int "Delay before first scan in secs"
> > > +     depends on DEBUG_KMEMLEAK
> > > +     default 60
> > > +
> > > +config DEBUG_KMEMLEAK_SECS_SCAN_WAIT
> > > +     int "Delay before subsequent auto scans in secs"
> > > +     depends on DEBUG_KMEMLEAK
> > > +     default 600
> > > +
> > > +config DEBUG_KMEMLEAK_MAX_SCAN_SIZE
> > > +     int "Maximum size of scanned block"
> > > +     depends on DEBUG_KMEMLEAK
> > > +     default 4096
> > > +
> >
> > Why do you make those configurable? I don't see anywhere you make use of
> > them except DEBUG_KMEMLEAK_MSECS_MIN_AGE?
> >
> 
> That's correct. Strictly speaking, only DEBUG_KMEMLEAK_MSECS_MIN_AGE
> is used to set a default when KUnit is configured.
> 
> There is no concrete reason why these other variables need to be
> configurable. At the time of writing this, it seemed to make the most
> sense to configure the other configuration options, given that I was
> already going to make MSECS_MIN_AGE configurable. It can definitely be
> taken out.
> 
> > Even then, how setting DEBUG_KMEMLEAK_MSECS_MIN_AGE=0 not giving too
> > many false positives? Kmemleak simply does not work that instantly.
> >
> 
> I did not experience this issue, but I see your point.
> 
> An alternative that I was thinking about -- and one that is not in
> this patch -- is to wait DEBUG_KMEMLEAK_MSECS_MIN_AGE after each test
> case in a test suite, while leaving kmemleak's default value as is. I
> was hesitant to do this initially because many KUnit test cases run
> quick, so this may result in a lot of time just waiting. But if we
> leave it configurable, the user can change this as needed and deal
> with the possible false positives.

I doubt that is good idea. We don't really want people to start
reporting those false positives to the MLs just because some kunit tests
starts to flag them. It is wasting everyone's time. Reports from
DEBUG_KMEMLEAK_MSECS_MIN_AGE=0 are simply trustful. I don't think there
is a way around. Kmemleak was designed to have a lot of
waitings/re-scans to be useful not even mentioning kfree_rcu() etc until
it is redesigned...

Re: [PATCH 2/2] kunit: kmemleak integration

[Uriel Guajardo]

On Mon, Jul 6, 2020 at 6:17 PM Qian Cai <cai@lca.pw> wrote:
>
> On Mon, Jul 06, 2020 at 05:48:21PM -0500, Uriel Guajardo wrote:
> > On Mon, Jul 6, 2020 at 4:39 PM Qian Cai <cai@lca.pw> wrote:
> > >
> > > On Mon, Jul 06, 2020 at 09:13:09PM +0000, Uriel Guajardo wrote:
> > > > From: Uriel Guajardo <urielguajardo@google.com>
> > > >
> > > > Integrate kmemleak into the KUnit testing framework.
> > > >
> > > > Kmemleak will now fail the currently running KUnit test case if it finds
> > > > any memory leaks.
> > > >
> > > > The minimum object age for reporting is set to 0 msecs so that leaks are
> > > > not ignored if the test case finishes too quickly.
> > > >
> > > > Signed-off-by: Uriel Guajardo <urielguajardo@google.com>
> > > > ---
> > > >  include/linux/kmemleak.h | 11 +++++++++++
> > > >  lib/Kconfig.debug        | 26 ++++++++++++++++++++++++++
> > > >  lib/kunit/test.c         | 36 +++++++++++++++++++++++++++++++++++-
> > > >  mm/kmemleak.c            | 27 +++++++++++++++++++++------
> > > >  4 files changed, 93 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/include/linux/kmemleak.h b/include/linux/kmemleak.h
> > > > index 34684b2026ab..0da427934462 100644
> > > > --- a/include/linux/kmemleak.h
> > > > +++ b/include/linux/kmemleak.h
> > > > @@ -35,6 +35,10 @@ extern void kmemleak_free_part_phys(phys_addr_t phys, size_t size) __ref;
> > > >  extern void kmemleak_not_leak_phys(phys_addr_t phys) __ref;
> > > >  extern void kmemleak_ignore_phys(phys_addr_t phys) __ref;
> > > >
> > > > +extern ssize_t kmemleak_write(struct file *file,
> > > > +                           const char __user *user_buf,
> > > > +                           size_t size, loff_t *ppos);
> > > > +
> > > >  static inline void kmemleak_alloc_recursive(const void *ptr, size_t size,
> > > >                                           int min_count, slab_flags_t flags,
> > > >                                           gfp_t gfp)
> > > > @@ -120,6 +124,13 @@ static inline void kmemleak_ignore_phys(phys_addr_t phys)
> > > >  {
> > > >  }
> > > >
> > > > +static inline ssize_t kmemleak_write(struct file *file,
> > > > +                                  const char __user *user_buf,
> > > > +                                  size_t size, loff_t *ppos)
> > > > +{
> > > > +     return -1;
> > > > +}
> > > > +
> > > >  #endif       /* CONFIG_DEBUG_KMEMLEAK */
> > > >
> > > >  #endif       /* __KMEMLEAK_H */
> > > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > > > index 21d9c5f6e7ec..e9c492cb3f4d 100644
> > > > --- a/lib/Kconfig.debug
> > > > +++ b/lib/Kconfig.debug
> > > > @@ -602,6 +602,32 @@ config DEBUG_KMEMLEAK_MEM_POOL_SIZE
> > > >         fully initialised, this memory pool acts as an emergency one
> > > >         if slab allocations fail.
> > > >
> > > > +config DEBUG_KMEMLEAK_MAX_TRACE
> > > > +     int "Kmemleak stack trace length"
> > > > +     depends on DEBUG_KMEMLEAK
> > > > +     default 16
> > > > +
> > > > +config DEBUG_KMEMLEAK_MSECS_MIN_AGE
> > > > +     int "Minimum object age before reporting in msecs"
> > > > +     depends on DEBUG_KMEMLEAK
> > > > +     default 0 if KUNIT
> > > > +     default 5000
> > > > +
> > > > +config DEBUG_KMEMLEAK_SECS_FIRST_SCAN
> > > > +     int "Delay before first scan in secs"
> > > > +     depends on DEBUG_KMEMLEAK
> > > > +     default 60
> > > > +
> > > > +config DEBUG_KMEMLEAK_SECS_SCAN_WAIT
> > > > +     int "Delay before subsequent auto scans in secs"
> > > > +     depends on DEBUG_KMEMLEAK
> > > > +     default 600
> > > > +
> > > > +config DEBUG_KMEMLEAK_MAX_SCAN_SIZE
> > > > +     int "Maximum size of scanned block"
> > > > +     depends on DEBUG_KMEMLEAK
> > > > +     default 4096
> > > > +
> > >
> > > Why do you make those configurable? I don't see anywhere you make use of
> > > them except DEBUG_KMEMLEAK_MSECS_MIN_AGE?
> > >
> >
> > That's correct. Strictly speaking, only DEBUG_KMEMLEAK_MSECS_MIN_AGE
> > is used to set a default when KUnit is configured.
> >
> > There is no concrete reason why these other variables need to be
> > configurable. At the time of writing this, it seemed to make the most
> > sense to configure the other configuration options, given that I was
> > already going to make MSECS_MIN_AGE configurable. It can definitely be
> > taken out.
> >
> > > Even then, how setting DEBUG_KMEMLEAK_MSECS_MIN_AGE=0 not giving too
> > > many false positives? Kmemleak simply does not work that instantly.
> > >
> >
> > I did not experience this issue, but I see your point.
> >
> > An alternative that I was thinking about -- and one that is not in
> > this patch -- is to wait DEBUG_KMEMLEAK_MSECS_MIN_AGE after each test
> > case in a test suite, while leaving kmemleak's default value as is. I
> > was hesitant to do this initially because many KUnit test cases run
> > quick, so this may result in a lot of time just waiting. But if we
> > leave it configurable, the user can change this as needed and deal
> > with the possible false positives.
>
> I doubt that is good idea. We don't really want people to start
> reporting those false positives to the MLs just because some kunit tests
> starts to flag them. It is wasting everyone's time. Reports from
> DEBUG_KMEMLEAK_MSECS_MIN_AGE=0 are simply trustful. I don't think there
> is a way around. Kmemleak was designed to have a lot of
> waitings/re-scans to be useful not even mentioning kfree_rcu() etc until
> it is redesigned...

I agree with your statement about false positives.
Is your suggestion to not make MSECS_MIN_AGE configurable and have
KUnit wait after each test case? Or are you saying that this will not
work entirely?
It seems like kmemleak should be able to work in some fashion under
KUnit, since it has specific documentation over testing parts of code
(https://www.kernel.org/doc/html/latest/dev-tools/kmemleak.html#testing-specific-sections-with-kmemleak).

Re: [PATCH 2/2] kunit: kmemleak integration

[Qian Cai]

On Tue, Jul 07, 2020 at 12:26:52PM -0500, Uriel Guajardo wrote:
> On Mon, Jul 6, 2020 at 6:17 PM Qian Cai <cai@lca.pw> wrote:
> >
> > On Mon, Jul 06, 2020 at 05:48:21PM -0500, Uriel Guajardo wrote:
> > > On Mon, Jul 6, 2020 at 4:39 PM Qian Cai <cai@lca.pw> wrote:
> > > >
> > > > On Mon, Jul 06, 2020 at 09:13:09PM +0000, Uriel Guajardo wrote:
> > > > > From: Uriel Guajardo <urielguajardo@google.com>
> > > > >
> > > > > Integrate kmemleak into the KUnit testing framework.
> > > > >
> > > > > Kmemleak will now fail the currently running KUnit test case if it finds
> > > > > any memory leaks.
> > > > >
> > > > > The minimum object age for reporting is set to 0 msecs so that leaks are
> > > > > not ignored if the test case finishes too quickly.
> > > > >
> > > > > Signed-off-by: Uriel Guajardo <urielguajardo@google.com>
> > > > > ---
> > > > >  include/linux/kmemleak.h | 11 +++++++++++
> > > > >  lib/Kconfig.debug        | 26 ++++++++++++++++++++++++++
> > > > >  lib/kunit/test.c         | 36 +++++++++++++++++++++++++++++++++++-
> > > > >  mm/kmemleak.c            | 27 +++++++++++++++++++++------
> > > > >  4 files changed, 93 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/kmemleak.h b/include/linux/kmemleak.h
> > > > > index 34684b2026ab..0da427934462 100644
> > > > > --- a/include/linux/kmemleak.h
> > > > > +++ b/include/linux/kmemleak.h
> > > > > @@ -35,6 +35,10 @@ extern void kmemleak_free_part_phys(phys_addr_t phys, size_t size) __ref;
> > > > >  extern void kmemleak_not_leak_phys(phys_addr_t phys) __ref;
> > > > >  extern void kmemleak_ignore_phys(phys_addr_t phys) __ref;
> > > > >
> > > > > +extern ssize_t kmemleak_write(struct file *file,
> > > > > +                           const char __user *user_buf,
> > > > > +                           size_t size, loff_t *ppos);
> > > > > +
> > > > >  static inline void kmemleak_alloc_recursive(const void *ptr, size_t size,
> > > > >                                           int min_count, slab_flags_t flags,
> > > > >                                           gfp_t gfp)
> > > > > @@ -120,6 +124,13 @@ static inline void kmemleak_ignore_phys(phys_addr_t phys)
> > > > >  {
> > > > >  }
> > > > >
> > > > > +static inline ssize_t kmemleak_write(struct file *file,
> > > > > +                                  const char __user *user_buf,
> > > > > +                                  size_t size, loff_t *ppos)
> > > > > +{
> > > > > +     return -1;
> > > > > +}
> > > > > +
> > > > >  #endif       /* CONFIG_DEBUG_KMEMLEAK */
> > > > >
> > > > >  #endif       /* __KMEMLEAK_H */
> > > > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > > > > index 21d9c5f6e7ec..e9c492cb3f4d 100644
> > > > > --- a/lib/Kconfig.debug
> > > > > +++ b/lib/Kconfig.debug
> > > > > @@ -602,6 +602,32 @@ config DEBUG_KMEMLEAK_MEM_POOL_SIZE
> > > > >         fully initialised, this memory pool acts as an emergency one
> > > > >         if slab allocations fail.
> > > > >
> > > > > +config DEBUG_KMEMLEAK_MAX_TRACE
> > > > > +     int "Kmemleak stack trace length"
> > > > > +     depends on DEBUG_KMEMLEAK
> > > > > +     default 16
> > > > > +
> > > > > +config DEBUG_KMEMLEAK_MSECS_MIN_AGE
> > > > > +     int "Minimum object age before reporting in msecs"
> > > > > +     depends on DEBUG_KMEMLEAK
> > > > > +     default 0 if KUNIT
> > > > > +     default 5000
> > > > > +
> > > > > +config DEBUG_KMEMLEAK_SECS_FIRST_SCAN
> > > > > +     int "Delay before first scan in secs"
> > > > > +     depends on DEBUG_KMEMLEAK
> > > > > +     default 60
> > > > > +
> > > > > +config DEBUG_KMEMLEAK_SECS_SCAN_WAIT
> > > > > +     int "Delay before subsequent auto scans in secs"
> > > > > +     depends on DEBUG_KMEMLEAK
> > > > > +     default 600
> > > > > +
> > > > > +config DEBUG_KMEMLEAK_MAX_SCAN_SIZE
> > > > > +     int "Maximum size of scanned block"
> > > > > +     depends on DEBUG_KMEMLEAK
> > > > > +     default 4096
> > > > > +
> > > >
> > > > Why do you make those configurable? I don't see anywhere you make use of
> > > > them except DEBUG_KMEMLEAK_MSECS_MIN_AGE?
> > > >
> > >
> > > That's correct. Strictly speaking, only DEBUG_KMEMLEAK_MSECS_MIN_AGE
> > > is used to set a default when KUnit is configured.
> > >
> > > There is no concrete reason why these other variables need to be
> > > configurable. At the time of writing this, it seemed to make the most
> > > sense to configure the other configuration options, given that I was
> > > already going to make MSECS_MIN_AGE configurable. It can definitely be
> > > taken out.
> > >
> > > > Even then, how setting DEBUG_KMEMLEAK_MSECS_MIN_AGE=0 not giving too
> > > > many false positives? Kmemleak simply does not work that instantly.
> > > >
> > >
> > > I did not experience this issue, but I see your point.
> > >
> > > An alternative that I was thinking about -- and one that is not in
> > > this patch -- is to wait DEBUG_KMEMLEAK_MSECS_MIN_AGE after each test
> > > case in a test suite, while leaving kmemleak's default value as is. I
> > > was hesitant to do this initially because many KUnit test cases run
> > > quick, so this may result in a lot of time just waiting. But if we
> > > leave it configurable, the user can change this as needed and deal
> > > with the possible false positives.
> >
> > I doubt that is good idea. We don't really want people to start
> > reporting those false positives to the MLs just because some kunit tests
> > starts to flag them. It is wasting everyone's time. Reports from
> > DEBUG_KMEMLEAK_MSECS_MIN_AGE=0 are simply trustful. I don't think there
> > is a way around. Kmemleak was designed to have a lot of
> > waitings/re-scans to be useful not even mentioning kfree_rcu() etc until
> > it is redesigned...
> 
> I agree with your statement about false positives.
> Is your suggestion to not make MSECS_MIN_AGE configurable and have
> KUnit wait after each test case? Or are you saying that this will not
> work entirely?
> It seems like kmemleak should be able to work in some fashion under
> KUnit, since it has specific documentation over testing parts of code
> (https://www.kernel.org/doc/html/latest/dev-tools/kmemleak.html#testing-specific-sections-with-kmemleak).

It is going to be tough. It is normal that sometimes when there is a
leak. It needs to rescan a few times to make sure it is stable.
Sometimes, even the real leaks will take quite a while to show up.

[PATCH 2/2] kunit: kmemleak integration

[Uriel Guajardo]

From: Uriel Guajardo <urielguajardo@google.com>

Integrate kmemleak into the KUnit testing framework.

Kmemleak will now fail the currently running KUnit test case if it finds
any memory leaks.

The minimum object age for reporting is set to 0 msecs so that leaks are
not ignored if the test case finishes too quickly.

Signed-off-by: Uriel Guajardo <urielguajardo@google.com>
---
 include/linux/kmemleak.h | 11 +++++++++++
 lib/Kconfig.debug        | 26 ++++++++++++++++++++++++++
 lib/kunit/test.c         | 36 +++++++++++++++++++++++++++++++++++-
 mm/kmemleak.c            | 27 +++++++++++++++++++++------
 4 files changed, 93 insertions(+), 7 deletions(-)

diff --git a/include/linux/kmemleak.h b/include/linux/kmemleak.h
index 34684b2026ab..0da427934462 100644
--- a/include/linux/kmemleak.h
+++ b/include/linux/kmemleak.h
@@ -35,6 +35,10 @@ extern void kmemleak_free_part_phys(phys_addr_t phys, size_t size) __ref;
 extern void kmemleak_not_leak_phys(phys_addr_t phys) __ref;
 extern void kmemleak_ignore_phys(phys_addr_t phys) __ref;
 
+extern ssize_t kmemleak_write(struct file *file,
+			      const char __user *user_buf,
+			      size_t size, loff_t *ppos);
+
 static inline void kmemleak_alloc_recursive(const void *ptr, size_t size,
 					    int min_count, slab_flags_t flags,
 					    gfp_t gfp)
@@ -120,6 +124,13 @@ static inline void kmemleak_ignore_phys(phys_addr_t phys)
 {
 }
 
+static inline ssize_t kmemleak_write(struct file *file,
+				     const char __user *user_buf,
+				     size_t size, loff_t *ppos)
+{
+	return -1;
+}
+
 #endif	/* CONFIG_DEBUG_KMEMLEAK */
 
 #endif	/* __KMEMLEAK_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 21d9c5f6e7ec..e9c492cb3f4d 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -602,6 +602,32 @@ config DEBUG_KMEMLEAK_MEM_POOL_SIZE
 	  fully initialised, this memory pool acts as an emergency one
 	  if slab allocations fail.
 
+config DEBUG_KMEMLEAK_MAX_TRACE
+	int "Kmemleak stack trace length"
+	depends on DEBUG_KMEMLEAK
+	default 16
+
+config DEBUG_KMEMLEAK_MSECS_MIN_AGE
+	int "Minimum object age before reporting in msecs"
+	depends on DEBUG_KMEMLEAK
+	default 0 if KUNIT
+	default 5000
+
+config DEBUG_KMEMLEAK_SECS_FIRST_SCAN
+	int "Delay before first scan in secs"
+	depends on DEBUG_KMEMLEAK
+	default 60
+
+config DEBUG_KMEMLEAK_SECS_SCAN_WAIT
+	int "Delay before subsequent auto scans in secs"
+	depends on DEBUG_KMEMLEAK
+	default 600
+
+config DEBUG_KMEMLEAK_MAX_SCAN_SIZE
+	int "Maximum size of scanned block"
+	depends on DEBUG_KMEMLEAK
+	default 4096
+
 config DEBUG_KMEMLEAK_TEST
 	tristate "Simple test for the kernel memory leak detector"
 	depends on DEBUG_KMEMLEAK && m
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 8580ed831a8f..8d113a6a214b 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -11,6 +11,7 @@
 #include <linux/kref.h>
 #include <linux/sched/debug.h>
 #include <linux/sched.h>
+#include <linux/kmemleak.h>
 
 #include "debugfs.h"
 #include "string-stream.h"
@@ -277,6 +278,27 @@ static void kunit_run_case_cleanup(struct kunit *test,
 	kunit_case_internal_cleanup(test);
 }
 
+/*
+ * Manually scans for memory leaks using the kmemleak tool.
+ *
+ * Any leaks that occurred since the previous scan will be
+ * reported and will cause the currently running test to fail.
+ */
+static inline void kmemleak_scan(void)
+{
+	loff_t pos;
+	kmemleak_write(NULL, "scan", 5, &pos);
+}
+
+/*
+ * Turn off the background automatic scan that kmemleak performs upon starting
+ */
+static inline void kmemleak_automatic_scan_off(void)
+{
+	loff_t pos;
+	kmemleak_write(NULL, "scan=off", 9, &pos);
+}
+
 struct kunit_try_catch_context {
 	struct kunit *test;
 	struct kunit_suite *suite;
@@ -290,6 +312,12 @@ static void kunit_try_run_case(void *data)
 	struct kunit_suite *suite = ctx->suite;
 	struct kunit_case *test_case = ctx->test_case;
 
+	/*
+	 * Clear any reported memory leaks since last scan, so that only the
+	 * leaks pertaining to the test case remain afterwards.
+	 */
+	kmemleak_scan();
+
 	current->kunit_test = test;
 
 	/*
@@ -298,7 +326,12 @@ static void kunit_try_run_case(void *data)
 	 * thread will resume control and handle any necessary clean up.
 	 */
 	kunit_run_case_internal(test, suite, test_case);
-	/* This line may never be reached. */
+
+	/* These lines may never be reached. */
+
+	/* Report any detected memory leaks that occurred in the test case */
+	kmemleak_scan();
+
 	kunit_run_case_cleanup(test, suite);
 }
 
@@ -388,6 +421,7 @@ static void kunit_init_suite(struct kunit_suite *suite)
 int __kunit_test_suites_init(struct kunit_suite **suites)
 {
 	unsigned int i;
+	kmemleak_automatic_scan_off();
 
 	for (i = 0; suites[i] != NULL; i++) {
 		kunit_init_suite(suites[i]);
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index e362dc3d2028..ad046c77e00c 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -99,15 +99,26 @@
 #include <linux/kasan.h>
 #include <linux/kmemleak.h>
 #include <linux/memory_hotplug.h>
+#include <kunit/test.h>
 
 /*
  * Kmemleak configuration and common defines.
  */
-#define MAX_TRACE		16	/* stack trace length */
-#define MSECS_MIN_AGE		5000	/* minimum object age for reporting */
-#define SECS_FIRST_SCAN		60	/* delay before the first scan */
-#define SECS_SCAN_WAIT		600	/* subsequent auto scanning delay */
-#define MAX_SCAN_SIZE		4096	/* maximum size of a scanned block */
+
+/* stack trace length */
+#define MAX_TRACE		CONFIG_DEBUG_KMEMLEAK_MAX_TRACE
+
+/* minimum object age for reporting */
+#define MSECS_MIN_AGE		CONFIG_DEBUG_KMEMLEAK_MSECS_MIN_AGE
+
+/* delay before the first scan */
+#define SECS_FIRST_SCAN		CONFIG_DEBUG_KMEMLEAK_SECS_FIRST_SCAN
+
+/* subsequent auto scanning delay */
+#define SECS_SCAN_WAIT		CONFIG_DEBUG_KMEMLEAK_SECS_SCAN_WAIT
+
+/* maximum size of a scanned lock */
+#define MAX_SCAN_SIZE		CONFIG_DEBUG_KMEMLEAK_MAX_SCAN_SIZE
 
 #define BYTES_PER_POINTER	sizeof(void *)
 
@@ -1490,6 +1501,7 @@ static void kmemleak_scan(void)
 	 * Check for new or unreferenced objects modified since the previous
 	 * scan and color them gray until the next scan.
 	 */
+#if (!IS_ENABLED(CONFIG_KUNIT))
 	rcu_read_lock();
 	list_for_each_entry_rcu(object, &object_list, object_list) {
 		raw_spin_lock_irqsave(&object->lock, flags);
@@ -1502,6 +1514,7 @@ static void kmemleak_scan(void)
 		raw_spin_unlock_irqrestore(&object->lock, flags);
 	}
 	rcu_read_unlock();
+#endif
 
 	/*
 	 * Re-scan the gray list for modified unreferenced objects.
@@ -1534,6 +1547,8 @@ static void kmemleak_scan(void)
 	rcu_read_unlock();
 
 	if (new_leaks) {
+		kunit_fail_current_test();
+
 		kmemleak_found_leaks = true;
 
 		pr_info("%d new suspected memory leaks (see /sys/kernel/debug/kmemleak)\n",
@@ -1764,7 +1779,7 @@ static void __kmemleak_do_cleanup(void);
  *		  if kmemleak has been disabled.
  *   dump=...	- dump information about the object found at the given address
  */
-static ssize_t kmemleak_write(struct file *file, const char __user *user_buf,
+ssize_t kmemleak_write(struct file *file, const char __user *user_buf,
 			      size_t size, loff_t *ppos)
 {
 	char buf[64];
-- 
2.27.0.212.ge8ba1cc988-goog