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

[Qian Cai <cai@lca.pw>]

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
> 
>