All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH bpf-next 0/3] introduce dummy BPF STRUCT_OPS
@ 2021-09-15  3:37 Hou Tao
  2021-09-15  3:37 ` [RFC PATCH bpf-next 1/3] bpf: add dummy BPF STRUCT_OPS for test purpose Hou Tao
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Hou Tao @ 2021-09-15  3:37 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau
  Cc: netdev, bpf, houtao1

Hi,

Currently the test of BPF STRUCT_OPS depends on the specific bpf
implementation of tcp_congestion_ops, but it can not cover all
basic functionalities (e.g, return value handling), so introduce
a dummy BPF STRUCT_OPS for test purpose.

The test procedure works in the following way:
(1) write to newly-created /sys/kernel/bpf_test/dummy_ops_ctl
The format is "test_case_N [option_integer_return]".

(2) test_case_N in bpf_testmod.ko will be call dummy_ops method
It will call bpf_get_dummy_ops() first to get the dummy_ops,
call its method, and check the return value of the method. If
the method succeeds and its return value is expected, the write
succeeds, else it fails.

But there is one concerns: the format of dummy_ops_ctl is too
simply. It can only check a integer state update from dummy_ops
method. If multiple states are updated in a dummy struct_ops
method, it can not verify these updates are expected in
bpf_testmod.ko. Are such test is needed here ?

Any comments are welcome.

Hou Tao (3):
  bpf: add dummy BPF STRUCT_OPS for test purpose
  selftests/bpf: call dummy struct_ops in bpf_testmode
  selftests/bpf: add test for BPF STRUCT_OPS

 include/linux/bpf_dummy_ops.h                 |  28 +++
 kernel/bpf/Kconfig                            |   7 +
 kernel/bpf/Makefile                           |   2 +
 kernel/bpf/bpf_dummy_struct_ops.c             | 173 ++++++++++++++++++
 kernel/bpf/bpf_struct_ops_types.h             |   4 +
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 152 ++++++++++++++-
 .../selftests/bpf/prog_tests/bpf_dummy_ops.c  |  95 ++++++++++
 .../selftests/bpf/progs/bpf_dummy_ops.c       |  34 ++++
 8 files changed, 493 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/bpf_dummy_ops.h
 create mode 100644 kernel/bpf/bpf_dummy_struct_ops.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/bpf_dummy_ops.c
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_dummy_ops.c

-- 
2.29.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [RFC PATCH bpf-next 1/3] bpf: add dummy BPF STRUCT_OPS for test purpose
  2021-09-15  3:37 [RFC PATCH bpf-next 0/3] introduce dummy BPF STRUCT_OPS Hou Tao
@ 2021-09-15  3:37 ` Hou Tao
  2021-09-15 20:58   ` Martin KaFai Lau
                     ` (2 more replies)
  2021-09-15  3:37 ` [RFC PATCH bpf-next 2/3] selftests/bpf: call dummy struct_ops in bpf_testmode Hou Tao
  2021-09-15  3:37 ` [RFC PATCH bpf-next 3/3] selftests/bpf: add test for BPF STRUCT_OPS Hou Tao
  2 siblings, 3 replies; 9+ messages in thread
From: Hou Tao @ 2021-09-15  3:37 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau
  Cc: netdev, bpf, houtao1

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.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 include/linux/bpf_dummy_ops.h     |  28 +++++
 kernel/bpf/Kconfig                |   7 ++
 kernel/bpf/Makefile               |   2 +
 kernel/bpf/bpf_dummy_struct_ops.c | 173 ++++++++++++++++++++++++++++++
 kernel/bpf/bpf_struct_ops_types.h |   4 +
 5 files changed, 214 insertions(+)
 create mode 100644 include/linux/bpf_dummy_ops.h
 create mode 100644 kernel/bpf/bpf_dummy_struct_ops.c

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 {};
+static inline struct bpf_dummy_ops *bpf_get_dummy_ops(void) { return NULL; }
+static inline void bpf_put_dummy_ops(struct bpf_dummy_ops *ops) {}
+#endif
+
+#endif
diff --git a/kernel/bpf/Kconfig b/kernel/bpf/Kconfig
index a82d6de86522..4a11eca42791 100644
--- a/kernel/bpf/Kconfig
+++ b/kernel/bpf/Kconfig
@@ -86,4 +86,11 @@ config BPF_LSM
 
 	  If you are unsure how to answer this question, answer N.
 
+config BPF_DUMMY_STRUCT_OPS
+	bool "Enable dummy struct ops"
+	depends on BPF_SYSCALL && BPF_JIT
+	help
+	  Enables dummy struct ops to test the basic functionalities of
+	  BPF STRUCT_OPS.
+
 endmenu # "BPF subsystem"
diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
index 7f33098ca63f..17e2bb59cceb 100644
--- a/kernel/bpf/Makefile
+++ b/kernel/bpf/Makefile
@@ -33,6 +33,8 @@ obj-$(CONFIG_DEBUG_INFO_BTF) += sysfs_btf.o
 endif
 ifeq ($(CONFIG_BPF_JIT),y)
 obj-$(CONFIG_BPF_SYSCALL) += bpf_struct_ops.o
+obj-$(CONFIG_BPF_SYSCALL) += bpf_dummy_struct_ops.o
 obj-${CONFIG_BPF_LSM} += bpf_lsm.o
 endif
+obj-$(CONFIG_BPF_DUMMY_STRUCT_OPS) += bpf_dummy_struct_ops.o
 obj-$(CONFIG_BPF_PRELOAD) += preload/
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_init(struct btf *btf)
+{
+	s32 type_id;
+
+	type_id = btf_find_by_name_kind(btf, "bpf_dummy_ops_state",
+					BTF_KIND_STRUCT);
+	if (type_id < 0)
+		return -EINVAL;
+
+	dummy_ops_state = btf_type_by_id(btf, type_id);
+
+	return 0;
+}
+
+static const struct bpf_func_proto *
+bpf_dummy_ops_get_func_proto(enum bpf_func_id func_id,
+			     const struct bpf_prog *prog)
+{
+	switch (func_id) {
+	case BPF_FUNC_map_lookup_elem:
+		return &bpf_map_lookup_elem_proto;
+	default:
+		return NULL;
+	}
+}
+
+static bool bpf_dummy_ops_is_valid_access(int off, int size,
+					  enum bpf_access_type type,
+					  const struct bpf_prog *prog,
+					  struct bpf_insn_access_aux *info)
+{
+	/* a common helper ? */
+	if (off < 0 || off >= sizeof(__u64) * MAX_BPF_FUNC_ARGS)
+		return false;
+	if (type != BPF_READ)
+		return false;
+	if (off % size != 0)
+		return false;
+
+	return btf_ctx_access(off, size, type, prog, info);
+}
+
+static int bpf_dummy_ops_btf_struct_access(struct bpf_verifier_log *log,
+					   const struct btf *btf,
+					   const struct btf_type *t, int off,
+					   int size, enum bpf_access_type atype,
+					   u32 *next_btf_id)
+{
+	size_t end;
+
+	if (atype == BPF_READ)
+		return btf_struct_access(log, btf, t, off, size, atype,
+					 next_btf_id);
+
+	if (t != dummy_ops_state) {
+		bpf_log(log, "only read is supported\n");
+		return -EACCES;
+	}
+
+	switch (off) {
+	case offsetof(struct bpf_dummy_ops_state, val):
+		end = offsetofend(struct bpf_dummy_ops_state, val);
+		break;
+	default:
+		bpf_log(log, "no write support to bpf_dummy_ops_state at off %d\n",
+			off);
+		return -EACCES;
+	}
+
+	if (off + size > end) {
+		bpf_log(log,
+			"write access at off %d with size %d beyond the member of bpf_dummy_ops_state ended at %zu\n",
+			off, size, end);
+		return -EACCES;
+	}
+
+	return NOT_INIT;
+}
+
+static const struct bpf_verifier_ops bpf_dummy_verifier_ops = {
+	.get_func_proto = bpf_dummy_ops_get_func_proto,
+	.is_valid_access = bpf_dummy_ops_is_valid_access,
+	.btf_struct_access = bpf_dummy_ops_btf_struct_access,
+};
+
+static int bpf_dummy_check_member(const struct btf_type *t,
+				  const struct btf_member *member)
+{
+	return 0;
+}
+
+
+static int bpf_dummy_init_member(const struct btf_type *t,
+				 const struct btf_member *member,
+				 void *kdata, const void *udata)
+{
+	return 0;
+}
+
+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;
+}
+
+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",
+};
diff --git a/kernel/bpf/bpf_struct_ops_types.h b/kernel/bpf/bpf_struct_ops_types.h
index 7ec458ead497..6d24c75f4d70 100644
--- a/kernel/bpf/bpf_struct_ops_types.h
+++ b/kernel/bpf/bpf_struct_ops_types.h
@@ -2,6 +2,10 @@
 /* internal file - do not include directly */
 
 #ifdef CONFIG_BPF_JIT
+#ifdef CONFIG_BPF_DUMMY_STRUCT_OPS
+#include <linux/bpf_dummy_ops.h>
+BPF_STRUCT_OPS_TYPE(bpf_dummy_ops)
+#endif
 #ifdef CONFIG_INET
 #include <net/tcp.h>
 BPF_STRUCT_OPS_TYPE(tcp_congestion_ops)
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [RFC PATCH bpf-next 2/3] selftests/bpf: call dummy struct_ops in bpf_testmode
  2021-09-15  3:37 [RFC PATCH bpf-next 0/3] introduce dummy BPF STRUCT_OPS Hou Tao
  2021-09-15  3:37 ` [RFC PATCH bpf-next 1/3] bpf: add dummy BPF STRUCT_OPS for test purpose Hou Tao
@ 2021-09-15  3:37 ` Hou Tao
  2021-09-15  3:37 ` [RFC PATCH bpf-next 3/3] selftests/bpf: add test for BPF STRUCT_OPS Hou Tao
  2 siblings, 0 replies; 9+ messages in thread
From: Hou Tao @ 2021-09-15  3:37 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau
  Cc: netdev, bpf, houtao1

The dummy BPF STRUCT_OPS in kernel, in order to test it, we need
provide a way to call its method and return state to userspace.

So a new sysfs file /sys/kernel/bpf_test/dummy_ops_ctl is created.
Its expected input is: "test_case_name [optinal_integer_state]",
When the content is written to the file, the specific method of
dummy struct_ops will be called and the returned state will be checked.

Now only two test cases are added: one to check the return value of
init() method, another to check the value returned by pointer
assignment.

It may be better to split the dummy struct_ops related code into
a separated file.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 152 +++++++++++++++++-
 1 file changed, 150 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 141d8da687d2..1286758b999c 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -6,11 +6,22 @@
 #include <linux/percpu-defs.h>
 #include <linux/sysfs.h>
 #include <linux/tracepoint.h>
+#include <linux/string.h>
+#include <linux/bpf_dummy_ops.h>
 #include "bpf_testmod.h"
 
 #define CREATE_TRACE_POINTS
 #include "bpf_testmod-events.h"
 
+typedef int (*dummy_ops_test_fn)(struct bpf_dummy_ops *ops,
+				 const char *buf, size_t cnt);
+struct dummy_ops_test {
+	const char *name;
+	dummy_ops_test_fn fn;
+};
+
+static struct kobject *bpf_test_kobj;
+
 DEFINE_PER_CPU(int, bpf_testmod_ksym_percpu) = 123;
 
 noinline ssize_t
@@ -55,14 +66,151 @@ static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = {
 	.write = bpf_testmod_test_write,
 };
 
+static int dummy_ops_chk_ret(struct bpf_dummy_ops *ops,
+			     const char *buf, size_t cnt)
+{
+	int exp;
+	int err;
+
+	if (cnt <= 1)
+		return -EINVAL;
+
+	if (kstrtoint(buf + 1, 0, &exp))
+		return -EINVAL;
+
+	err = ops->init(NULL);
+	if (err != exp)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int dummy_ops_chk_ret_by_ptr(struct bpf_dummy_ops *ops,
+				    const char *buf, size_t cnt)
+{
+	int exp;
+	int err;
+	struct bpf_dummy_ops_state state;
+
+	if (cnt <= 1)
+		return -EINVAL;
+
+	if (kstrtoint(buf + 1, 0, &exp))
+		return -EINVAL;
+
+	memset(&state, 0, sizeof(state));
+	err = ops->init(&state);
+	if (err || state.val != exp)
+		return -EINVAL;
+
+	return 0;
+}
+
+static const struct dummy_ops_test tests[] = {
+	{.name = "init_1", .fn = dummy_ops_chk_ret},
+	{.name = "init_2", .fn = dummy_ops_chk_ret_by_ptr},
+};
+
+static const struct dummy_ops_test *dummy_ops_find_test(const char *buf,
+							size_t cnt)
+{
+	char *c;
+	size_t nm_len;
+	unsigned int i;
+
+	/*
+	 * There may be test-specific string (e.g, return value)
+	 * after the name of test. The delimiter is one space.
+	 */
+	c = strchr(buf, ' ');
+	if (c)
+		nm_len = c - buf;
+	else
+		nm_len = cnt;
+	for (i = 0; i < ARRAY_SIZE(tests); i++) {
+		if (nm_len >= strlen(tests[i].name) &&
+		    !strncmp(buf, tests[i].name, nm_len))
+			return &tests[i];
+	}
+
+	return NULL;
+}
+
+static ssize_t dummy_ops_ctl_store(struct kobject *kobj,
+				   struct kobj_attribute *attr,
+				   const char *buf, size_t cnt)
+{
+	struct bpf_dummy_ops *ops = bpf_get_dummy_ops();
+	const struct dummy_ops_test *test;
+	size_t nm_len;
+	int err;
+
+	/* dummy struct_ops is disabled, so always return success */
+	if (!ops)
+		return cnt;
+	if (IS_ERR(ops))
+		return PTR_ERR(ops);
+
+	test = dummy_ops_find_test(buf, cnt);
+	if (!test) {
+		err = -EINVAL;
+		goto out;
+	}
+
+	nm_len = strlen(test->name);
+	err = test->fn(ops, buf + nm_len, cnt - nm_len);
+	if (!err)
+		err = cnt;
+out:
+	bpf_put_dummy_ops(ops);
+	return err;
+}
+
+static struct kobj_attribute dummy_ops_ctl = __ATTR_WO(dummy_ops_ctl);
+
+static struct attribute *bpf_test_attrs[] = {
+	&dummy_ops_ctl.attr,
+	NULL,
+};
+
+static const struct attribute_group bpf_test_attr_group = {
+	.attrs = bpf_test_attrs,
+};
+
 static int bpf_testmod_init(void)
 {
-	return sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
+	int err;
+
+	bpf_test_kobj = kobject_create_and_add("bpf_test", kernel_kobj);
+	if (!bpf_test_kobj) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	err = sysfs_create_group(bpf_test_kobj, &bpf_test_attr_group);
+	if (err)
+		goto put_out;
+
+	err = sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
+	if (err)
+		goto rm_grp_out;
+
+	return 0;
+
+rm_grp_out:
+	sysfs_remove_group(bpf_test_kobj, &bpf_test_attr_group);
+put_out:
+	kobject_put(bpf_test_kobj);
+	bpf_test_kobj = NULL;
+out:
+	return err;
 }
 
 static void bpf_testmod_exit(void)
 {
-	return sysfs_remove_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
+	sysfs_remove_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
+	sysfs_remove_group(bpf_test_kobj, &bpf_test_attr_group);
+	kobject_put(bpf_test_kobj);
 }
 
 module_init(bpf_testmod_init);
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [RFC PATCH bpf-next 3/3] selftests/bpf: add test for BPF STRUCT_OPS
  2021-09-15  3:37 [RFC PATCH bpf-next 0/3] introduce dummy BPF STRUCT_OPS Hou Tao
  2021-09-15  3:37 ` [RFC PATCH bpf-next 1/3] bpf: add dummy BPF STRUCT_OPS for test purpose Hou Tao
  2021-09-15  3:37 ` [RFC PATCH bpf-next 2/3] selftests/bpf: call dummy struct_ops in bpf_testmode Hou Tao
@ 2021-09-15  3:37 ` Hou Tao
  2 siblings, 0 replies; 9+ messages in thread
From: Hou Tao @ 2021-09-15  3:37 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau
  Cc: netdev, bpf, houtao1

Add two test cases for BPF STRUCT_OPS: one to check the return
value of BPF_PROG_TYPE_STRUCT_OPS is returned, another to check
the returned value through output parameter is expected.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 .../selftests/bpf/prog_tests/bpf_dummy_ops.c  | 95 +++++++++++++++++++
 .../selftests/bpf/progs/bpf_dummy_ops.c       | 34 +++++++
 2 files changed, 129 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/bpf_dummy_ops.c
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_dummy_ops.c

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_dummy_ops.c b/tools/testing/selftests/bpf/prog_tests/bpf_dummy_ops.c
new file mode 100644
index 000000000000..d9a45579c716
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_dummy_ops.c
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2021. Huawei Technologies Co., Ltd */
+#include <linux/err.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <assert.h>
+#include <string.h>
+#include <errno.h>
+#include <test_progs.h>
+
+#include "bpf_dummy_ops.skel.h"
+
+#define OPS_CTL_CMD_SIZE 64
+
+static void do_ctl(const char *cmd)
+{
+	int duration = 0;
+	int fd;
+	size_t len;
+	ssize_t wr;
+
+	fd = open("/sys/kernel/bpf_test/dummy_ops_ctl", O_WRONLY);
+	if (CHECK(fd < 0, "open", "open errno %d", errno))
+		goto out;
+
+	len = strlen(cmd);
+	wr = write(fd, cmd, len);
+	if (CHECK(wr != len, "write", "write cmd %s errno %d", cmd, errno))
+		goto out;
+out:
+	if (fd >= 0)
+		close(fd);
+}
+
+static void test_ret_value(void)
+{
+	int duration = 0;
+	struct bpf_dummy_ops *skel;
+	struct bpf_link *link;
+	char cmd[OPS_CTL_CMD_SIZE];
+
+	skel = bpf_dummy_ops__open_and_load();
+	if (CHECK(!skel, "bpf_dummy_ops__open_and_load", "failed\n"))
+		return;
+
+	skel->bss->init_ret = 1024;
+	link = bpf_map__attach_struct_ops(skel->maps.dummy);
+	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops"))
+		goto out;
+
+	snprintf(cmd, sizeof(cmd), "init_1 %d", skel->bss->init_ret);
+	do_ctl(cmd);
+out:
+	bpf_link__destroy(link);
+	bpf_dummy_ops__destroy(skel);
+}
+
+static void test_ret_by_ptr(void)
+{
+	int duration = 0;
+	struct bpf_dummy_ops *skel;
+	struct bpf_link *link;
+	char cmd[OPS_CTL_CMD_SIZE];
+
+	skel = bpf_dummy_ops__open_and_load();
+	if (CHECK(!skel, "bpf_dummy_ops__open_and_load", "failed\n"))
+		return;
+
+	skel->bss->state_val = 0x5a;
+	link = bpf_map__attach_struct_ops(skel->maps.dummy);
+	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops"))
+		goto out;
+
+	snprintf(cmd, sizeof(cmd), "init_2 %d", skel->bss->state_val);
+	do_ctl(cmd);
+out:
+	bpf_link__destroy(link);
+	bpf_dummy_ops__destroy(skel);
+}
+
+void test_bpf_dummy_ops(void)
+{
+	if (!env.has_testmod) {
+		test__skip();
+		return;
+	}
+
+	if (test__start_subtest("ret_value"))
+		test_ret_value();
+	if (test__start_subtest("ret_by_ptr"))
+		test_ret_by_ptr();
+}
diff --git a/tools/testing/selftests/bpf/progs/bpf_dummy_ops.c b/tools/testing/selftests/bpf/progs/bpf_dummy_ops.c
new file mode 100644
index 000000000000..e414532b3fc0
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bpf_dummy_ops.c
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2021. Huawei Technologies Co., Ltd */
+#include <stddef.h>
+#include <linux/bpf.h>
+#include <linux/types.h>
+#include <linux/stddef.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+struct bpf_dummy_ops_state {
+	int val;
+};
+
+struct bpf_dummy_ops {
+	int (*init)(void);
+};
+
+int state_val = 0;
+int init_ret = 0;
+
+SEC("struct_ops/dummy_ops_init")
+int BPF_PROG(dummy_ops_init, struct bpf_dummy_ops_state *state)
+{
+	if (state)
+		state->val = state_val;
+	return init_ret;
+}
+
+SEC(".struct_ops")
+struct bpf_dummy_ops dummy = {
+	.init = (void *)dummy_ops_init,
+};
+
+char _license[] SEC("license") = "GPL";
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH bpf-next 1/3] bpf: add dummy BPF STRUCT_OPS for test purpose
  2021-09-15  3:37 ` [RFC PATCH bpf-next 1/3] bpf: add dummy BPF STRUCT_OPS for test purpose Hou Tao
@ 2021-09-15 20:58   ` Martin KaFai Lau
  2021-09-18  2:03     ` Hou Tao
  2021-09-16  3:25   ` kernel test robot
  2021-09-16  7:09     ` kernel test robot
  2 siblings, 1 reply; 9+ messages in thread
From: Martin KaFai Lau @ 2021-09-15 20:58 UTC (permalink / raw)
  To: Hou Tao; +Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, netdev, 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 !

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH bpf-next 1/3] bpf: add dummy BPF STRUCT_OPS for test purpose
  2021-09-15  3:37 ` [RFC PATCH bpf-next 1/3] bpf: add dummy BPF STRUCT_OPS for test purpose Hou Tao
  2021-09-15 20:58   ` Martin KaFai Lau
@ 2021-09-16  3:25   ` kernel test robot
  2021-09-16  7:09     ` kernel test robot
  2 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2021-09-16  3:25 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 7394 bytes --]

Hi Hou,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on bpf/master]
[also build test WARNING on v5.15-rc1 next-20210915]
[cannot apply to bpf-next/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Hou-Tao/introduce-dummy-BPF-STRUCT_OPS/20210915-112614
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master
config: i386-randconfig-a016-20210916 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/3eeddb24d6b805983fd6147abf5bcaa65091ab2b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Hou-Tao/introduce-dummy-BPF-STRUCT_OPS/20210915-112614
        git checkout 3eeddb24d6b805983fd6147abf5bcaa65091ab2b
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from kernel/bpf/bpf_dummy_struct_ops.c:10:
   include/linux/bpf_dummy_ops.h:23:24: error: stray '\357' in program
      23 | struct bpf_dummy_ops {};
         |                        ^
   include/linux/bpf_dummy_ops.h:23:25: error: stray '\274' in program
      23 | struct bpf_dummy_ops {};
         |                         ^
   include/linux/bpf_dummy_ops.h:23:26: error: stray '\233' in program
      23 | struct bpf_dummy_ops {};
         |                          ^
>> include/linux/bpf_dummy_ops.h:24:1: warning: 'static' is not at beginning of declaration [-Wold-style-declaration]
      24 | static inline struct bpf_dummy_ops *bpf_get_dummy_ops(void) { return NULL; }
         | ^~~~~~
>> include/linux/bpf_dummy_ops.h:24:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration]
   include/linux/bpf_dummy_ops.h:24:15: error: expected ';', identifier or '(' before 'struct'
      24 | static inline struct bpf_dummy_ops *bpf_get_dummy_ops(void) { return NULL; }
         |               ^~~~~~
   include/linux/bpf_dummy_ops.h:24:15: error: 'inline' in empty declaration
>> include/linux/bpf_dummy_ops.h:24:37: warning: no previous prototype for 'bpf_get_dummy_ops' [-Wmissing-prototypes]
      24 | static inline struct bpf_dummy_ops *bpf_get_dummy_ops(void) { return NULL; }
         |                                     ^~~~~~~~~~~~~~~~~
   kernel/bpf/bpf_dummy_struct_ops.c:17:23: error: redefinition of 'bpf_get_dummy_ops'
      17 | struct bpf_dummy_ops *bpf_get_dummy_ops(void)
         |                       ^~~~~~~~~~~~~~~~~
   In file included from kernel/bpf/bpf_dummy_struct_ops.c:10:
   include/linux/bpf_dummy_ops.h:24:37: note: previous definition of 'bpf_get_dummy_ops' was here
      24 | static inline struct bpf_dummy_ops *bpf_get_dummy_ops(void) { return NULL; }
         |                                     ^~~~~~~~~~~~~~~~~
   kernel/bpf/bpf_dummy_struct_ops.c: In function 'bpf_get_dummy_ops':
   kernel/bpf/bpf_dummy_struct_ops.c:23:41: error: 'struct bpf_dummy_ops' has no member named 'owner'
      23 |  if (ops && !bpf_try_module_get(ops, ops->owner))
         |                                         ^~
   kernel/bpf/bpf_dummy_struct_ops.c: At top level:
   kernel/bpf/bpf_dummy_struct_ops.c:31:6: error: redefinition of 'bpf_put_dummy_ops'
      31 | void bpf_put_dummy_ops(struct bpf_dummy_ops *ops)
         |      ^~~~~~~~~~~~~~~~~
   In file included from kernel/bpf/bpf_dummy_struct_ops.c:10:
   include/linux/bpf_dummy_ops.h:25:20: note: previous definition of 'bpf_put_dummy_ops' was here
      25 | static inline void bpf_put_dummy_ops(struct bpf_dummy_ops *ops) {}
         |                    ^~~~~~~~~~~~~~~~~
   kernel/bpf/bpf_dummy_struct_ops.c: In function 'bpf_put_dummy_ops':
   kernel/bpf/bpf_dummy_struct_ops.c:33:25: error: 'struct bpf_dummy_ops' has no member named 'owner'
      33 |  bpf_module_put(ops, ops->owner);
         |                         ^~
   In file included from <command-line>:
   kernel/bpf/bpf_dummy_struct_ops.c: In function 'bpf_dummy_ops_btf_struct_access':
   include/linux/compiler_types.h:140:35: error: invalid use of undefined type 'struct bpf_dummy_ops_state'
     140 | #define __compiler_offsetof(a, b) __builtin_offsetof(a, b)
         |                                   ^~~~~~~~~~~~~~~~~~
   include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
      17 | #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
         |                                ^~~~~~~~~~~~~~~~~~~
   kernel/bpf/bpf_dummy_struct_ops.c:97:7: note: in expansion of macro 'offsetof'
      97 |  case offsetof(struct bpf_dummy_ops_state, val):
         |       ^~~~~~~~
   include/linux/compiler_types.h:140:35: error: invalid use of undefined type 'struct bpf_dummy_ops_state'
     140 | #define __compiler_offsetof(a, b) __builtin_offsetof(a, b)
         |                                   ^~~~~~~~~~~~~~~~~~
   include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
      17 | #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
         |                                ^~~~~~~~~~~~~~~~~~~
   include/linux/stddef.h:37:3: note: in expansion of macro 'offsetof'
      37 |  (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
         |   ^~~~~~~~
   kernel/bpf/bpf_dummy_struct_ops.c:98:9: note: in expansion of macro 'offsetofend'
      98 |   end = offsetofend(struct bpf_dummy_ops_state, val);
         |         ^~~~~~~~~~~
   In file included from include/uapi/linux/posix_types.h:5,
                    from include/uapi/linux/types.h:14,
                    from include/linux/types.h:6,
                    from include/linux/limits.h:6,
                    from include/linux/kernel.h:7,
                    from kernel/bpf/bpf_dummy_struct_ops.c:5:
   include/linux/stddef.h:28:55: error: dereferencing pointer to incomplete type 'struct bpf_dummy_ops_state'
      28 | #define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
         |                                                       ^~
   include/linux/stddef.h:37:28: note: in expansion of macro 'sizeof_field'
      37 |  (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
         |                            ^~~~~~~~~~~~
   kernel/bpf/bpf_dummy_struct_ops.c:98:9: note: in expansion of macro 'offsetofend'
      98 |   end = offsetofend(struct bpf_dummy_ops_state, val);
         |         ^~~~~~~~~~~


vim +/static +24 include/linux/bpf_dummy_ops.h

    19	
    20	extern struct bpf_dummy_ops *bpf_get_dummy_ops(void);
    21	extern void bpf_put_dummy_ops(struct bpf_dummy_ops *ops);
    22	#else
    23	struct bpf_dummy_ops {};
  > 24	static inline struct bpf_dummy_ops *bpf_get_dummy_ops(void) { return NULL; }
    25	static inline void bpf_put_dummy_ops(struct bpf_dummy_ops *ops) {}
    26	#endif
    27	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 30704 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH bpf-next 1/3] bpf: add dummy BPF STRUCT_OPS for test purpose
  2021-09-15  3:37 ` [RFC PATCH bpf-next 1/3] bpf: add dummy BPF STRUCT_OPS for test purpose Hou Tao
@ 2021-09-16  7:09     ` kernel test robot
  2021-09-16  3:25   ` kernel test robot
  2021-09-16  7:09     ` kernel test robot
  2 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2021-09-16  7:09 UTC (permalink / raw)
  To: Hou Tao; +Cc: llvm, kbuild-all

[-- Attachment #1: Type: text/plain, Size: 6243 bytes --]

Hi Hou,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on bpf/master]
[also build test ERROR on v5.15-rc1 next-20210915]
[cannot apply to bpf-next/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Hou-Tao/introduce-dummy-BPF-STRUCT_OPS/20210915-112614
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master
config: i386-buildonly-randconfig-r004-20210916 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 8cbbd7e0b2aa21ce7e416cfb63d9965518948c35)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/3eeddb24d6b805983fd6147abf5bcaa65091ab2b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Hou-Tao/introduce-dummy-BPF-STRUCT_OPS/20210915-112614
        git checkout 3eeddb24d6b805983fd6147abf5bcaa65091ab2b
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from kernel/bpf/bpf_dummy_struct_ops.c:10:
>> include/linux/bpf_dummy_ops.h:23:24: error: unexpected character <U+FF1B>
   struct bpf_dummy_ops {};
                          ^~
>> include/linux/bpf_dummy_ops.h:24:15: error: cannot combine with previous 'struct' declaration specifier
   static inline struct bpf_dummy_ops *bpf_get_dummy_ops(void) { return NULL; }
                 ^
   kernel/bpf/bpf_dummy_struct_ops.c:17:23: error: redefinition of 'bpf_get_dummy_ops'
   struct bpf_dummy_ops *bpf_get_dummy_ops(void)
                         ^
   include/linux/bpf_dummy_ops.h:24:37: note: previous definition is here
   static inline struct bpf_dummy_ops *bpf_get_dummy_ops(void) { return NULL; }
                                       ^
>> kernel/bpf/bpf_dummy_struct_ops.c:23:43: error: no member named 'owner' in 'struct bpf_dummy_ops'
           if (ops && !bpf_try_module_get(ops, ops->owner))
                                               ~~~  ^
   kernel/bpf/bpf_dummy_struct_ops.c:31:6: error: redefinition of 'bpf_put_dummy_ops'
   void bpf_put_dummy_ops(struct bpf_dummy_ops *ops)
        ^
   include/linux/bpf_dummy_ops.h:25:20: note: previous definition is here
   static inline void bpf_put_dummy_ops(struct bpf_dummy_ops *ops) {}
                      ^
   kernel/bpf/bpf_dummy_struct_ops.c:33:27: error: no member named 'owner' in 'struct bpf_dummy_ops'
           bpf_module_put(ops, ops->owner);
                               ~~~  ^
>> kernel/bpf/bpf_dummy_struct_ops.c:97:7: error: offsetof of incomplete type 'struct bpf_dummy_ops_state'
           case offsetof(struct bpf_dummy_ops_state, val):
                ^        ~~~~~~
   include/linux/stddef.h:17:32: note: expanded from macro 'offsetof'
   #define offsetof(TYPE, MEMBER)  __compiler_offsetof(TYPE, MEMBER)
                                   ^                   ~~~~
   include/linux/compiler_types.h:140:35: note: expanded from macro '__compiler_offsetof'
   #define __compiler_offsetof(a, b)       __builtin_offsetof(a, b)
                                           ^                  ~
   kernel/bpf/bpf_dummy_struct_ops.c:97:23: note: forward declaration of 'struct bpf_dummy_ops_state'
           case offsetof(struct bpf_dummy_ops_state, val):
                                ^
   kernel/bpf/bpf_dummy_struct_ops.c:98:9: error: offsetof of incomplete type 'struct bpf_dummy_ops_state'
                   end = offsetofend(struct bpf_dummy_ops_state, val);
                         ^           ~~~~~~
   include/linux/stddef.h:37:3: note: expanded from macro 'offsetofend'
           (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
            ^        ~~~~
   include/linux/stddef.h:17:32: note: expanded from macro 'offsetof'
   #define offsetof(TYPE, MEMBER)  __compiler_offsetof(TYPE, MEMBER)
                                   ^                   ~~~~
   include/linux/compiler_types.h:140:35: note: expanded from macro '__compiler_offsetof'
   #define __compiler_offsetof(a, b)       __builtin_offsetof(a, b)
                                           ^                  ~
   kernel/bpf/bpf_dummy_struct_ops.c:97:23: note: forward declaration of 'struct bpf_dummy_ops_state'
           case offsetof(struct bpf_dummy_ops_state, val):
                                ^
>> kernel/bpf/bpf_dummy_struct_ops.c:98:9: error: incomplete definition of type 'struct bpf_dummy_ops_state'
                   end = offsetofend(struct bpf_dummy_ops_state, val);
                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/stddef.h:37:28: note: expanded from macro 'offsetofend'
           (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/stddef.h:28:55: note: expanded from macro 'sizeof_field'
   #define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
                                              ~~~~~~~~~~~^
   kernel/bpf/bpf_dummy_struct_ops.c:97:23: note: forward declaration of 'struct bpf_dummy_ops_state'
           case offsetof(struct bpf_dummy_ops_state, val):
                                ^
   9 errors generated.


vim +23 include/linux/bpf_dummy_ops.h

    19	
    20	extern struct bpf_dummy_ops *bpf_get_dummy_ops(void);
    21	extern void bpf_put_dummy_ops(struct bpf_dummy_ops *ops);
    22	#else
  > 23	struct bpf_dummy_ops {};
  > 24	static inline struct bpf_dummy_ops *bpf_get_dummy_ops(void) { return NULL; }
    25	static inline void bpf_put_dummy_ops(struct bpf_dummy_ops *ops) {}
    26	#endif
    27	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 49045 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH bpf-next 1/3] bpf: add dummy BPF STRUCT_OPS for test purpose
@ 2021-09-16  7:09     ` kernel test robot
  0 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2021-09-16  7:09 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 6357 bytes --]

Hi Hou,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on bpf/master]
[also build test ERROR on v5.15-rc1 next-20210915]
[cannot apply to bpf-next/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Hou-Tao/introduce-dummy-BPF-STRUCT_OPS/20210915-112614
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master
config: i386-buildonly-randconfig-r004-20210916 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 8cbbd7e0b2aa21ce7e416cfb63d9965518948c35)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/3eeddb24d6b805983fd6147abf5bcaa65091ab2b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Hou-Tao/introduce-dummy-BPF-STRUCT_OPS/20210915-112614
        git checkout 3eeddb24d6b805983fd6147abf5bcaa65091ab2b
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from kernel/bpf/bpf_dummy_struct_ops.c:10:
>> include/linux/bpf_dummy_ops.h:23:24: error: unexpected character <U+FF1B>
   struct bpf_dummy_ops {};
                          ^~
>> include/linux/bpf_dummy_ops.h:24:15: error: cannot combine with previous 'struct' declaration specifier
   static inline struct bpf_dummy_ops *bpf_get_dummy_ops(void) { return NULL; }
                 ^
   kernel/bpf/bpf_dummy_struct_ops.c:17:23: error: redefinition of 'bpf_get_dummy_ops'
   struct bpf_dummy_ops *bpf_get_dummy_ops(void)
                         ^
   include/linux/bpf_dummy_ops.h:24:37: note: previous definition is here
   static inline struct bpf_dummy_ops *bpf_get_dummy_ops(void) { return NULL; }
                                       ^
>> kernel/bpf/bpf_dummy_struct_ops.c:23:43: error: no member named 'owner' in 'struct bpf_dummy_ops'
           if (ops && !bpf_try_module_get(ops, ops->owner))
                                               ~~~  ^
   kernel/bpf/bpf_dummy_struct_ops.c:31:6: error: redefinition of 'bpf_put_dummy_ops'
   void bpf_put_dummy_ops(struct bpf_dummy_ops *ops)
        ^
   include/linux/bpf_dummy_ops.h:25:20: note: previous definition is here
   static inline void bpf_put_dummy_ops(struct bpf_dummy_ops *ops) {}
                      ^
   kernel/bpf/bpf_dummy_struct_ops.c:33:27: error: no member named 'owner' in 'struct bpf_dummy_ops'
           bpf_module_put(ops, ops->owner);
                               ~~~  ^
>> kernel/bpf/bpf_dummy_struct_ops.c:97:7: error: offsetof of incomplete type 'struct bpf_dummy_ops_state'
           case offsetof(struct bpf_dummy_ops_state, val):
                ^        ~~~~~~
   include/linux/stddef.h:17:32: note: expanded from macro 'offsetof'
   #define offsetof(TYPE, MEMBER)  __compiler_offsetof(TYPE, MEMBER)
                                   ^                   ~~~~
   include/linux/compiler_types.h:140:35: note: expanded from macro '__compiler_offsetof'
   #define __compiler_offsetof(a, b)       __builtin_offsetof(a, b)
                                           ^                  ~
   kernel/bpf/bpf_dummy_struct_ops.c:97:23: note: forward declaration of 'struct bpf_dummy_ops_state'
           case offsetof(struct bpf_dummy_ops_state, val):
                                ^
   kernel/bpf/bpf_dummy_struct_ops.c:98:9: error: offsetof of incomplete type 'struct bpf_dummy_ops_state'
                   end = offsetofend(struct bpf_dummy_ops_state, val);
                         ^           ~~~~~~
   include/linux/stddef.h:37:3: note: expanded from macro 'offsetofend'
           (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
            ^        ~~~~
   include/linux/stddef.h:17:32: note: expanded from macro 'offsetof'
   #define offsetof(TYPE, MEMBER)  __compiler_offsetof(TYPE, MEMBER)
                                   ^                   ~~~~
   include/linux/compiler_types.h:140:35: note: expanded from macro '__compiler_offsetof'
   #define __compiler_offsetof(a, b)       __builtin_offsetof(a, b)
                                           ^                  ~
   kernel/bpf/bpf_dummy_struct_ops.c:97:23: note: forward declaration of 'struct bpf_dummy_ops_state'
           case offsetof(struct bpf_dummy_ops_state, val):
                                ^
>> kernel/bpf/bpf_dummy_struct_ops.c:98:9: error: incomplete definition of type 'struct bpf_dummy_ops_state'
                   end = offsetofend(struct bpf_dummy_ops_state, val);
                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/stddef.h:37:28: note: expanded from macro 'offsetofend'
           (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/stddef.h:28:55: note: expanded from macro 'sizeof_field'
   #define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
                                              ~~~~~~~~~~~^
   kernel/bpf/bpf_dummy_struct_ops.c:97:23: note: forward declaration of 'struct bpf_dummy_ops_state'
           case offsetof(struct bpf_dummy_ops_state, val):
                                ^
   9 errors generated.


vim +23 include/linux/bpf_dummy_ops.h

    19	
    20	extern struct bpf_dummy_ops *bpf_get_dummy_ops(void);
    21	extern void bpf_put_dummy_ops(struct bpf_dummy_ops *ops);
    22	#else
  > 23	struct bpf_dummy_ops {};
  > 24	static inline struct bpf_dummy_ops *bpf_get_dummy_ops(void) { return NULL; }
    25	static inline void bpf_put_dummy_ops(struct bpf_dummy_ops *ops) {}
    26	#endif
    27	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 49045 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH bpf-next 1/3] bpf: add dummy BPF STRUCT_OPS for test purpose
  2021-09-15 20:58   ` Martin KaFai Lau
@ 2021-09-18  2:03     ` Hou Tao
  0 siblings, 0 replies; 9+ messages in thread
From: Hou Tao @ 2021-09-18  2:03 UTC (permalink / raw)
  To: Martin KaFai Lau
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, netdev, bpf

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:
>> 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.
Will do and thanks for your suggestions.
>> +
>> +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",
>> +};
> .


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-09-18  2:03 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-15  3:37 [RFC PATCH bpf-next 0/3] introduce dummy BPF STRUCT_OPS Hou Tao
2021-09-15  3:37 ` [RFC PATCH bpf-next 1/3] bpf: add dummy BPF STRUCT_OPS for test purpose Hou Tao
2021-09-15 20:58   ` Martin KaFai Lau
2021-09-18  2:03     ` Hou Tao
2021-09-16  3:25   ` kernel test robot
2021-09-16  7:09   ` kernel test robot
2021-09-16  7:09     ` kernel test robot
2021-09-15  3:37 ` [RFC PATCH bpf-next 2/3] selftests/bpf: call dummy struct_ops in bpf_testmode Hou Tao
2021-09-15  3:37 ` [RFC PATCH bpf-next 3/3] selftests/bpf: add test for BPF STRUCT_OPS Hou Tao

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.