Message-ID: <CABVgOSmnaa1-G_QPYjYnA+wYwS9DhW6ugO3sbTR+oOiHc6YeKQ@mail.gmail.com>
Date:   Sat, 3 Dec 2022 18:29:23 +0800
From:   David Gow <davidgow@...gle.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        Daniel Latypov <dlatypov@...gle.com>,
        kunit-dev@...glegroups.com, Petr Skocik <pskocik@...il.com>,
        linux-hardening@...r.kernel.org
Subject: Re: mocking init_task ?

On Fri, Dec 2, 2022 at 12:28 PM Kees Cook <keescook@...omium.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

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4003 bytes)

Powered by blists - more mailing lists