Re: [RFC PATCH bpf-next 1/3] bpf: add dummy BPF STRUCT_OPS for test purpose
From: Martin KaFai Lau <hidden>
Date: 2021-09-15 20:59:07
Also in:
bpf
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 !
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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",
+};