Re: [PATCH v2 4/7] kunit: Handle test faults

["Mickaël Salaün" <mic@digikod.net>]

On Mon, Mar 11, 2024 at 05:21:11PM -0400, Rae Moar wrote:
> On Fri, Mar 1, 2024 at 2:40 PM Mickaël Salaün <mic@digikod.net> wrote:
> >
> > Previously, when a kernel test thread crashed (e.g. NULL pointer
> > dereference, general protection fault), the KUnit test hanged for 30
> > seconds and exited with a timeout error.
> >
> > Fix this issue by waiting on task_struct->vfork_done instead of the
> > custom kunit_try_catch.try_completion, and track the execution state by
> > initially setting try_result with -EFAULT and only setting it to 0 if
> 
> Hello!
> 
> Thanks for your patch! This has been tested and seems pretty good to
> me but I just have a few questions. First, do you mean here "setting
> try_result with -EINTR"  instead?

Good catch, I indeed meant -EINTR.

> 
> But happy to add the tested-by.
> 
> Tested-by: Rae Moar <rmoar@google.com>
> 
> Thanks!
> -Rae
> 
> > the test passed.
> >
> > Fix kunit_generic_run_threadfn_adapter() signature by returning 0
> > instead of calling kthread_complete_and_exit().  Because thread's exit
> > code is never checked, always set it to 0 to make it clear.
> >
> > Fix the -EINTR error message, which couldn't be reached until now.
> >
> > This is tested with a following patch.
> >
> > Cc: Brendan Higgins <brendanhiggins@google.com>
> > Cc: David Gow <davidgow@google.com>
> > Cc: Rae Moar <rmoar@google.com>
> > Cc: Shuah Khan <skhan@linuxfoundation.org>
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > Link: https://lore.kernel.org/r/20240301194037.532117-5-mic@digikod.net
> > ---
> >
> > Changes since v1:
> > * Added Kees's Reviewed-by.
> > ---
> >  include/kunit/try-catch.h |  3 ---
> >  lib/kunit/try-catch.c     | 14 +++++++-------
> >  2 files changed, 7 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
> > index c507dd43119d..7c966a1adbd3 100644
> > --- a/include/kunit/try-catch.h
> > +++ b/include/kunit/try-catch.h
> > @@ -14,13 +14,11 @@
> >
> >  typedef void (*kunit_try_catch_func_t)(void *);
> >
> > -struct completion;
> >  struct kunit;
> >
> >  /**
> >   * struct kunit_try_catch - provides a generic way to run code which might fail.
> >   * @test: The test case that is currently being executed.
> > - * @try_completion: Completion that the control thread waits on while test runs.
> >   * @try_result: Contains any errno obtained while running test case.
> >   * @try: The function, the test case, to attempt to run.
> >   * @catch: The function called if @try bails out.
> > @@ -46,7 +44,6 @@ struct kunit;
> >  struct kunit_try_catch {
> >         /* private: internal use only. */
> >         struct kunit *test;
> > -       struct completion *try_completion;
> >         int try_result;
> >         kunit_try_catch_func_t try;
> >         kunit_try_catch_func_t catch;
> > diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> > index cab8b24b5d5a..c6ee4db0b3bd 100644
> > --- a/lib/kunit/try-catch.c
> > +++ b/lib/kunit/try-catch.c
> > @@ -18,7 +18,7 @@
> >  void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch)
> >  {
> >         try_catch->try_result = -EFAULT;
> > -       kthread_complete_and_exit(try_catch->try_completion, -EFAULT);
> > +       kthread_exit(0);
> >  }
> >  EXPORT_SYMBOL_GPL(kunit_try_catch_throw);
> >
> > @@ -26,9 +26,12 @@ static int kunit_generic_run_threadfn_adapter(void *data)
> >  {
> >         struct kunit_try_catch *try_catch = data;
> >
> > +       try_catch->try_result = -EINTR;
> >         try_catch->try(try_catch->context);
> > +       if (try_catch->try_result == -EINTR)
> > +               try_catch->try_result = 0;
> >
> > -       kthread_complete_and_exit(try_catch->try_completion, 0);
> > +       return 0;
> 
> Really my only question is why we do not need to still do a
> kthread_exit(0) here? I realize we are not checking the thread's exit
> code but isn't it safer to call kthread_exit(). I'm new to kthread so
> I am not too sure.

This function is the body of the thread, and as we can see in the
signature it should return an integer that will then be passed to
kthread_exit() (by kthread-specific code).  It is then useless to
directly call kthread_exit() here, and it is cleaner to follow common
thread function signature.

> 
> >  }
> >
> >  static unsigned long kunit_test_timeout(void)
> > @@ -58,13 +61,11 @@ static unsigned long kunit_test_timeout(void)
> >
> >  void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
> >  {
> > -       DECLARE_COMPLETION_ONSTACK(try_completion);
> >         struct kunit *test = try_catch->test;
> >         struct task_struct *task_struct;
> >         int exit_code, time_remaining;
> >
> >         try_catch->context = context;
> > -       try_catch->try_completion = &try_completion;
> >         try_catch->try_result = 0;
> >         task_struct = kthread_create(kunit_generic_run_threadfn_adapter,
> >                                      try_catch, "kunit_try_catch_thread");
> > @@ -75,8 +76,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
> >         }
> >         get_task_struct(task_struct);
> >         wake_up_process(task_struct);
> > -
> > -       time_remaining = wait_for_completion_timeout(&try_completion,
> > +       time_remaining = wait_for_completion_timeout(task_struct->vfork_done,
> >                                                      kunit_test_timeout());
> >         if (time_remaining == 0) {
> >                 try_catch->try_result = -ETIMEDOUT;
> > @@ -92,7 +92,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
> >         if (exit_code == -EFAULT)
> >                 try_catch->try_result = 0;
> >         else if (exit_code == -EINTR)
> > -               kunit_err(test, "wake_up_process() was never called\n");
> > +               kunit_err(test, "try faulted\n");
> >         else if (exit_code == -ETIMEDOUT)
> >                 kunit_err(test, "try timed out\n");
> >         else if (exit_code)
> > --
> > 2.44.0
> >
>