[PATCH bpf-next 0/5] introduce dummy BPF STRUCT_OPS

Hi,

Currently the test of BPF STRUCT_OPS depends on the specific bpf
implementation (e.g, 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.

Instead of loading a userspace-implemeted bpf_dummy_ops map into
kernel and calling the specific function by writing to sysfs provided
by bpf_testmode.ko, only loading bpf_dummy_ops related prog into
kernel and calling these prog by bpf_prog_test_run(). The latter
is more flexible and has no dependency on extra kernel module.

Now only the return value handling related test cases are supported,
if more is needed, we can add afterwards.

Comments are always welcome.
Regards,
Hou

Changelog:
* RFC: https://www.spinics.net/lists/bpf/msg46117.html


Hou Tao (5):
  bpf: add dummy BPF STRUCT_OPS for test purpose
  bpf: factor out a helper to prepare trampoline for struct_ops prog
  bpf: do .test_run in dummy BPF STRUCT_OPS
  bpf: hook .test_run for struct_ops program
  selftests/bpf: test return value handling for struct_ops prog

 include/linux/bpf.h                           |   5 +
 include/linux/bpf_dummy_ops.h                 |  25 ++
 kernel/bpf/bpf_struct_ops.c                   |  43 +++-
 kernel/bpf/bpf_struct_ops_types.h             |   2 +
 net/bpf/Makefile                              |   3 +
 net/bpf/bpf_dummy_struct_ops.c                | 220 ++++++++++++++++++
 .../selftests/bpf/prog_tests/dummy_st_ops.c   |  81 +++++++
 .../selftests/bpf/progs/dummy_st_ops.c        |  33 +++
 8 files changed, 403 insertions(+), 9 deletions(-)
 create mode 100644 include/linux/bpf_dummy_ops.h
 create mode 100644 net/bpf/bpf_dummy_struct_ops.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/dummy_st_ops.c
 create mode 100644 tools/testing/selftests/bpf/progs/dummy_st_ops.c

-- 
2.29.2

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.

Loading a bpf_dummy_ops implementation from userspace is prohibited,
and its only purpose is to run BPF_PROG_TYPE_STRUCT_OPS program
through bpf(BPF_PROG_TEST_RUN).

Signed-off-by: Hou Tao <redacted>
---
 include/linux/bpf_dummy_ops.h     | 14 ++++++++++
 kernel/bpf/bpf_struct_ops_types.h |  2 ++
 net/bpf/Makefile                  |  3 +++
 net/bpf/bpf_dummy_struct_ops.c    | 44 +++++++++++++++++++++++++++++++
 4 files changed, 63 insertions(+)
 create mode 100644 include/linux/bpf_dummy_ops.h
 create mode 100644 net/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..a594ae830eba
--- /dev/null
+++ b/include/linux/bpf_dummy_ops.h

@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2021. Huawei Technologies Co., Ltd
+ */
+#ifndef _BPF_DUMMY_OPS_H
+#define _BPF_DUMMY_OPS_H
+
+typedef int (*bpf_dummy_ops_init_t)(void);
+
+struct bpf_dummy_ops {
+	bpf_dummy_ops_init_t init;
+};
+
+#endif

diff --git a/kernel/bpf/bpf_struct_ops_types.h b/kernel/bpf/bpf_struct_ops_types.h
index 066d83ea1c99..02c86cf9c207 100644
--- a/kernel/bpf/bpf_struct_ops_types.h
+++ b/kernel/bpf/bpf_struct_ops_types.h

@@ -2,6 +2,8 @@
 /* internal file - do not include directly */
 
 #ifdef CONFIG_BPF_JIT
+#include <linux/bpf_dummy_ops.h>
+BPF_STRUCT_OPS_TYPE(bpf_dummy_ops)
 #ifdef CONFIG_INET
 #include <net/tcp.h>
 BPF_STRUCT_OPS_TYPE(tcp_congestion_ops)

diff --git a/net/bpf/Makefile b/net/bpf/Makefile
index 1c0a98d8c28f..1ebe270bde23 100644
--- a/net/bpf/Makefile
+++ b/net/bpf/Makefile

@@ -1,2 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_BPF_SYSCALL)	:= test_run.o
+ifeq ($(CONFIG_BPF_JIT),y)
+obj-$(CONFIG_BPF_SYSCALL)	+= bpf_dummy_struct_ops.o
+endif

diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
new file mode 100644
index 000000000000..1249e4bb4ccb
--- /dev/null
+++ b/net/bpf/bpf_dummy_struct_ops.c

@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021. Huawei Technologies Co., Ltd
+ */
+#include <linux/kernel.h>
+#include <linux/bpf_verifier.h>
+#include <linux/bpf.h>
+#include <linux/btf.h>
+#include <linux/bpf_dummy_ops.h>
+
+extern struct bpf_struct_ops bpf_bpf_dummy_ops;
+
+static int bpf_dummy_init(struct btf *btf)
+{
+	return 0;
+}
+
+static const struct bpf_verifier_ops bpf_dummy_verifier_ops = {
+};
+
+static int bpf_dummy_init_member(const struct btf_type *t,
+				 const struct btf_member *member,
+				 void *kdata, const void *udata)
+{
+	return -EOPNOTSUPP;
+}
+
+static int bpf_dummy_reg(void *kdata)
+{
+	return -EOPNOTSUPP;
+}
+
+static void bpf_dummy_unreg(void *kdata)
+{
+}
+
+struct bpf_struct_ops bpf_bpf_dummy_ops = {
+	.verifier_ops = &bpf_dummy_verifier_ops,
+	.init = bpf_dummy_init,
+	.init_member = bpf_dummy_init_member,
+	.reg = bpf_dummy_reg,
+	.unreg = bpf_dummy_unreg,
+	.name = "bpf_dummy_ops",
+};

-- 
2.29.2

Factor out a helper bpf_prepare_st_ops_prog() to prepare trampoline
for BPF_PROG_TYPE_STRUCT_OPS prog. It will be used by .test_run
callback in following patch.

Signed-off-by: Hou Tao <redacted>
---
 include/linux/bpf.h         |  5 +++++
 kernel/bpf/bpf_struct_ops.c | 26 +++++++++++++++++---------
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 155dfcfb8923..002bbb2c8bc7 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h

@@ -2224,4 +2224,9 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
 			u32 **bin_buf, u32 num_args);
 void bpf_bprintf_cleanup(void);
 
+int bpf_prepare_st_ops_prog(struct bpf_tramp_progs *tprogs,
+			    struct bpf_prog *prog,
+			    const struct btf_func_model *model,
+			    void *image, void *image_end);
+
 #endif /* _LINUX_BPF_H */

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 9abcc33f02cf..ec3c25174923 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c

@@ -312,6 +312,20 @@ static int check_zero_holes(const struct btf_type *t, void *data)
 	return 0;
 }
 
+int bpf_prepare_st_ops_prog(struct bpf_tramp_progs *tprogs,
+			    struct bpf_prog *prog,
+			    const struct btf_func_model *model,
+			    void *image, void *image_end)
+{
+	u32 flags;
+
+	tprogs[BPF_TRAMP_FENTRY].progs[0] = prog;
+	tprogs[BPF_TRAMP_FENTRY].nr_progs = 1;
+	flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0;
+	return arch_prepare_bpf_trampoline(NULL, image, image_end,
+					   model, flags, tprogs, NULL);
+}
+
 static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 					  void *value, u64 flags)
 {

@@ -368,7 +382,6 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 		const struct btf_type *mtype, *ptype;
 		struct bpf_prog *prog;
 		u32 moff;
-		u32 flags;
 
 		moff = btf_member_bit_offset(t, member) / 8;
 		ptype = btf_type_resolve_ptr(btf_vmlinux, member->type, NULL);

@@ -430,14 +443,9 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 			goto reset_unlock;
 		}
 
-		tprogs[BPF_TRAMP_FENTRY].progs[0] = prog;
-		tprogs[BPF_TRAMP_FENTRY].nr_progs = 1;
-		flags = st_ops->func_models[i].ret_size > 0 ?
-			BPF_TRAMP_F_RET_FENTRY_RET : 0;
-		err = arch_prepare_bpf_trampoline(NULL, image,
-						  st_map->image + PAGE_SIZE,
-						  &st_ops->func_models[i],
-						  flags, tprogs, NULL);
+		err = bpf_prepare_st_ops_prog(tprogs, prog,
+					      &st_ops->func_models[i],
+					      image, st_map->image + PAGE_SIZE);
 		if (err < 0)
 			goto reset_unlock;
 

-- 
2.29.2

Now only program for bpf_dummy_ops::init() is supported. The following
two cases are exercised in bpf_dummy_st_ops_test_run():

(1) test and check the value returned from state arg in init(state)
The content of state is copied from data_in before calling init() and
copied back to data_out after calling, so test program could use
data_in to pass the input state and use data_out to get the
output state.

(2) test and check the return value of init(NULL)
data_in_size is set as 0, so the state will be NULL and there will be
no copy-in & copy-out.

Signed-off-by: Hou Tao <redacted>
---
 include/linux/bpf_dummy_ops.h  |  13 ++-
 net/bpf/bpf_dummy_struct_ops.c | 176 +++++++++++++++++++++++++++++++++
 2 files changed, 188 insertions(+), 1 deletion(-)

diff --git a/include/linux/bpf_dummy_ops.h b/include/linux/bpf_dummy_ops.h
index a594ae830eba..5049484e6693 100644
--- a/include/linux/bpf_dummy_ops.h
+++ b/include/linux/bpf_dummy_ops.h

@@ -5,10 +5,21 @@
 #ifndef _BPF_DUMMY_OPS_H
 #define _BPF_DUMMY_OPS_H
 
-typedef int (*bpf_dummy_ops_init_t)(void);
+#include <linux/bpf.h>
+#include <linux/filter.h>
+
+struct bpf_dummy_ops_state {
+	int val;
+};
+
+typedef int (*bpf_dummy_ops_init_t)(struct bpf_dummy_ops_state *cb);
 
 struct bpf_dummy_ops {
 	bpf_dummy_ops_init_t init;
 };
 
+extern int bpf_dummy_st_ops_test_run(struct bpf_prog *prog,
+				     const union bpf_attr *kattr,
+				     union bpf_attr __user *uattr);
+
 #endif

diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index 1249e4bb4ccb..da77736cd093 100644
--- a/net/bpf/bpf_dummy_struct_ops.c
+++ b/net/bpf/bpf_dummy_struct_ops.c

@@ -10,12 +10,188 @@
 
 extern struct bpf_struct_ops bpf_bpf_dummy_ops;
 
+static const struct btf_type *dummy_ops_state;
+
+static struct bpf_dummy_ops_state *
+init_dummy_ops_state(const union bpf_attr *kattr)
+{
+	__u32 size_in;
+	struct bpf_dummy_ops_state *state;
+	void __user *data_in;
+
+	size_in = kattr->test.data_size_in;
+	if (!size_in)
+		return NULL;
+
+	if (size_in != sizeof(*state))
+		return ERR_PTR(-EINVAL);
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return ERR_PTR(-ENOMEM);
+
+	data_in = u64_to_user_ptr(kattr->test.data_in);
+	if (copy_from_user(state, data_in, size_in)) {
+		kfree(state);
+		return ERR_PTR(-EFAULT);
+	}
+
+	return state;
+}
+
+static int copy_dummy_ops_state(struct bpf_dummy_ops_state *state,
+				const union bpf_attr *kattr,
+				union bpf_attr __user *uattr)
+{
+	int err = 0;
+	void __user *data_out;
+
+	if (!state)
+		return 0;
+
+	data_out = u64_to_user_ptr(kattr->test.data_out);
+	if (copy_to_user(data_out, state, sizeof(*state))) {
+		err = -EFAULT;
+		goto out;
+	}
+	if (put_user(sizeof(*state), &uattr->test.data_size_out)) {
+		err = -EFAULT;
+		goto out;
+	}
+out:
+	return err;
+}
+
+static inline void exit_dummy_ops_state(struct bpf_dummy_ops_state *state)
+{
+	kfree(state);
+}
+
+int bpf_dummy_st_ops_test_run(struct bpf_prog *prog,
+			      const union bpf_attr *kattr,
+			      union bpf_attr __user *uattr)
+{
+	const struct bpf_struct_ops *st_ops = &bpf_bpf_dummy_ops;
+	struct bpf_dummy_ops_state *state = NULL;
+	struct bpf_tramp_progs *tprogs = NULL;
+	void *image = NULL;
+	int err;
+	int prog_ret;
+
+	/* Now only support to call init(...) */
+	if (prog->expected_attach_type != 0) {
+		err = -EOPNOTSUPP;
+		goto out;
+	}
+
+	/* state will be NULL when data_size_in == 0 */
+	state = init_dummy_ops_state(kattr);
+	if (IS_ERR(state)) {
+		err = PTR_ERR(state);
+		state = NULL;
+		goto out;
+	}
+
+	tprogs = kcalloc(BPF_TRAMP_MAX, sizeof(*tprogs), GFP_KERNEL);
+	if (!tprogs) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	image = bpf_jit_alloc_exec(PAGE_SIZE);
+	if (!image) {
+		err = -ENOMEM;
+		goto out;
+	}
+	set_vm_flush_reset_perms(image);
+
+	err = bpf_prepare_st_ops_prog(tprogs, prog, &st_ops->func_models[0],
+				      image, image + PAGE_SIZE);
+	if (err < 0)
+		goto out;
+
+	set_memory_ro((long)image, 1);
+	set_memory_x((long)image, 1);
+	prog_ret = ((bpf_dummy_ops_init_t)image)(state);
+
+	err = copy_dummy_ops_state(state, kattr, uattr);
+	if (err)
+		goto out;
+	if (put_user(prog_ret, &uattr->test.retval))
+		err = -EFAULT;
+out:
+	exit_dummy_ops_state(state);
+	bpf_jit_free_exec(image);
+	kfree(tprogs);
+	return err;
+}
+
 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 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)
+{
+	/* init(state) only has one argument */
+	if (off || type != BPF_READ)
+		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 = {
+	.is_valid_access = bpf_dummy_ops_is_valid_access,
+	.btf_struct_access = bpf_dummy_ops_btf_struct_access,
 };
 
 static int bpf_dummy_init_member(const struct btf_type *t,

-- 
2.29.2

Running a BPF_PROG_TYPE_STRUCT_OPS prog for dummy_st_ops::init()
through bpf_prog_test_run(). Three test cases are added:
(1) attach dummy_st_ops should fail
(2) function return value of bpf_dummy_ops::init() is expected
(3) pointer argument of bpf_dummy_ops::init() works as expected

Signed-off-by: Hou Tao <redacted>
---
 .../selftests/bpf/prog_tests/dummy_st_ops.c   | 81 +++++++++++++++++++
 .../selftests/bpf/progs/dummy_st_ops.c        | 33 ++++++++
 2 files changed, 114 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/dummy_st_ops.c
 create mode 100644 tools/testing/selftests/bpf/progs/dummy_st_ops.c

diff --git a/tools/testing/selftests/bpf/prog_tests/dummy_st_ops.c b/tools/testing/selftests/bpf/prog_tests/dummy_st_ops.c
new file mode 100644
index 000000000000..4b1b52b847e6
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/dummy_st_ops.c

@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2021. Huawei Technologies Co., Ltd */
+#include <test_progs.h>
+#include "dummy_st_ops.skel.h"
+
+/* Need to keep consistent with definitions in include/linux/bpf_dummy_ops.h */
+struct bpf_dummy_ops_state {
+	int val;
+};
+
+static void test_dummy_st_ops_attach(void)
+{
+	struct dummy_st_ops *skel;
+	struct bpf_link *link;
+
+	skel = dummy_st_ops__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "dummy_st_ops_load"))
+		goto out;
+
+	link = bpf_map__attach_struct_ops(skel->maps.dummy_1);
+	if (!ASSERT_EQ(libbpf_get_error(link), -EOPNOTSUPP,
+		       "dummy_st_ops_attach"))
+		goto out;
+out:
+	dummy_st_ops__destroy(skel);
+}
+
+static void test_dummy_init_ret_value(void)
+{
+	struct dummy_st_ops *skel;
+	int err, fd;
+	__u32 duration = 0, retval = 0;
+
+	skel = dummy_st_ops__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "dummy_st_ops_load"))
+		goto out;
+
+	fd = bpf_program__fd(skel->progs.init_1);
+	err = bpf_prog_test_run(fd, 1, NULL, 0,
+				NULL, NULL, &retval, &duration);
+	ASSERT_OK(err, "test_run");
+	ASSERT_EQ(retval, 0xf2f3f4f5, "test_ret");
+out:
+	dummy_st_ops__destroy(skel);
+}
+
+static void test_dummy_init_ptr_arg(void)
+{
+	struct dummy_st_ops *skel;
+	int err, fd;
+	__u32 duration = 0, retval = 0;
+	struct bpf_dummy_ops_state in_state, out_state;
+	__u32 state_size;
+
+	skel = dummy_st_ops__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "dummy_st_ops_load"))
+		goto out;
+
+	fd = bpf_program__fd(skel->progs.init_1);
+	memset(&in_state, 0, sizeof(in_state));
+	in_state.val = 0xbeef;
+	memset(&out_state, 0, sizeof(out_state));
+	err = bpf_prog_test_run(fd, 1, &in_state, sizeof(in_state),
+				&out_state, &state_size, &retval, &duration);
+	ASSERT_OK(err, "test_run");
+	ASSERT_EQ(state_size, sizeof(out_state), "test_data_out");
+	ASSERT_EQ(out_state.val, 0x5a, "test_ptr_ret");
+	ASSERT_EQ(retval, in_state.val, "test_ret");
+out:
+	dummy_st_ops__destroy(skel);
+}
+
+void test_dummy_st_ops(void)
+{
+	if (test__start_subtest("dummy_st_ops_attach"))
+		test_dummy_st_ops_attach();
+	if (test__start_subtest("dummy_init_ret_value"))
+		test_dummy_init_ret_value();
+	if (test__start_subtest("dummy_init_ptr_arg"))
+		test_dummy_init_ptr_arg();
+}

diff --git a/tools/testing/selftests/bpf/progs/dummy_st_ops.c b/tools/testing/selftests/bpf/progs/dummy_st_ops.c
new file mode 100644
index 000000000000..133c328f082a
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/dummy_st_ops.c

@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2021. Huawei Technologies Co., Ltd */
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+struct bpf_dummy_ops_state {
+	int val;
+} __attribute__((preserve_access_index));
+
+struct bpf_dummy_ops {
+	int (*init)(struct bpf_dummy_ops_state *state);
+};
+
+char _liencse[] SEC("license") = "GPL";
+
+SEC("struct_ops/init_1")
+int BPF_PROG(init_1, struct bpf_dummy_ops_state *state)
+{
+	int ret;
+
+	if (!state)
+		return 0xf2f3f4f5;
+
+	ret = state->val;
+	state->val = 0x5a;
+	return ret;
+}
+
+SEC(".struct_ops")
+struct bpf_dummy_ops dummy_1 = {
+	.init = (void *)init_1,
+};

-- 
2.29.2

bpf_struct_ops_test_run() will be used to run struct_ops program
from bpf_dummy_ops and now its main purpose is to test the handling
of return value.

Signed-off-by: Hou Tao <redacted>
---
 kernel/bpf/bpf_struct_ops.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index ec3c25174923..3cedd2f489db 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c

@@ -11,6 +11,9 @@
 #include <linux/refcount.h>
 #include <linux/mutex.h>
 
+static int bpf_struct_ops_test_run(struct bpf_prog *prog,
+				   const union bpf_attr *kattr,
+				   union bpf_attr __user *uattr);
 enum bpf_struct_ops_state {
 	BPF_STRUCT_OPS_STATE_INIT,
 	BPF_STRUCT_OPS_STATE_INUSE,

@@ -93,6 +96,7 @@ const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = {
 };
 
 const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
+	.test_run = bpf_struct_ops_test_run,
 };
 
 static const struct btf_type *module_type;

@@ -666,3 +670,16 @@ void bpf_struct_ops_put(const void *kdata)
 		call_rcu(&st_map->rcu, bpf_struct_ops_put_rcu);
 	}
 }
+
+static int bpf_struct_ops_test_run(struct bpf_prog *prog,
+				   const union bpf_attr *kattr,
+				   union bpf_attr __user *uattr)
+{
+	const struct bpf_struct_ops *st_ops;
+
+	st_ops = bpf_struct_ops_find(prog->aux->attach_btf_id);
+	if (st_ops != &bpf_bpf_dummy_ops)
+		return -EOPNOTSUPP;
+
+	return bpf_dummy_st_ops_test_run(prog, kattr, uattr);
+}

-- 
2.29.2

On Mon, Sep 27, 2021 at 7:38 PM Hou Tao [off-list ref] wrote:

no need for __destroy() as we haven't created skeleton, so this could
be just a return

+
+       link = bpf_map__attach_struct_ops(skel->maps.dummy_1);
+       if (!ASSERT_EQ(libbpf_get_error(link), -EOPNOTSUPP,
+                      "dummy_st_ops_attach"))
+               goto out;

nit: unless you expect to add something here soon, probably doing
ASSERT_EQ() and let it fall through to out: and destroy would be a bit
more readable

+out:
+       dummy_st_ops__destroy(skel);
+}
+
+static void test_dummy_init_ret_value(void)
+{
+       struct dummy_st_ops *skel;
+       int err, fd;
+       __u32 duration = 0, retval = 0;
+
+       skel = dummy_st_ops__open_and_load();
+       if (!ASSERT_OK_PTR(skel, "dummy_st_ops_load"))
+               goto out;

same, just return is fine and no need for out: label

+
+       fd = bpf_program__fd(skel->progs.init_1);
+       err = bpf_prog_test_run(fd, 1, NULL, 0,
+                               NULL, NULL, &retval, &duration);
+       ASSERT_OK(err, "test_run");
+       ASSERT_EQ(retval, 0xf2f3f4f5, "test_ret");
+out:
+       dummy_st_ops__destroy(skel);
+}
+
+static void test_dummy_init_ptr_arg(void)
+{
+       struct dummy_st_ops *skel;
+       int err, fd;
+       __u32 duration = 0, retval = 0;
+       struct bpf_dummy_ops_state in_state, out_state;
+       __u32 state_size;
+
+       skel = dummy_st_ops__open_and_load();
+       if (!ASSERT_OK_PTR(skel, "dummy_st_ops_load"))
+               goto out;

here as well

typo: _license (but it doesn't matter to libbpf, it looks at the
section name only

+
+SEC("struct_ops/init_1")
+int BPF_PROG(init_1, struct bpf_dummy_ops_state *state)
+{
+       int ret;
+
+       if (!state)
+               return 0xf2f3f4f5;
+
+       ret = state->val;
+       state->val = 0x5a;
+       return ret;
+}
+
+SEC(".struct_ops")
+struct bpf_dummy_ops dummy_1 = {
+       .init = (void *)init_1,
+};
--
2.29.2

On Tue, Sep 28, 2021 at 10:52:25AM +0800, Hou Tao wrote:

Factor out a helper bpf_prepare_st_ops_prog() to prepare trampoline
for BPF_PROG_TYPE_STRUCT_OPS prog. It will be used by .test_run
callback in following patch.

Thanks for the patches.

This preparation change should be the first patch instead.

Move it under where other bpf_struct_ops_.*() resides in bpf.h.

The existing struct_ops functions in the kernel now have naming like
bpf_struct_ops_.*().  How about renaming it to
bpf_struct_ops_prepare_trampoline()?

This change can't apply to bpf-next now because
commit 356ed64991c6 ("bpf: Handle return value of BPF_PROG_TYPE_STRUCT_OPS prog")
is not pulled into bpf-next yet.  Please mention the dependency
in the cover letter if it is still the case in v2.

-		err = arch_prepare_bpf_trampoline(NULL, image,
-						  st_map->image + PAGE_SIZE,
-						  &st_ops->func_models[i],
-						  flags, tprogs, NULL);
+		err = bpf_prepare_st_ops_prog(tprogs, prog,
+					      &st_ops->func_models[i],
+					      image, st_map->image + PAGE_SIZE);
 		if (err < 0)
 			goto reset_unlock;
 
-- 
2.29.2

On Tue, Sep 28, 2021 at 10:52:26AM +0800, Hou Tao wrote:

Now only program for bpf_dummy_ops::init() is supported. The following
two cases are exercised in bpf_dummy_st_ops_test_run():

(1) test and check the value returned from state arg in init(state)
The content of state is copied from data_in before calling init() and
copied back to data_out after calling, so test program could use
data_in to pass the input state and use data_out to get the
output state.

(2) test and check the return value of init(NULL)
data_in_size is set as 0, so the state will be NULL and there will be
no copy-in & copy-out.

Patch 1 and patch 3 in this set should be combined.

The changes here seem not worth a new header file.
Let see if they can be simplified and move the only needed things to bpf.h.

This struct can be moved to net/bpf/bpf_dummy_struct_ops.c.

+
+typedef int (*bpf_dummy_ops_init_t)(struct bpf_dummy_ops_state *cb);

If I read it correctly, the typedef is only useful in casting later.
It would need another typedef in the future if new test function is added.
Lets try to remove it (more on this later).

 
 struct bpf_dummy_ops {
 	bpf_dummy_ops_init_t init;

"init" is a little confusing since it is not doing initialization.
It is for testing purpose.  How about renaming it to test1, test2, test3...:

	int (*test1)(struct bpf_dummy_ops_state *cb);

Also, it should at least add another function to test more
arguments which is another limitation of testing with
tcp_congestion_ops.

 };

The whole struct bpf_dummy_ops can be moved to include/linux/bpf.h also
next to where other bpf_struct_ops_*() functions are residing.

 
+extern int bpf_dummy_st_ops_test_run(struct bpf_prog *prog,
+				     const union bpf_attr *kattr,
+				     union bpf_attr __user *uattr);

Same here.  It can be moved to include/linux/bpf.h and remove the
"extern" also.

These are the args for the test functions?  Using ctx_in/ctx_size_in
and ctx_out/ctx_size_out instead should be more consistent
with other bpf_prog_test_run* in test_run.c.

+	if (!size_in)
+		return NULL;
+
+	if (size_in != sizeof(*state))
+		return ERR_PTR(-EINVAL);
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return ERR_PTR(-ENOMEM);
+
+	data_in = u64_to_user_ptr(kattr->test.data_in);
+	if (copy_from_user(state, data_in, size_in)) {
+		kfree(state);
+		return ERR_PTR(-EFAULT);
+	}
+
+	return state;
+}
+
+static int copy_dummy_ops_state(struct bpf_dummy_ops_state *state,
+				const union bpf_attr *kattr,
+				union bpf_attr __user *uattr)
+{
+	int err = 0;
+	void __user *data_out;
+
+	if (!state)
+		return 0;
+
+	data_out = u64_to_user_ptr(kattr->test.data_out);
+	if (copy_to_user(data_out, state, sizeof(*state))) {
+		err = -EFAULT;
+		goto out;

Directly return err;

+	}
+	if (put_user(sizeof(*state), &uattr->test.data_size_out)) {
+		err = -EFAULT;
+		goto out;

Same here.

+	}
+out:
+	return err;
+}
+
+static inline void exit_dummy_ops_state(struct bpf_dummy_ops_state *state)

static is good enough.  no need to inline.  Allow the compiler to decide.

+{
+	kfree(state);

Probably just remove this helper function and directly call kfree instead.

Could you help to check if bpf_ctx_init and bpf_ctx_finish can be directly
reused instead?  I haven't looked at them closely to compare yet.

+}
+
+int bpf_dummy_st_ops_test_run(struct bpf_prog *prog,
+			      const union bpf_attr *kattr,
+			      union bpf_attr __user *uattr)
+{
+	const struct bpf_struct_ops *st_ops = &bpf_bpf_dummy_ops;
+	struct bpf_dummy_ops_state *state = NULL;
+	struct bpf_tramp_progs *tprogs = NULL;
+	void *image = NULL;
+	int err;
+	int prog_ret;
+
+	/* Now only support to call init(...) */
+	if (prog->expected_attach_type != 0) {
+		err = -EOPNOTSUPP;
+		goto out;
+	}
+
+	/* state will be NULL when data_size_in == 0 */
+	state = init_dummy_ops_state(kattr);
+	if (IS_ERR(state)) {
+		err = PTR_ERR(state);
+		state = NULL;
+		goto out;
+	}
+
+	tprogs = kcalloc(BPF_TRAMP_MAX, sizeof(*tprogs), GFP_KERNEL);
+	if (!tprogs) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	image = bpf_jit_alloc_exec(PAGE_SIZE);
+	if (!image) {
+		err = -ENOMEM;
+		goto out;
+	}
+	set_vm_flush_reset_perms(image);
+
+	err = bpf_prepare_st_ops_prog(tprogs, prog, &st_ops->func_models[0],
+				      image, image + PAGE_SIZE);
+	if (err < 0)
+		goto out;
+
+	set_memory_ro((long)image, 1);
+	set_memory_x((long)image, 1);
+	prog_ret = ((bpf_dummy_ops_init_t)image)(state);

I would do something like this to avoid creating the
bpf_dummy_ops_init_t typedef.

	struct bpf_dummy_ops ops;

	ops.init = (void *)image;
	prog_ret = ops.init(state);

+
+	err = copy_dummy_ops_state(state, kattr, uattr);
+	if (err)
+		goto out;
+	if (put_user(prog_ret, &uattr->test.retval))
+		err = -EFAULT;
+out:
+	exit_dummy_ops_state(state);
+	bpf_jit_free_exec(image);
+	kfree(tprogs);
+	return err;
+}
+
 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 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)
+{
+	/* init(state) only has one argument */
+	if (off || type != BPF_READ)
+		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;
+	}

The idea is to only allow writing to dummy_ops_state?

How about something like this (uncompiled code):

	int ret;

	if (atype != BPF_READ && t != dummy_ops_state)
		return -EACCES;

	ret = btf_struct_access(log, btf, t, off, size, atype,
				next_btf_id);
	if (ret < 0)
		return ret;

	return atype == BPF_READ ? ret : NOT_INIT;

Then the following switch and offset logic can go away.

+
+	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 = {
+	.is_valid_access = bpf_dummy_ops_is_valid_access,
+	.btf_struct_access = bpf_dummy_ops_btf_struct_access,
 };
 
 static int bpf_dummy_init_member(const struct btf_type *t,
-- 
2.29.2

Hi

On 9/30/2021 1:56 AM, Martin KaFai Lau wrote:

On Tue, Sep 28, 2021 at 10:52:25AM +0800, Hou Tao wrote:

quoted

Factor out a helper bpf_prepare_st_ops_prog() to prepare trampoline
for BPF_PROG_TYPE_STRUCT_OPS prog. It will be used by .test_run
callback in following patch.

Thanks for the patches.

Thanks for you review.

This preparation change should be the first patch instead.

Will do.

quoted

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 155dfcfb8923..002bbb2c8bc7 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h

@@ -2224,4 +2224,9 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
 			u32 **bin_buf, u32 num_args);
 void bpf_bprintf_cleanup(void);
 
+int bpf_prepare_st_ops_prog(struct bpf_tramp_progs *tprogs,
+			    struct bpf_prog *prog,
+			    const struct btf_func_model *model,
+			    void *image, void *image_end);

Move it under where other bpf_struct_ops_.*() resides in bpf.h.

quoted

+
 #endif /* _LINUX_BPF_H */

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 9abcc33f02cf..ec3c25174923 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c

@@ -312,6 +312,20 @@ static int check_zero_holes(const struct btf_type *t, void *data)
 	return 0;
 }
 
+int bpf_prepare_st_ops_prog(struct bpf_tramp_progs *tprogs,
+			    struct bpf_prog *prog,
+			    const struct btf_func_model *model,
+			    void *image, void *image_end)

The existing struct_ops functions in the kernel now have naming like
bpf_struct_ops_.*().  How about renaming it to
bpf_struct_ops_prepare_trampoline()?

bpf_struct_ops_prepare_trampoline() may be a little long, and it will make
the indentations of its parameters look ugly, so how about
bpf_struct_ops_prep_prog() ?

quoted

+{
+	u32 flags;
+
+	tprogs[BPF_TRAMP_FENTRY].progs[0] = prog;
+	tprogs[BPF_TRAMP_FENTRY].nr_progs = 1;
+	flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0;
+	return arch_prepare_bpf_trampoline(NULL, image, image_end,
+					   model, flags, tprogs, NULL);
+}
+
 static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 					  void *value, u64 flags)
 {

@@ -368,7 +382,6 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 		const struct btf_type *mtype, *ptype;
 		struct bpf_prog *prog;
 		u32 moff;
-		u32 flags;
 
 		moff = btf_member_bit_offset(t, member) / 8;
 		ptype = btf_type_resolve_ptr(btf_vmlinux, member->type, NULL);

@@ -430,14 +443,9 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 			goto reset_unlock;
 		}
 
-		tprogs[BPF_TRAMP_FENTRY].progs[0] = prog;
-		tprogs[BPF_TRAMP_FENTRY].nr_progs = 1;
-		flags = st_ops->func_models[i].ret_size > 0 ?
-			BPF_TRAMP_F_RET_FENTRY_RET : 0;

This change can't apply to bpf-next now because
commit 356ed64991c6 ("bpf: Handle return value of BPF_PROG_TYPE_STRUCT_OPS prog")
is not pulled into bpf-next yet.  Please mention the dependency
in the cover letter if it is still the case in v2.

Thanks for the reminder. Will do.

quoted

-		err = arch_prepare_bpf_trampoline(NULL, image,
-						  st_map->image + PAGE_SIZE,
-						  &st_ops->func_models[i],
-						  flags, tprogs, NULL);
+		err = bpf_prepare_st_ops_prog(tprogs, prog,
+					      &st_ops->func_models[i],
+					      image, st_map->image + PAGE_SIZE);
 		if (err < 0)
 			goto reset_unlock;
 
-- 
2.29.2

.

Hi,

On 9/30/2021 2:55 AM, Martin KaFai Lau wrote:

On Tue, Sep 28, 2021 at 10:52:26AM +0800, Hou Tao wrote:

quoted

Now only program for bpf_dummy_ops::init() is supported. The following
two cases are exercised in bpf_dummy_st_ops_test_run():

(1) test and check the value returned from state arg in init(state)
The content of state is copied from data_in before calling init() and
copied back to data_out after calling, so test program could use
data_in to pass the input state and use data_out to get the
output state.

(2) test and check the return value of init(NULL)
data_in_size is set as 0, so the state will be NULL and there will be
no copy-in & copy-out.

Patch 1 and patch 3 in this set should be combined.

Will do. The purpose of splitting into two patches is that if only the return
value test is needed, patch 3 can be dropped. But now we will add more
tests, so i think combine two patches into one is OK.

quoted

 include/linux/bpf_dummy_ops.h  |  13 ++-
 net/bpf/bpf_dummy_struct_ops.c | 176 +++++++++++++++++++++++++++++++++
 2 files changed, 188 insertions(+), 1 deletion(-)

diff --git a/include/linux/bpf_dummy_ops.h b/include/linux/bpf_dummy_ops.h
index a594ae830eba..5049484e6693 100644
--- a/include/linux/bpf_dummy_ops.h
+++ b/include/linux/bpf_dummy_ops.h

The changes here seem not worth a new header file.
Let see if they can be simplified and move the only needed things to bpf.h.

quoted

@@ -5,10 +5,21 @@
 #ifndef _BPF_DUMMY_OPS_H
 #define _BPF_DUMMY_OPS_H
 
-typedef int (*bpf_dummy_ops_init_t)(void);
+#include <linux/bpf.h>
+#include <linux/filter.h>
+
+struct bpf_dummy_ops_state {
+	int val;
+};

This struct can be moved to net/bpf/bpf_dummy_struct_ops.c.

quoted

+
+typedef int (*bpf_dummy_ops_init_t)(struct bpf_dummy_ops_state *cb);

If I read it correctly, the typedef is only useful in casting later.
It would need another typedef in the future if new test function is added.
Lets try to remove it (more on this later).

quoted

 
 struct bpf_dummy_ops {
 	bpf_dummy_ops_init_t init;

"init" is a little confusing since it is not doing initialization.
It is for testing purpose.  How about renaming it to test1, test2, test3...:

	int (*test1)(struct bpf_dummy_ops_state *cb);

Also, it should at least add another function to test more
arguments which is another limitation of testing with
tcp_congestion_ops.

quoted

 };

The whole struct bpf_dummy_ops can be moved to include/linux/bpf.h also
next to where other bpf_struct_ops_*() functions are residing.

Will do. Thanks for your suggestions.

quoted

 
+extern int bpf_dummy_st_ops_test_run(struct bpf_prog *prog,
+				     const union bpf_attr *kattr,
+				     union bpf_attr __user *uattr);

Same here.  It can be moved to include/linux/bpf.h and remove the
"extern" also.

quoted

+
 #endif

diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index 1249e4bb4ccb..da77736cd093 100644
--- a/net/bpf/bpf_dummy_struct_ops.c
+++ b/net/bpf/bpf_dummy_struct_ops.c

@@ -10,12 +10,188 @@
 
 extern struct bpf_struct_ops bpf_bpf_dummy_ops;
 
+static const struct btf_type *dummy_ops_state;
+
+static struct bpf_dummy_ops_state *
+init_dummy_ops_state(const union bpf_attr *kattr)
+{
+	__u32 size_in;
+	struct bpf_dummy_ops_state *state;
+	void __user *data_in;
+
+	size_in = kattr->test.data_size_in;

These are the args for the test functions?  Using ctx_in/ctx_size_in
and ctx_out/ctx_size_out instead should be more consistent
with other bpf_prog_test_run* in test_run.c.

Yes, there are args. I had think about using ctx_in/ctx_out, but I didn't
because I thought the program which using ctx_in/ctx_out only has
one argument (namely bpf_context *), but the bpf_dummy_ops::init
may have multiple arguments. Anyway I will check it again and use
ctx_in/ctx_out if possible.

quoted

+	if (!size_in)
+		return NULL;
+
+	if (size_in != sizeof(*state))
+		return ERR_PTR(-EINVAL);
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return ERR_PTR(-ENOMEM);
+
+	data_in = u64_to_user_ptr(kattr->test.data_in);
+	if (copy_from_user(state, data_in, size_in)) {
+		kfree(state);
+		return ERR_PTR(-EFAULT);
+	}
+
+	return state;
+}
+
+static int copy_dummy_ops_state(struct bpf_dummy_ops_state *state,
+				const union bpf_attr *kattr,
+				union bpf_attr __user *uattr)
+{
+	int err = 0;
+	void __user *data_out;
+
+	if (!state)
+		return 0;
+
+	data_out = u64_to_user_ptr(kattr->test.data_out);
+	if (copy_to_user(data_out, state, sizeof(*state))) {
+		err = -EFAULT;
+		goto out;

Directly return err;

Will do

quoted

+	}
+	if (put_user(sizeof(*state), &uattr->test.data_size_out)) {
+		err = -EFAULT;
+		goto out;

Same here.

quoted

+	}
+out:
+	return err;
+}
+
+static inline void exit_dummy_ops_state(struct bpf_dummy_ops_state *state)

static is good enough.  no need to inline.  Allow the compiler to decide.

quoted

+{
+	kfree(state);

Probably just remove this helper function and directly call kfree instead.

Could you help to check if bpf_ctx_init and bpf_ctx_finish can be directly
reused instead?  I haven't looked at them closely to compare yet.

Will do.

quoted

+}
+
+int bpf_dummy_st_ops_test_run(struct bpf_prog *prog,
+			      const union bpf_attr *kattr,
+			      union bpf_attr __user *uattr)
+{
+	const struct bpf_struct_ops *st_ops = &bpf_bpf_dummy_ops;
+	struct bpf_dummy_ops_state *state = NULL;
+	struct bpf_tramp_progs *tprogs = NULL;
+	void *image = NULL;
+	int err;
+	int prog_ret;
+
+	/* Now only support to call init(...) */
+	if (prog->expected_attach_type != 0) {
+		err = -EOPNOTSUPP;
+		goto out;
+	}
+
+	/* state will be NULL when data_size_in == 0 */
+	state = init_dummy_ops_state(kattr);
+	if (IS_ERR(state)) {
+		err = PTR_ERR(state);
+		state = NULL;
+		goto out;
+	}
+
+	tprogs = kcalloc(BPF_TRAMP_MAX, sizeof(*tprogs), GFP_KERNEL);
+	if (!tprogs) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	image = bpf_jit_alloc_exec(PAGE_SIZE);
+	if (!image) {
+		err = -ENOMEM;
+		goto out;
+	}
+	set_vm_flush_reset_perms(image);
+
+	err = bpf_prepare_st_ops_prog(tprogs, prog, &st_ops->func_models[0],
+				      image, image + PAGE_SIZE);
+	if (err < 0)
+		goto out;
+
+	set_memory_ro((long)image, 1);
+	set_memory_x((long)image, 1);
+	prog_ret = ((bpf_dummy_ops_init_t)image)(state);

I would do something like this to avoid creating the
bpf_dummy_ops_init_t typedef.

	struct bpf_dummy_ops ops;

	ops.init = (void *)image;
	prog_ret = ops.init(state);

Good idea. Will do that.

quoted

+
+	err = copy_dummy_ops_state(state, kattr, uattr);
+	if (err)
+		goto out;
+	if (put_user(prog_ret, &uattr->test.retval))
+		err = -EFAULT;
+out:
+	exit_dummy_ops_state(state);
+	bpf_jit_free_exec(image);
+	kfree(tprogs);
+	return err;
+}
+
 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 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)
+{
+	/* init(state) only has one argument */
+	if (off || type != BPF_READ)
+		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;
+	}

The idea is to only allow writing to dummy_ops_state?

How about something like this (uncompiled code):

	int ret;

	if (atype != BPF_READ && t != dummy_ops_state)
		return -EACCES;

	ret = btf_struct_access(log, btf, t, off, size, atype,
				next_btf_id);
	if (ret < 0)
		return ret;

	return atype == BPF_READ ? ret : NOT_INIT;

Then the following switch and offset logic can go away.

Good idea. Will do that in v2.

quoted

+
+	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 = {
+	.is_valid_access = bpf_dummy_ops_is_valid_access,
+	.btf_struct_access = bpf_dummy_ops_btf_struct_access,
 };
 
 static int bpf_dummy_init_member(const struct btf_type *t,
-- 
2.29.2

.

Hi,

On 9/29/2021 7:19 AM, Andrii Nakryiko wrote:

On Mon, Sep 27, 2021 at 7:38 PM Hou Tao [off-list ref] wrote:

quoted

Running a BPF_PROG_TYPE_STRUCT_OPS prog for dummy_st_ops::init()
through bpf_prog_test_run(). Three test cases are added:
(1) attach dummy_st_ops should fail
(2) function return value of bpf_dummy_ops::init() is expected
(3) pointer argument of bpf_dummy_ops::init() works as expected

Signed-off-by: Hou Tao <redacted>
---
 .../selftests/bpf/prog_tests/dummy_st_ops.c   | 81 +++++++++++++++++++
 .../selftests/bpf/progs/dummy_st_ops.c        | 33 ++++++++
 2 files changed, 114 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/dummy_st_ops.c
 create mode 100644 tools/testing/selftests/bpf/progs/dummy_st_ops.c

diff --git a/tools/testing/selftests/bpf/prog_tests/dummy_st_ops.c b/tools/testing/selftests/bpf/prog_tests/dummy_st_ops.c
new file mode 100644
index 000000000000..4b1b52b847e6
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/dummy_st_ops.c

@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2021. Huawei Technologies Co., Ltd */
+#include <test_progs.h>
+#include "dummy_st_ops.skel.h"
+
+/* Need to keep consistent with definitions in include/linux/bpf_dummy_ops.h */
+struct bpf_dummy_ops_state {
+       int val;
+};
+
+static void test_dummy_st_ops_attach(void)
+{
+       struct dummy_st_ops *skel;
+       struct bpf_link *link;
+
+       skel = dummy_st_ops__open_and_load();
+       if (!ASSERT_OK_PTR(skel, "dummy_st_ops_load"))
+               goto out;

no need for __destroy() as we haven't created skeleton, so this could
be just a return

Will do.

quoted

+
+       link = bpf_map__attach_struct_ops(skel->maps.dummy_1);
+       if (!ASSERT_EQ(libbpf_get_error(link), -EOPNOTSUPP,
+                      "dummy_st_ops_attach"))
+               goto out;

nit: unless you expect to add something here soon, probably doing
ASSERT_EQ() and let it fall through to out: and destroy would be a bit
more readable

Make sense. Will do.

quoted

+out:
+       dummy_st_ops__destroy(skel);
+}
+
+static void test_dummy_init_ret_value(void)
+{
+       struct dummy_st_ops *skel;
+       int err, fd;
+       __u32 duration = 0, retval = 0;
+
+       skel = dummy_st_ops__open_and_load();
+       if (!ASSERT_OK_PTR(skel, "dummy_st_ops_load"))
+               goto out;

same, just return is fine and no need for out: label

OK. Will do in v2.

quoted

+
+       fd = bpf_program__fd(skel->progs.init_1);
+       err = bpf_prog_test_run(fd, 1, NULL, 0,
+                               NULL, NULL, &retval, &duration);
+       ASSERT_OK(err, "test_run");
+       ASSERT_EQ(retval, 0xf2f3f4f5, "test_ret");
+out:
+       dummy_st_ops__destroy(skel);
+}
+
+static void test_dummy_init_ptr_arg(void)
+{
+       struct dummy_st_ops *skel;
+       int err, fd;
+       __u32 duration = 0, retval = 0;
+       struct bpf_dummy_ops_state in_state, out_state;
+       __u32 state_size;
+
+       skel = dummy_st_ops__open_and_load();
+       if (!ASSERT_OK_PTR(skel, "dummy_st_ops_load"))
+               goto out;

here as well

OK. Will do.

quoted

+
+       fd = bpf_program__fd(skel->progs.init_1);
+       memset(&in_state, 0, sizeof(in_state));
+       in_state.val = 0xbeef;
+       memset(&out_state, 0, sizeof(out_state));
+       err = bpf_prog_test_run(fd, 1, &in_state, sizeof(in_state),
+                               &out_state, &state_size, &retval, &duration);
+       ASSERT_OK(err, "test_run");
+       ASSERT_EQ(state_size, sizeof(out_state), "test_data_out");
+       ASSERT_EQ(out_state.val, 0x5a, "test_ptr_ret");
+       ASSERT_EQ(retval, in_state.val, "test_ret");
+out:
+       dummy_st_ops__destroy(skel);
+}
+
+void test_dummy_st_ops(void)
+{
+       if (test__start_subtest("dummy_st_ops_attach"))
+               test_dummy_st_ops_attach();
+       if (test__start_subtest("dummy_init_ret_value"))
+               test_dummy_init_ret_value();
+       if (test__start_subtest("dummy_init_ptr_arg"))
+               test_dummy_init_ptr_arg();
+}

diff --git a/tools/testing/selftests/bpf/progs/dummy_st_ops.c b/tools/testing/selftests/bpf/progs/dummy_st_ops.c
new file mode 100644
index 000000000000..133c328f082a
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/dummy_st_ops.c

@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2021. Huawei Technologies Co., Ltd */
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+struct bpf_dummy_ops_state {
+       int val;
+} __attribute__((preserve_access_index));
+
+struct bpf_dummy_ops {
+       int (*init)(struct bpf_dummy_ops_state *state);
+};
+
+char _liencse[] SEC("license") = "GPL";

typo: _license (but it doesn't matter to libbpf, it looks at the
section name only

Will fix.

quoted

+
+SEC("struct_ops/init_1")
+int BPF_PROG(init_1, struct bpf_dummy_ops_state *state)
+{
+       int ret;
+
+       if (!state)
+               return 0xf2f3f4f5;
+
+       ret = state->val;
+       state->val = 0x5a;
+       return ret;
+}
+
+SEC(".struct_ops")
+struct bpf_dummy_ops dummy_1 = {
+       .init = (void *)init_1,
+};
--
2.29.2

.

On Thu, Sep 30, 2021 at 06:17:33PM +0800, Hou Tao wrote:

quoted

quoted

+int bpf_prepare_st_ops_prog(struct bpf_tramp_progs *tprogs,
+			    struct bpf_prog *prog,
+			    const struct btf_func_model *model,
+			    void *image, void *image_end)

The existing struct_ops functions in the kernel now have naming like
bpf_struct_ops_.*().  How about renaming it to
bpf_struct_ops_prepare_trampoline()?

bpf_struct_ops_prepare_trampoline() may be a little long, and it will make
the indentations of its parameters look ugly, so how about
bpf_struct_ops_prep_prog() ?

hmm... naming is hard...
but it is preparing the trampoline instead of preparing the
prog, and most other bpf funcs are using 'prepare' instead of 'prep'.
My preference is a better naming on what the func does and a
consistent naming with others.  The indentation looks fine also.

It is not too bad ;)
bpf_struct_ops_prepare_prog()
arch_prepare_bpf_trampoline()
bpf_struct_ops_prepare_trampoline()

The params indentation looks fine and within 80 cols:

int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_progs *tprogs,
				      struct bpf_prog *prog,
				      const struct btf_func_model *model,
				      void *image, void *image_end0
{

}

On Thu, Sep 30, 2021 at 07:05:41PM +0800, Hou Tao wrote:

quoted

quoted

diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index 1249e4bb4ccb..da77736cd093 100644
--- a/net/bpf/bpf_dummy_struct_ops.c
+++ b/net/bpf/bpf_dummy_struct_ops.c

@@ -10,12 +10,188 @@
 
 extern struct bpf_struct_ops bpf_bpf_dummy_ops;
 
+static const struct btf_type *dummy_ops_state;
+
+static struct bpf_dummy_ops_state *
+init_dummy_ops_state(const union bpf_attr *kattr)
+{
+	__u32 size_in;
+	struct bpf_dummy_ops_state *state;
+	void __user *data_in;
+
+	size_in = kattr->test.data_size_in;

These are the args for the test functions?  Using ctx_in/ctx_size_in
and ctx_out/ctx_size_out instead should be more consistent
with other bpf_prog_test_run* in test_run.c.

Yes, there are args. I had think about using ctx_in/ctx_out, but I didn't
because I thought the program which using ctx_in/ctx_out only has
one argument (namely bpf_context *), but the bpf_dummy_ops::init
may have multiple arguments. Anyway I will check it again and use
ctx_in/ctx_out if possible.

got it.

ctx_in could have multiple args.
I was more thinking on the muliple arg test also. Potentially some of them
are just integers, e.g. 

int test2(struct bpf_dummy_ops_state *state, char a, short b, int c, long d)
{

}

All args can be put in ctx_in like bpf_prog_test_run_raw_tp().
Take a look at raw_tp_test_run.c.  Although it is not strictly
necessary to use u64 for all args in the struct_ops test
because the struct_ops test still wants to prepare the
trampoline to catch the return value issue...etc,  passing
an array of u64 args in ctx_in should make it easier to program
the userspace and optimizing the ctx_in based on the sizeof each
arg seems not gaining much as a test also.

For "struct bpf_dummy_ops_state *state", instead of making an
exception to pass ptr arg in data_in,  the user ptr can be directly
passed as a u64 stored in ctx_in also, then there is no need to use
data_in or data_size_in.  If it is needed, the userspace's
sizeof(struct bpf_dummy_ops_state) can be found from the
prog->aux->btf.
There is no need to use data_out/data_out_size also, just directly
copy it back to the same user ptr stored in ctx_in.