On Wed, Sep 15, 2021 at 11:37:51AM +0800, Hou Tao wrote:
> Currently the test of BPF STRUCT_OPS depends on the specific bpf
> implementation of tcp_congestion_ops, and it can not cover all
> basic functionalities (e.g, return value handling), so introduce
> a dummy BPF STRUCT_OPS for test purpose.
>
> Dummy BPF STRUCT_OPS may not being needed for release kernel, so
> adding a kconfig option BPF_DUMMY_STRUCT_OPS to enable it separatedly.
Thanks for the patches !
> diff --git a/include/linux/bpf_dummy_ops.h b/include/linux/bpf_dummy_ops.h
> new file mode 100644
> index 000000000000..b2aad3e6e2fe
> --- /dev/null
> +++ b/include/linux/bpf_dummy_ops.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2021. Huawei Technologies Co., Ltd
> + */
> +#ifndef _BPF_DUMMY_OPS_H
> +#define _BPF_DUMMY_OPS_H
> +
> +#ifdef CONFIG_BPF_DUMMY_STRUCT_OPS
> +#include <linux/module.h>
> +
> +struct bpf_dummy_ops_state {
> + int val;
> +};
> +
> +struct bpf_dummy_ops {
> + int (*init)(struct bpf_dummy_ops_state *state);
> + struct module *owner;
> +};
> +
> +extern struct bpf_dummy_ops *bpf_get_dummy_ops(void);
> +extern void bpf_put_dummy_ops(struct bpf_dummy_ops *ops);
> +#else
> +struct bpf_dummy_ops {}；
This ';' looks different ;)
It probably has dodged the compiler due to the kconfig.
I think CONFIG_BPF_DUMMY_STRUCT_OPS and the bpf_(get|put)_dummy_ops
are not needed. More on this later.
> diff --git a/kernel/bpf/bpf_dummy_struct_ops.c b/kernel/bpf/bpf_dummy_struct_ops.c
> new file mode 100644
> index 000000000000..f76c4a3733f0
> --- /dev/null
> +++ b/kernel/bpf/bpf_dummy_struct_ops.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021. Huawei Technologies Co., Ltd
> + */
> +#include <linux/kernel.h>
> +#include <linux/spinlock.h>
> +#include <linux/bpf_verifier.h>
> +#include <linux/bpf.h>
> +#include <linux/btf.h>
> +#include <linux/bpf_dummy_ops.h>
> +
> +static struct bpf_dummy_ops *bpf_dummy_ops_singletion;
> +static DEFINE_SPINLOCK(bpf_dummy_ops_lock);
> +
> +static const struct btf_type *dummy_ops_state;
> +
> +struct bpf_dummy_ops *bpf_get_dummy_ops(void)
> +{
> + struct bpf_dummy_ops *ops;
> +
> + spin_lock(&bpf_dummy_ops_lock);
> + ops = bpf_dummy_ops_singletion;
> + if (ops && !bpf_try_module_get(ops, ops->owner))
> + ops = NULL;
> + spin_unlock(&bpf_dummy_ops_lock);
> +
> + return ops ? ops : ERR_PTR(-ENXIO);
> +}
> +EXPORT_SYMBOL_GPL(bpf_get_dummy_ops);
> +
> +void bpf_put_dummy_ops(struct bpf_dummy_ops *ops)
> +{
> + bpf_module_put(ops, ops->owner);
> +}
> +EXPORT_SYMBOL_GPL(bpf_put_dummy_ops);
[ ... ]
> +static int bpf_dummy_reg(void *kdata)
> +{
> + struct bpf_dummy_ops *ops = kdata;
> + int err = 0;
> +
> + spin_lock(&bpf_dummy_ops_lock);
> + if (!bpf_dummy_ops_singletion)
> + bpf_dummy_ops_singletion = ops;
> + else
> + err = -EEXIST;
> + spin_unlock(&bpf_dummy_ops_lock);
> +
> + return err;
> +}
I don't think we are interested in testing register/unregister
a struct_ops. This common infra logic should have already
been covered by bpf_tcp_ca. Lets see if it can be avoided
such that the above singleton instance and EXPORT_SYMBOL_GPL
can also be removed.
It can reuse the bpf_prog_test_run() which can run a particular
bpf prog. Then it allows a flexible way to select which prog
to call instead of creating a file and then triggering individual
prog by writing a name string into this new file.
For bpf_prog_test_run(), it needs a ".test_run" implementation in
"const struct bpf_prog_ops bpf_struct_ops_prog_ops".
This to-be-implemented ".test_run" can check the prog->aux->attach_btf_id
to ensure it is the bpf_dummy_ops. The prog->expected_attach_type can
tell which "func" ptr within the bpf_dummy_ops and then ".test_run" will
know how to call it. The extra thing for the struct_ops's ".test_run" is
to first call arch_prepare_bpf_trampoline() to prepare the trampoline
before calling into the bpf prog.
You can take a look at the other ".test_run" implementations,
e.g. bpf_prog_test_run_skb() and bpf_prog_test_run_tracing().
test_skb_pkt_end.c and fentry_test.c (likely others also) can be
used as reference for prog_tests/ purpose. For the dummy_ops test in
prog_tests/, it does not need to call bpf_map__attach_struct_ops() since
there is no need to reg(). Instead, directly bpf_prog_test_run() to
exercise each prog in bpf_dummy_ops.skel.h.
bpf_dummy_init_member() should return -ENOTSUPP.
bpf_dummy_reg() and bpf_dummy_unreg() should then be never called.
bpf_dummy_struct_ops.c should be moved into net/bpf/.
No need to have CONFIG_BPF_DUMMY_STRUCT_OPS. In the future, a generic one
could be created for the test_run related codes, if there is a need.
> +
> +static void bpf_dummy_unreg(void *kdata)
> +{
> + struct bpf_dummy_ops *ops = kdata;
> +
> + spin_lock(&bpf_dummy_ops_lock);
> + if (bpf_dummy_ops_singletion == ops)
> + bpf_dummy_ops_singletion = NULL;
> + else
> + WARN_ON(1);
> + spin_unlock(&bpf_dummy_ops_lock);
> +}
> +
> +extern struct bpf_struct_ops bpf_bpf_dummy_ops;
> +
> +struct bpf_struct_ops bpf_bpf_dummy_ops = {
> + .verifier_ops = &bpf_dummy_verifier_ops,
> + .init = bpf_dummy_init,
> + .init_member = bpf_dummy_init_member,
> + .check_member = bpf_dummy_check_member,
> + .reg = bpf_dummy_reg,
> + .unreg = bpf_dummy_unreg,
> + .name = "bpf_dummy_ops",
> +};
