diff mbox series

[bpf-next,2/9] bpf: Move func_models from bpf_struct_ops to bpf_struct_ops_desc

Message ID 20241127-bpf-const-ops-v1-2-a698b8d58680@weissschuh.net (mailing list archive)
State New
Headers show
Series bpf: Constify BPF ops | expand

Commit Message

Thomas Weißschuh Nov. 27, 2024, 7:15 p.m. UTC 
The func_models are the only member of struct bpf_struct_ops which are
modified by the BPF core.
Moving it into bpf_struct_ops_desc allows the static definitions of
bpf_struct_ops throughout the tree being moved into read-only memory.

The dummy ops don't have access to a bpf_struct_ops_desc.
For them declare a dedicated static variable for the func_members.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 include/linux/bpf.h            | 4 ++--
 kernel/bpf/bpf_struct_ops.c    | 4 ++--
 net/bpf/bpf_dummy_struct_ops.c | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

Comments

Alexei Starovoitov Nov. 28, 2024, 2:43 a.m. UTC | #1 
On Wed, Nov 27, 2024 at 11:20 AM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> --- a/net/bpf/bpf_dummy_struct_ops.c
> +++ b/net/bpf/bpf_dummy_struct_ops.c
> @@ -129,7 +129,7 @@ extern const struct bpf_link_ops bpf_struct_ops_link_lops;
>  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 = &bpf_bpf_dummy_ops;
> +       static typeof_member(struct bpf_struct_ops_desc, func_models) func_models;
>         const struct btf_type *func_proto;
>         struct bpf_dummy_ops_test_args *args;
>         struct bpf_tramp_links *tlinks = NULL;
> @@ -175,7 +175,7 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
>
>         op_idx = prog->expected_attach_type;
>         err = bpf_struct_ops_prepare_trampoline(tlinks, link,
> -                                               &st_ops->func_models[op_idx],
> +                                               &func_models[op_idx],

This is sad. You didn't bother running the tests.
Above is producing garbage.
That's why so many BPF CI tests are failing.

Overall I think it's a minimal positive value to constify struct_ops.
Unless other bpf developers see a huge value
I'd prefer to keep the code as-is.

pw-bot: cr
diff mbox series

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 3ace0d6227e3cb70dd75bdc6afa52870dee250f1..b734543b7a2b48e717d4f1be765e0198114b4f0a 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1779,7 +1779,6 @@  struct btf_member;
  * @type: BTF type.
  * @value_type: Value type.
  * @name: The name of the struct bpf_struct_ops object.
- * @func_models: Func models
  * @type_id: BTF type id.
  * @value_id: BTF value id.
  */
@@ -1799,7 +1798,6 @@  struct bpf_struct_ops {
 	void *cfi_stubs;
 	struct module *owner;
 	const char *name;
-	struct btf_func_model func_models[BPF_STRUCT_OPS_MAX_NR_MEMBERS];
 };
 
 /* Every member of a struct_ops type has an instance even a member is not
@@ -1825,6 +1823,8 @@  struct bpf_struct_ops_desc {
 
 	/* Collection of argument information for each member */
 	struct bpf_struct_ops_arg_info *arg_info;
+
+	struct btf_func_model func_models[BPF_STRUCT_OPS_MAX_NR_MEMBERS];
 };
 
 enum bpf_struct_ops_state {
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 606efe32485a985b854aa7e96e1a7f7cfd5057d3..7ee7c25d21d480d46b40b23ff8aec9d12ea81f68 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -397,7 +397,7 @@  int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
 
 		if (btf_distill_func_proto(log, btf,
 					   func_proto, mname,
-					   &st_ops->func_models[i])) {
+					   &st_ops_desc->func_models[i])) {
 			pr_warn("Error in parsing func ptr %s in struct %s\n",
 				mname, st_ops->name);
 			err = -EINVAL;
@@ -777,7 +777,7 @@  static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 
 		trampoline_start = image_off;
 		err = bpf_struct_ops_prepare_trampoline(tlinks, link,
-						&st_ops->func_models[i],
+						&st_ops_desc->func_models[i],
 						*(void **)(st_ops->cfi_stubs + moff),
 						&image, &image_off,
 						st_map->image_pages_cnt < MAX_TRAMP_IMAGE_PAGES);
diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index f71f67c6896b3de862a556d6f1e959a378cbda96..3a675227074e546e6a02e80db3841ed72d5bbff8 100644
--- a/net/bpf/bpf_dummy_struct_ops.c
+++ b/net/bpf/bpf_dummy_struct_ops.c
@@ -129,7 +129,7 @@  extern const struct bpf_link_ops bpf_struct_ops_link_lops;
 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 = &bpf_bpf_dummy_ops;
+	static typeof_member(struct bpf_struct_ops_desc, func_models) func_models;
 	const struct btf_type *func_proto;
 	struct bpf_dummy_ops_test_args *args;
 	struct bpf_tramp_links *tlinks = NULL;
@@ -175,7 +175,7 @@  int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
 
 	op_idx = prog->expected_attach_type;
 	err = bpf_struct_ops_prepare_trampoline(tlinks, link,
-						&st_ops->func_models[op_idx],
+						&func_models[op_idx],
 						&dummy_ops_test_ret_function,
 						&image, &image_off,
 						true);