mocking init_task ?

mocking init_task ?

[Kees Cook]

Hi,

I want to make a unit test for kill_something_info(), as there is a patch
to fix a bug with it not working as expected under a specific process
tree arrangement[1]. This seems like a great candidate for a unit test:
given a specific state, return a specific result. Emboldened, I applied
the "kunit: Support redirecting function calls" series[2], preparing to
mock group_send_sig_info(), and ran head-long into for_each_process()
... which uses the address of the global init_task:

#define for_each_process(p) \
        for (p = &init_task ; (p = next_task(p)) != &init_task ; )

:(

I'm curious what you think might be the right approach to mock
init_task, or for_each_process(), so I can apply unit tests to some of
the "simple" process tree walkers...

One idea I had was using the "kunit: Provide a static key to check if
KUnit is actively running tests" series[3], and do something like this:

#ifndef CONFIG_KUNIT
#define init_task_ptr	&init_task
#else
#define init_task_ptr	({					\
		struct task_struct *task = &init_task;		\
		if (static_branch_unlikely(&kunit_running)) {	\
			struct kunit *test;			\
			test = current->kunit_test;		\
			if (test->mock_init_task)		\
				task = test->mock_init_task;	\
		}						\
		task;						\
	})
#endif

#define for_each_process(p) \
        for (p = init_task_ptr ; (p = next_task(p)) != init_task_ptr ; )

And then tests can hang a mock init_task off the test? It seems really
horrible, but there is a LOT of global state in the kernel, so I figured
I had to start somewhere? :P

Thoughts?

-Kees

[1] https://lore.kernel.org/lkml/20221122161240.137570-1-pskocik@gmail.com/
[2] https://lore.kernel.org/lkml/20220910212804.670622-1-davidgow@google.com/
[3] https://lore.kernel.org/lkml/20221125084306.1063074-1-davidgow@google.com/

-- 
Kees Cook

Re: mocking init_task ?

[Daniel Latypov]

On Thu, Dec 1, 2022 at 8:28 PM Kees Cook <keescook@chromium.org> wrote:
>
> Hi,
>
> I want to make a unit test for kill_something_info(), as there is a patch
> to fix a bug with it not working as expected under a specific process
> tree arrangement[1]. This seems like a great candidate for a unit test:
> given a specific state, return a specific result. Emboldened, I applied
> the "kunit: Support redirecting function calls" series[2], preparing to
> mock group_send_sig_info(), and ran head-long into for_each_process()
> ... which uses the address of the global init_task:
>
> #define for_each_process(p) \
>         for (p = &init_task ; (p = next_task(p)) != &init_task ; )
>
> :(
>
> I'm curious what you think might be the right approach to mock
> init_task, or for_each_process(), so I can apply unit tests to some of
> the "simple" process tree walkers...
>
> One idea I had was using the "kunit: Provide a static key to check if
> KUnit is actively running tests" series[3], and do something like this:
>
> #ifndef CONFIG_KUNIT
> #define init_task_ptr   &init_task
> #else
> #define init_task_ptr   ({                                      \
>                 struct task_struct *task = &init_task;          \
>                 if (static_branch_unlikely(&kunit_running)) {   \

nit: if(kunit_get_current_test()) is probably a bit safer.
For UML, it will never make a difference (IIUC), but we probably want
to ensure that current == the task_struct executing kunit tests.

>                         struct kunit *test;                     \
>                         test = current->kunit_test;             \
>                         if (test->mock_init_task)               \
>                                 task = test->mock_init_task;    \
>                 }                                               \
>                 task;                                           \
>         })
> #endif
>
> #define for_each_process(p) \
>         for (p = init_task_ptr ; (p = next_task(p)) != init_task_ptr ; )

I think this is pretty close to being the best you can do.

I have a terrible idea that afaict should work.
If it does work, I like it a lot better since it's less intrusive.

== my_test.c ==

#include <linux/sched/signal.h>
#undef for_each_process
#define for_each_process(p) \
        for (p = test_init_task ; (p = next_task(p)) != test_init_task ; )

struct task_struct _test_init_task = {
        .tasks          = LIST_HEAD_INIT(_test_init_task.tasks),
};

#define test_init_task ({ \
                        struct task_struct *task = &init_task;\
                        if (kunit_get_current_test()) {\
                                task = &_test_init_task;\
                        }\
                        task;\
})

#include <kunit/test.h>

static void my_test(struct kunit *test)
{
        struct task_struct *p;

        for_each_process(p)
                KUNIT_FAIL(test, "should not be called");

       /* can modify test_init_task as you see fit here */
}

And then you explicitly keep nothing else in this translation unit so
we limit the damage we've done.

Daniel

>
> And then tests can hang a mock init_task off the test? It seems really
> horrible, but there is a LOT of global state in the kernel, so I figured
> I had to start somewhere? :P
>
> Thoughts?
>
> -Kees
>
> [1] https://lore.kernel.org/lkml/20221122161240.137570-1-pskocik@gmail.com/
> [2] https://lore.kernel.org/lkml/20220910212804.670622-1-davidgow@google.com/
> [3] https://lore.kernel.org/lkml/20221125084306.1063074-1-davidgow@google.com/
>
> --
> Kees Cook

Re: mocking init_task ?

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 5266 bytes --]

On Fri, Dec 2, 2022 at 12:28 PM Kees Cook <keescook@chromium.org> wrote:
>
> Hi,
>
> I want to make a unit test for kill_something_info(), as there is a patch
> to fix a bug with it not working as expected under a specific process
> tree arrangement[1]. This seems like a great candidate for a unit test:
> given a specific state, return a specific result. Emboldened, I applied
> the "kunit: Support redirecting function calls" series[2], preparing to
> mock group_send_sig_info(), and ran head-long into for_each_process()
> ... which uses the address of the global init_task:
>
> #define for_each_process(p) \
>         for (p = &init_task ; (p = next_task(p)) != &init_task ; )
>
> :(
>
> I'm curious what you think might be the right approach to mock
> init_task, or for_each_process(), so I can apply unit tests to some of
> the "simple" process tree walkers...

Wow -- thanks for this.

And yeah, this is really close to being really nice to test, apart
from the global state rearing its ugly head again.

Also, FYI, a version of the static stub patch which uses the static
key should be out this year (just pending cleanups to documentation /
examples / etc). You can find it here for now:
https://kunit-review.git.corp.google.com/c/linux/+/5589

>
> One idea I had was using the "kunit: Provide a static key to check if
> KUnit is actively running tests" series[3], and do something like this:
>
> #ifndef CONFIG_KUNIT
> #define init_task_ptr   &init_task
> #else
> #define init_task_ptr   ({                                      \
>                 struct task_struct *task = &init_task;          \
>                 if (static_branch_unlikely(&kunit_running)) {   \
>                         struct kunit *test;                     \
>                         test = current->kunit_test;             \
>                         if (test->mock_init_task)               \
>                                 task = test->mock_init_task;    \
>                 }                                               \
>                 task;                                           \
>         })
> #endif
>
> #define for_each_process(p) \
>         for (p = init_task_ptr ; (p = next_task(p)) != init_task_ptr ; )
>
> And then tests can hang a mock init_task off the test? It seems really
> horrible, but there is a LOT of global state in the kernel, so I figured
> I had to start somewhere? :P

I confess, I can't think of a better way to do this overall, but have
a few suggestions for the details.

My biggest worry would be if there's anything else using
for_each_process() or similar from the KUnit task context which could
cause bigger problems, though I _suspect_ we're okay. If Daniel's
suggestion for overriding things just for the test file works, that's
probably safer (though it doesn't work as well as a "generic" way of
replacing the task lists for other tests).

Related;y, in terms of overall structure, I'm not sure whether it
makes more sense to replace init_task with init_task_ptr everywhere,
or to simply have a separate definition of the macros for the KUnit
case. If this is only going to be contained to for_each_process(),
maybe just providing a separate definition of that would be easiest.
Though if we're also handling tasklist_empty(), do_each_thread(), etc.
the init_task_ptr route seems better.

If it gets more unwieldy, maybe having an always inline
get_init_task() function would be cleaner, too.

Also, the static key patch is already sitting in the kselftest/kunit
branch, and provides (as Daniel notes) the kunit_get_current_task()
function, which is cleaner to use.

Equally, instead of adding a 'mock_init_task' member to struct kunit,
it may make more sense to use a KUnit named resource. These are a
little bit slower and require a little bit more handling re: reference
counts, but mean we don't have to add a new member to struct kunit for
each new piece of mocked global state (and don't consume memory for
tests which don't need them). Documentation is here (though we do plan
to revise and simplify the API at some point in the next year):
https://docs.kernel.org/dev-tools/kunit/api/resource.html#c.kunit_find_named_resource

If this turns into a useful pattern, we could combine these to have a
kunit helper function like:
static inline void *kunit_get_mock_variable_ptr(const char
*resource_name, void *default_ptr)

This could do all of the above in one place: get the current test.
lookup the resource (handling the refcounting as needed), and
returning default_ptr if either isn't running. And it could be defined
just as default_ptr if CONFIG_KUNIT=n.

Then, init_task_ptr could just be:
#define init_task_ptr (struct task_struct
*)kunit_get_mock_variable_ptr("mock_init_task", &init_task)

(And for simpler cases which don't need to handle for loops, it'd be
possible to forgo the extra #define altogether.

Equally, we could try doing something halfway between that and the
static stub implementation, where we key off the address of the mocked
data, rather than a string.

Of course, I suspect it makes sense to try out a simpler, init_task
specific option like you suggest before going too far to design and
implement something more generic, in case there's an obvious flaw
we're missing.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]