Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 7 Aug 2020 15:15:49 +0100 (IST)
From:   Alan Maguire <alan.maguire@oracle.com>
To:     Uriel Guajardo <urielguajardojr@gmail.com>
cc:     brendanhiggins@google.com, urielguajardo@google.com,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        kunit-dev@googlegroups.com
Subject: Re: [PATCH 1/2] kunit: support failure from dynamic analysis tools
In-Reply-To: <20200806174326.3577537-1-urielguajardojr@gmail.com>
Message-ID: <alpine.LRH.2.21.2008071510580.10525@localhost>
References: <20200806174326.3577537-1-urielguajardojr@gmail.com>
User-Agent: Alpine 2.21 (LRH 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 6 Aug 2020, Uriel Guajardo wrote:

> Adds an API to allow dynamic analysis tools to fail the currently
> running KUnit test case.
> 
> - Always places the kunit test in the task_struct to allow other tools
> to access the currently running KUnit test.
> 
> - Creates a new header file to avoid circular dependencies that could be
> created from the test.h file.
> 
> Requires KASAN-KUnit integration patch to access the kunit test from
> task_struct:
> 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 | 24 ++++++++++++++++++++++++
>  include/kunit/test.h     |  1 +
>  lib/kunit/test.c         | 10 ++++++----
>  3 files changed, 31 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..283c19ec328f
> --- /dev/null
> +++ b/include/kunit/test-bug.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * KUnit API allowing dynamic analysis tools to interact with KUnit tests
> + *
> + * Copyright (C) 2020, Google LLC.
> + * Author: Uriel Guajardo <urielguajardo@google.com>
> + */
> +
> +#ifndef _KUNIT_TEST_BUG_H
> +#define _KUNIT_TEST_BUG_H
> +
> +#if IS_ENABLED(CONFIG_KUNIT)
> +
> +extern void kunit_fail_current_test(void);
> +
> +#else
> +
> +static inline void kunit_fail_current_test(void)
> +{
> +}
> +
> +#endif
> +
> +#endif /* _KUNIT_TEST_BUG_H */

This is great stuff!

One thing I wonder though; how obvious will it be to someone
running a KUnit test that the cause of the test failure
is a dynamic analysis tool?  Yes we'll see the dmesg logging
from that tool but I don't think there's any context _within_
KUnit that could clarify the source of the failure.  What about
changing the above API to include a string message that KUnit can
log, so it can at least identify the source of the failure
(ubsan, kasan etc).  That would alert anyone looking at KUnit
output only that there's an external context to examine.

> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 3391f38389f8..81bf43a1abda 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 dcc35fd30d95..d8189d827368 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,
> @@ -602,9 +606,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.28.0.163.g6104cc2f0b6-goog
> 
>