| Message ID | 20200721174036.71072-1-vitor@massaru.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] lib: kunit: Provides a userspace memory context when tests are compiled as module | expand |
On Tue, Jul 21, 2020 at 02:40:36PM -0300, Vitor Massaru Iha wrote: > KUnit test cases run on kthreads, and kthreads don't have an > adddress space (current->mm is NULL), but processes have mm. > > The purpose of this patch is to allow to borrow mm to KUnit kthread > after userspace is brought up, because we know that there are processes > running, at least the process that loaded the module to borrow mm. > > This allows, for example, tests such as user_copy_kunit, which uses > vm_mmap, which needs current->mm. Ah! In the case of kunit starting before there IS a userspace... interesting. > Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> > --- > v2: > * splitted patch in 3: > - Allows to install and load modules in root filesystem; > - Provides an userspace memory context when tests are compiled > as module; > - Convert test_user_copy to KUnit test; > * added documentation; > * added more explanation; > * added a missed test pointer; > * released mm with mmput(); > v3: > * rebased with last kunit branch > * Please apply this commit from kunit-fixes: > 3f37d14b8a3152441f36b6bc74000996679f0998 > > Documentation/dev-tools/kunit/usage.rst | 14 ++++++++++++++ > include/kunit/test.h | 12 ++++++++++++ > lib/kunit/try-catch.c | 15 ++++++++++++++- > 3 files changed, 40 insertions(+), 1 deletion(-) > --- > diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst > index 3c3fe8b5fecc..9f909157be34 100644 > --- a/Documentation/dev-tools/kunit/usage.rst > +++ b/Documentation/dev-tools/kunit/usage.rst > @@ -448,6 +448,20 @@ We can now use it to test ``struct eeprom_buffer``: > > .. _kunit-on-non-uml: > > +User-space context > +------------------ > + > +I case you need a user-space context, for now this is only possible through typo: In case ... > +tests compiled as a module. And it will be necessary to use a root filesystem > +and uml_utilities. > + > +Example: > + > +.. code-block:: bash > + > + ./tools/testing/kunit/kunit.py run --timeout=60 --uml_rootfs_dir=.uml_rootfs > + > + > KUnit on non-UML architectures > ============================== > > diff --git a/include/kunit/test.h b/include/kunit/test.h > index 59f3144f009a..ae3337139c65 100644 > --- a/include/kunit/test.h > +++ b/include/kunit/test.h > @@ -222,6 +222,18 @@ struct kunit { > * protect it with some type of lock. > */ > struct list_head resources; /* Protected by lock. */ > + /* > + * KUnit test cases run on kthreads, and kthreads don't have an > + * adddress space (current->mm is NULL), but processes have mm. > + * > + * The purpose of this mm_struct is to allow to borrow mm to KUnit kthread > + * after userspace is brought up, because we know that there are processes > + * running, at least the process that loaded the module to borrow mm. > + * > + * This allows, for example, tests such as user_copy_kunit, which uses > + * vm_mmap, which needs current->mm. > + */ > + struct mm_struct *mm; I have a general concern that this will need more careful solving in the future as there are likely to be many tests that need a userspace context to operate sanely. But that's just a note; this solves the specific case now. > }; > > void kunit_init_test(struct kunit *test, const char *name, char *log); > diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c > index 0dd434e40487..d03e2093985b 100644 > --- a/lib/kunit/try-catch.c > +++ b/lib/kunit/try-catch.c > @@ -11,7 +11,8 @@ > #include <linux/completion.h> > #include <linux/kernel.h> > #include <linux/kthread.h> > - > +#include <linux/sched/mm.h> > +#include <linux/sched/task.h> > #include "try-catch-impl.h" > > void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch) > @@ -24,8 +25,17 @@ EXPORT_SYMBOL_GPL(kunit_try_catch_throw); > static int kunit_generic_run_threadfn_adapter(void *data) > { > struct kunit_try_catch *try_catch = data; > + struct kunit *test = try_catch->test; > + > + if (test != NULL && test->mm != NULL) > + kthread_use_mm(test->mm); > > try_catch->try(try_catch->context); > + if (test != NULL && test->mm != NULL) { > + kthread_unuse_mm(test->mm); > + mmput(test->mm); > + test->mm = NULL; This mmput() seems unbalanced... see below. > + } > > complete_and_exit(try_catch->try_completion, 0); > } > @@ -65,6 +75,9 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context) > try_catch->context = context; > try_catch->try_completion = &try_completion; > try_catch->try_result = 0; > + > + test->mm = get_task_mm(current); > + > task_struct = kthread_run(kunit_generic_run_threadfn_adapter, > try_catch, > "kunit_try_catch_thread"); Isn't there something that destroys a "struct kunit"? I would expect that to perform the mmput(). Why is it up in the threadfn? > > base-commit: d43c7fb05765152d4d4a39a8ef957c4ea14d8847 > -- > 2.26.2 >
On Tue, Jul 21, 2020 at 4:06 PM Kees Cook <keescook@chromium.org> wrote: > > On Tue, Jul 21, 2020 at 02:40:36PM -0300, Vitor Massaru Iha wrote: > > KUnit test cases run on kthreads, and kthreads don't have an > > adddress space (current->mm is NULL), but processes have mm. > > > > The purpose of this patch is to allow to borrow mm to KUnit kthread > > after userspace is brought up, because we know that there are processes > > running, at least the process that loaded the module to borrow mm. > > > > This allows, for example, tests such as user_copy_kunit, which uses > > vm_mmap, which needs current->mm. > > Ah! In the case of kunit starting before there IS a userspace... > interesting. I didn't think that way, but I can rewrite if it looked like that. > > Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> > > --- > > v2: > > * splitted patch in 3: > > - Allows to install and load modules in root filesystem; > > - Provides an userspace memory context when tests are compiled > > as module; > > - Convert test_user_copy to KUnit test; > > * added documentation; > > * added more explanation; > > * added a missed test pointer; > > * released mm with mmput(); > > v3: > > * rebased with last kunit branch > > * Please apply this commit from kunit-fixes: > > 3f37d14b8a3152441f36b6bc74000996679f0998 > > > > Documentation/dev-tools/kunit/usage.rst | 14 ++++++++++++++ > > include/kunit/test.h | 12 ++++++++++++ > > lib/kunit/try-catch.c | 15 ++++++++++++++- > > 3 files changed, 40 insertions(+), 1 deletion(-) > > --- > > diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst > > index 3c3fe8b5fecc..9f909157be34 100644 > > --- a/Documentation/dev-tools/kunit/usage.rst > > +++ b/Documentation/dev-tools/kunit/usage.rst > > @@ -448,6 +448,20 @@ We can now use it to test ``struct eeprom_buffer``: > > > > .. _kunit-on-non-uml: > > > > +User-space context > > +------------------ > > + > > +I case you need a user-space context, for now this is only possible through > > typo: In case ... Oops, thanks! > > > +tests compiled as a module. And it will be necessary to use a root filesystem > > +and uml_utilities. > > + > > +Example: > > + > > +.. code-block:: bash > > + > > + ./tools/testing/kunit/kunit.py run --timeout=60 --uml_rootfs_dir=.uml_rootfs > > + > > + > > KUnit on non-UML architectures > > ============================== > > > > diff --git a/include/kunit/test.h b/include/kunit/test.h > > index 59f3144f009a..ae3337139c65 100644 > > --- a/include/kunit/test.h > > +++ b/include/kunit/test.h > > @@ -222,6 +222,18 @@ struct kunit { > > * protect it with some type of lock. > > */ > > struct list_head resources; /* Protected by lock. */ > > + /* > > + * KUnit test cases run on kthreads, and kthreads don't have an > > + * adddress space (current->mm is NULL), but processes have mm. > > + * > > + * The purpose of this mm_struct is to allow to borrow mm to KUnit kthread > > + * after userspace is brought up, because we know that there are processes > > + * running, at least the process that loaded the module to borrow mm. > > + * > > + * This allows, for example, tests such as user_copy_kunit, which uses > > + * vm_mmap, which needs current->mm. > > + */ > > + struct mm_struct *mm; > > I have a general concern that this will need more careful solving in the > future as there are likely to be many tests that need a userspace > context to operate sanely. But that's just a note; this solves the > specific case now. > > > }; > > > > void kunit_init_test(struct kunit *test, const char *name, char *log); > > diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c > > index 0dd434e40487..d03e2093985b 100644 > > --- a/lib/kunit/try-catch.c > > +++ b/lib/kunit/try-catch.c > > @@ -11,7 +11,8 @@ > > #include <linux/completion.h> > > #include <linux/kernel.h> > > #include <linux/kthread.h> > > - > > +#include <linux/sched/mm.h> > > +#include <linux/sched/task.h> > > #include "try-catch-impl.h" > > > > void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch) > > @@ -24,8 +25,17 @@ EXPORT_SYMBOL_GPL(kunit_try_catch_throw); > > static int kunit_generic_run_threadfn_adapter(void *data) > > { > > struct kunit_try_catch *try_catch = data; > > + struct kunit *test = try_catch->test; > > + > > + if (test != NULL && test->mm != NULL) > > + kthread_use_mm(test->mm); > > > > try_catch->try(try_catch->context); > > + if (test != NULL && test->mm != NULL) { > > + kthread_unuse_mm(test->mm); > > + mmput(test->mm); > > + test->mm = NULL; > > This mmput() seems unbalanced... see below. > > > + } > > > > complete_and_exit(try_catch->try_completion, 0); > > } > > @@ -65,6 +75,9 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context) > > try_catch->context = context; > > try_catch->try_completion = &try_completion; > > try_catch->try_result = 0; > > + > > + test->mm = get_task_mm(current); > > + > > task_struct = kthread_run(kunit_generic_run_threadfn_adapter, > > try_catch, > > "kunit_try_catch_thread"); > > Isn't there something that destroys a "struct kunit"? I would expect > that to perform the mmput(). Why is it up in the threadfn? My bad. From what I saw, there's nothing that would destroy "struct kunit", but I will fix this umbalance. Thanks for the review!
On Tue, Jul 21, 2020 at 10:40 AM Vitor Massaru Iha <vitor@massaru.org> wrote: > > KUnit test cases run on kthreads, and kthreads don't have an > adddress space (current->mm is NULL), but processes have mm. > > The purpose of this patch is to allow to borrow mm to KUnit kthread > after userspace is brought up, because we know that there are processes > running, at least the process that loaded the module to borrow mm. > > This allows, for example, tests such as user_copy_kunit, which uses > vm_mmap, which needs current->mm. > > Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> Isn't this a dependency for the test_user_copy? Also, don't we also need the change that makes kunit_tool build modules? It seems like you should put these together in a patchset like you did before. If not, you should at least reference the dependencies in each patch.
diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst index 3c3fe8b5fecc..9f909157be34 100644 --- a/Documentation/dev-tools/kunit/usage.rst +++ b/Documentation/dev-tools/kunit/usage.rst @@ -448,6 +448,20 @@ We can now use it to test ``struct eeprom_buffer``: .. _kunit-on-non-uml: +User-space context +------------------ + +I case you need a user-space context, for now this is only possible through +tests compiled as a module. And it will be necessary to use a root filesystem +and uml_utilities. + +Example: + +.. code-block:: bash + + ./tools/testing/kunit/kunit.py run --timeout=60 --uml_rootfs_dir=.uml_rootfs + + KUnit on non-UML architectures ============================== diff --git a/include/kunit/test.h b/include/kunit/test.h index 59f3144f009a..ae3337139c65 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -222,6 +222,18 @@ struct kunit { * protect it with some type of lock. */ struct list_head resources; /* Protected by lock. */ + /* + * KUnit test cases run on kthreads, and kthreads don't have an + * adddress space (current->mm is NULL), but processes have mm. + * + * The purpose of this mm_struct is to allow to borrow mm to KUnit kthread + * after userspace is brought up, because we know that there are processes + * running, at least the process that loaded the module to borrow mm. + * + * This allows, for example, tests such as user_copy_kunit, which uses + * vm_mmap, which needs current->mm. + */ + struct mm_struct *mm; }; void kunit_init_test(struct kunit *test, const char *name, char *log); diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c index 0dd434e40487..d03e2093985b 100644 --- a/lib/kunit/try-catch.c +++ b/lib/kunit/try-catch.c @@ -11,7 +11,8 @@ #include <linux/completion.h> #include <linux/kernel.h> #include <linux/kthread.h> - +#include <linux/sched/mm.h> +#include <linux/sched/task.h> #include "try-catch-impl.h" void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch) @@ -24,8 +25,17 @@ EXPORT_SYMBOL_GPL(kunit_try_catch_throw); static int kunit_generic_run_threadfn_adapter(void *data) { struct kunit_try_catch *try_catch = data; + struct kunit *test = try_catch->test; + + if (test != NULL && test->mm != NULL) + kthread_use_mm(test->mm); try_catch->try(try_catch->context); + if (test != NULL && test->mm != NULL) { + kthread_unuse_mm(test->mm); + mmput(test->mm); + test->mm = NULL; + } complete_and_exit(try_catch->try_completion, 0); } @@ -65,6 +75,9 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context) try_catch->context = context; try_catch->try_completion = &try_completion; try_catch->try_result = 0; + + test->mm = get_task_mm(current); + task_struct = kthread_run(kunit_generic_run_threadfn_adapter, try_catch, "kunit_try_catch_thread");
KUnit test cases run on kthreads, and kthreads don't have an adddress space (current->mm is NULL), but processes have mm. The purpose of this patch is to allow to borrow mm to KUnit kthread after userspace is brought up, because we know that there are processes running, at least the process that loaded the module to borrow mm. This allows, for example, tests such as user_copy_kunit, which uses vm_mmap, which needs current->mm. Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> --- v2: * splitted patch in 3: - Allows to install and load modules in root filesystem; - Provides an userspace memory context when tests are compiled as module; - Convert test_user_copy to KUnit test; * added documentation; * added more explanation; * added a missed test pointer; * released mm with mmput(); v3: * rebased with last kunit branch * Please apply this commit from kunit-fixes: 3f37d14b8a3152441f36b6bc74000996679f0998 Documentation/dev-tools/kunit/usage.rst | 14 ++++++++++++++ include/kunit/test.h | 12 ++++++++++++ lib/kunit/try-catch.c | 15 ++++++++++++++- 3 files changed, 40 insertions(+), 1 deletion(-) --- base-commit: d43c7fb05765152d4d4a39a8ef957c4ea14d8847 -- 2.26.2