Date:   Mon, 26 Sep 2022 12:19:46 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     David Gow <davidgow@...gle.com>
Cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        Daniel Latypov <dlatypov@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Joe Fradley <joefradley@...gle.com>,
        Steve Muckle <smuckle@...gle.com>, kunit-dev@...glegroups.com,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2 2/2] kunit: expose ftrace-based API for stubbing
 out functions during tests

On Sun, 11 Sep 2022 05:28:04 +0800
David Gow <davidgow@...gle.com> wrote:

> +++ b/lib/kunit/ftrace_stub.c
> @@ -0,0 +1,137 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <kunit/test.h>
> +
> +#include <linux/typecheck.h>
> +
> +#include <linux/ftrace.h>
> +#include <linux/sched.h>
> +
> +struct kunit_ftrace_stub_ctx {
> +	struct kunit *test;
> +	unsigned long real_fn_addr; /* used as a key to lookup the stub */
> +	unsigned long replacement_addr;
> +	struct ftrace_ops ops; /* a copy of kunit_stub_base_ops with .private set */
> +};
> +
> +static void kunit_stub_trampoline(unsigned long ip, unsigned long parent_ip,
> +				  struct ftrace_ops *ops,
> +				  struct ftrace_regs *fregs)
> +{
> +	struct kunit_ftrace_stub_ctx *ctx = ops->private;
> +	int lock_bit;
> +
> +	if (current->kunit_test != ctx->test)
> +		return;
> +
> +	lock_bit = ftrace_test_recursion_trylock(ip, parent_ip);
> +	KUNIT_ASSERT_GE(ctx->test, lock_bit, 0);
> +
> +	ftrace_instruction_pointer_set(fregs, ctx->replacement_addr);
> +
> +	ftrace_test_recursion_unlock(lock_bit);
> +}
> +
> +static struct ftrace_ops kunit_stub_base_ops = {
> +	.func = &kunit_stub_trampoline,
> +	.flags = FTRACE_OPS_FL_IPMODIFY |
> +#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
> +		FTRACE_OPS_FL_SAVE_REGS |
> +#endif
> +		FTRACE_OPS_FL_DYNAMIC
> +};
> +
> +static void __kunit_ftrace_stub_resource_free(struct kunit_resource *res)
> +{
> +	struct kunit_ftrace_stub_ctx *ctx = res->data;
> +
> +	unregister_ftrace_function(&ctx->ops);
> +	kfree(ctx);
> +}
> +
> +/* Matching function for kunit_find_resource(). match_data is real_fn_addr. */
> +static bool __kunit_ftrace_stub_resource_match(struct kunit *test,
> +						struct kunit_resource *res,
> +						void *match_real_fn_addr)
> +{
> +	/* This pointer is only valid if res is a static stub resource. */
> +	struct kunit_ftrace_stub_ctx *ctx = res->data;
> +
> +	/* Make sure the resource is a static stub resource. */
> +	if (res->free != &__kunit_ftrace_stub_resource_free)
> +		return false;
> +
> +	return ctx->real_fn_addr == (unsigned long)match_real_fn_addr;
> +}
> +
> +void kunit_deactivate_ftrace_stub(struct kunit *test, void *real_fn_addr)
> +{
> +	struct kunit_resource *res;
> +
> +	KUNIT_ASSERT_PTR_NE_MSG(test, real_fn_addr, NULL,
> +				"Tried to deactivate a NULL stub.");
> +
> +	/* Look up the existing stub for this function. */
> +	res = kunit_find_resource(test,
> +				  __kunit_ftrace_stub_resource_match,
> +				  real_fn_addr);
> +
> +	/* Error out if the stub doesn't exist. */
> +	KUNIT_ASSERT_PTR_NE_MSG(test, res, NULL,
> +				"Tried to deactivate a nonexistent stub.");
> +
> +	/* Free the stub. We 'put' twice, as we got a reference
> +	 * from kunit_find_resource(). The free function will deactivate the
> +	 * ftrace stub.
> +	 */
> +	kunit_remove_resource(test, res);
> +	kunit_put_resource(res);
> +}
> +EXPORT_SYMBOL_GPL(kunit_deactivate_ftrace_stub);
> +
> +void __kunit_activate_ftrace_stub(struct kunit *test,
> +				  const char *name,
> +				  void *real_fn_addr,
> +				  void *replacement_addr)
> +{
> +	unsigned long ftrace_ip;
> +	struct kunit_ftrace_stub_ctx *ctx;
> +	int ret;
> +
> +	ftrace_ip = ftrace_location((unsigned long)real_fn_addr);
> +	if (!ftrace_ip)
> +		KUNIT_ASSERT_FAILURE(test, "%s ip is invalid: not a function, or is marked notrace or inline", name);
> +
> +	/* Allocate the stub context, which contains pointers to the replacement
> +	 * function and the test object. It's also registered as a KUnit
> +	 * resource which can be looked up by address (to deactivate manually)
> +	 * and is destroyed automatically on test exit.
> +	 */
> +	ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL_MSG(test, ctx, "failed to allocate kunit stub for %s", name);
> +
> +	ctx->test = test;
> +	ctx->ops = kunit_stub_base_ops;
> +	ctx->ops.private = ctx;
> +	ctx->real_fn_addr = (unsigned long)real_fn_addr;
> +	ctx->replacement_addr = (unsigned long)replacement_addr;
> +
> +	ret = ftrace_set_filter_ip(&ctx->ops, ftrace_ip, 0, 0);
> +	if (ret) {
> +		kfree(ctx);
> +		KUNIT_ASSERT_FAILURE(test, "failed to set filter ip for %s: %d", name, ret);

I don't know the KUNIT_ASSERT content, but I'm guessing that any failure is
just to crash the kernel or something where you do not need to bail out of
the function?

> +	}
> +
> +	ret = register_ftrace_function(&ctx->ops);
> +	if (ret) {
> +		kfree(ctx);
> +		if (ret == -EBUSY)
> +			KUNIT_ASSERT_FAILURE(test, "failed to register stub (-EBUSY) for %s, likely due to already stubbing it?", name);
> +		KUNIT_ASSERT_FAILURE(test, "failed to register stub for %s: %d", name, ret);
> +	}
> +
> +	/* Register the stub as a resource with a cleanup function */
> +	kunit_alloc_resource(test, NULL,
> +			     __kunit_ftrace_stub_resource_free,
> +			     GFP_KERNEL, ctx);

And I'm also guessing that there's no race concern with registering the
free resource after enabling the function code?

Other than that, looks good to me.

Reviewed-by: Steven Rostedt (Google) <rostedt@...dmis.org>

-- Steve

> +}

Powered by blists - more mailing lists