Re: [RFC PATCH bpf-next 1/3] bpf: add dummy BPF STRUCT_OPS for test purpose

Hi,

On 9/16/2021 4:58 AM, Martin KaFai Lau wrote:

On Wed, Sep 15, 2021 at 11:37:51AM +0800, Hou Tao wrote:

quoted

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

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

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);

[ ... ]

quoted

+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.

Will do and thanks for your suggestions.

quoted

+
+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",
+};

.