[PATCH bpf-next v1 0/8] libbpf: type suffixes and autocreate flag for struct_ops maps

[PATCH bpf-next v1 0/8] libbpf: type suffixes and autocreate flag for struct_ops maps

[Eduard Zingerman]

Tweak struct_ops related APIs to allow the following features:
- specify version suffixes for stuct_ops map types;
- share same BPF program between several map definitions with
  different local BTF types, assuming only maps with same
  kernel BTF type would be selected for load;
- toggle autocreate flag for struct_ops maps;
- toggle autoload for referenced struct_ops programs
  when autocreate flag of struct_ops maps is toggled.

This would allow loading programs like below:

    SEC("struct_ops/foo") int BPF_PROG(foo) { ... }
    SEC("struct_ops/bar") int BPF_PROG(bar) { ... }

    struct bpf_testmod_ops___v1 {
        int (*foo)(void);
    };

    struct bpf_testmod_ops___v2 {
        int (*foo)(void);
        int (*bar)(void);
    };

    /* Assume kernel type name to be 'test_ops' */
    SEC(".struct_ops.link")
    struct test_ops___v1 map_v1 = {
        /* Program 'foo' shared by maps with
         * different local BTF type
         */
        .foo = (void *)foo
    };

    SEC(".struct_ops.link")
    struct test_ops___v2 map_v2 = {
        .foo = (void *)foo,
        .bar = (void *)bar
    };

Assuming the following tweaks are done before loading:

    /* to load v1 */
    bpf_map__set_autocreate(skel->maps.map_v1, true);
    bpf_map__set_autocreate(skel->maps.map_v2, false);

    /* to load v2 */
    bpf_map__set_autocreate(skel->maps.map_v1, false);
    bpf_map__set_autocreate(skel->maps.map_v2, true);

Patch #7 ties autocreate and autoload flags for struct_ops maps and
programs, I'm curious if people find such bundling useful and in line
with other libbpf APIs.

Eduard Zingerman (8):
  libbpf: allow version suffixes (___smth) for struct_ops types
  libbpf: tie struct_ops programs to kernel BTF ids, not to local ids
  libbpf: honor autocreate flag for struct_ops maps
  selftests/bpf: test struct_ops map definition with type suffix
  selftests/bpf: bad_struct_ops test
  selftests/bpf: test autocreate behavior for struct_ops maps
  libbpf: sync progs autoload with maps autocreate for struct_ops maps
  selftests/bpf: tests for struct_ops autoload/autocreate toggling

 tools/lib/bpf/libbpf.c                        | 105 ++++++++++----
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  25 ++++
 .../selftests/bpf/bpf_testmod/bpf_testmod.h   |   4 +
 .../selftests/bpf/prog_tests/bad_struct_ops.c |  42 ++++++
 .../bpf/prog_tests/struct_ops_autocreate.c    | 136 ++++++++++++++++++
 .../bpf/prog_tests/test_struct_ops_module.c   |  32 +++--
 .../selftests/bpf/progs/bad_struct_ops.c      |  17 +++
 .../bpf/progs/struct_ops_autocreate.c         |  42 ++++++
 .../selftests/bpf/progs/struct_ops_module.c   |  21 ++-
 9 files changed, 383 insertions(+), 41 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
 create mode 100644 tools/testing/selftests/bpf/progs/bad_struct_ops.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_autocreate.c

--
2.43.0

[PATCH bpf-next v1 1/8] libbpf: allow version suffixes (___smth) for struct_ops types

[Eduard Zingerman]

E.g. allow the following struct_ops definitions:

    struct bpf_testmod_ops___v1 { int (*test)(void); };
    struct bpf_testmod_ops___v2 { int (*test)(void); };

    SEC(".struct_ops.link")
    struct bpf_testmod_ops___v1 a = { .test = ... }
    SEC(".struct_ops.link")
    struct bpf_testmod_ops___v2 b = { .test = ... }

Where both bpf_testmod_ops__v1 and bpf_testmod_ops__v2 would be
resolved as 'struct bpf_testmod_ops' from kernel BTF.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/lib/bpf/libbpf.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 01f407591a92..abe663927013 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -948,7 +948,7 @@ static int find_btf_by_prefix_kind(const struct btf *btf, const char *prefix,
 				   const char *name, __u32 kind);
 
 static int
-find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
+find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
 			   struct module_btf **mod_btf,
 			   const struct btf_type **type, __u32 *type_id,
 			   const struct btf_type **vtype, __u32 *vtype_id,
@@ -957,15 +957,21 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
 	const struct btf_type *kern_type, *kern_vtype;
 	const struct btf_member *kern_data_member;
 	struct btf *btf;
-	__s32 kern_vtype_id, kern_type_id;
+	__s32 kern_vtype_id, kern_type_id, err;
+	char *tname;
 	__u32 i;
 
+	tname = strndup(tname_raw, bpf_core_essential_name_len(tname_raw));
+	if (!tname)
+		return -ENOMEM;
+
 	kern_type_id = find_ksym_btf_id(obj, tname, BTF_KIND_STRUCT,
 					&btf, mod_btf);
 	if (kern_type_id < 0) {
 		pr_warn("struct_ops init_kern: struct %s is not found in kernel BTF\n",
 			tname);
-		return kern_type_id;
+		err = kern_type_id;
+		goto err_out;
 	}
 	kern_type = btf__type_by_id(btf, kern_type_id);
 
@@ -979,7 +985,8 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
 	if (kern_vtype_id < 0) {
 		pr_warn("struct_ops init_kern: struct %s%s is not found in kernel BTF\n",
 			STRUCT_OPS_VALUE_PREFIX, tname);
-		return kern_vtype_id;
+		err = kern_vtype_id;
+		goto err_out;
 	}
 	kern_vtype = btf__type_by_id(btf, kern_vtype_id);
 
@@ -997,7 +1004,8 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
 	if (i == btf_vlen(kern_vtype)) {
 		pr_warn("struct_ops init_kern: struct %s data is not found in struct %s%s\n",
 			tname, STRUCT_OPS_VALUE_PREFIX, tname);
-		return -EINVAL;
+		err = -EINVAL;
+		goto err_out;
 	}
 
 	*type = kern_type;
@@ -1007,6 +1015,10 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
 	*data_member = kern_data_member;
 
 	return 0;
+
+err_out:
+	free(tname);
+	return err;
 }
 
 static bool bpf_map__is_struct_ops(const struct bpf_map *map)
-- 
2.43.0

Re: [PATCH bpf-next v1 1/8] libbpf: allow version suffixes (___smth) for struct_ops types

[Kui-Feng Lee]

On 2/27/24 12:45, Eduard Zingerman wrote:
> E.g. allow the following struct_ops definitions:
> 
>      struct bpf_testmod_ops___v1 { int (*test)(void); };
>      struct bpf_testmod_ops___v2 { int (*test)(void); };
> 
>      SEC(".struct_ops.link")
>      struct bpf_testmod_ops___v1 a = { .test = ... }
>      SEC(".struct_ops.link")
>      struct bpf_testmod_ops___v2 b = { .test = ... }
> 
> Where both bpf_testmod_ops__v1 and bpf_testmod_ops__v2 would be
> resolved as 'struct bpf_testmod_ops' from kernel BTF.
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>   tools/lib/bpf/libbpf.c | 22 +++++++++++++++++-----
>   1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 01f407591a92..abe663927013 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -948,7 +948,7 @@ static int find_btf_by_prefix_kind(const struct btf *btf, const char *prefix,
>   				   const char *name, __u32 kind);
>   
>   static int
> -find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
> +find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
>   			   struct module_btf **mod_btf,
>   			   const struct btf_type **type, __u32 *type_id,
>   			   const struct btf_type **vtype, __u32 *vtype_id,
> @@ -957,15 +957,21 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>   	const struct btf_type *kern_type, *kern_vtype;
>   	const struct btf_member *kern_data_member;
>   	struct btf *btf;
> -	__s32 kern_vtype_id, kern_type_id;
> +	__s32 kern_vtype_id, kern_type_id, err;
> +	char *tname;
>   	__u32 i;
>   
> +	tname = strndup(tname_raw, bpf_core_essential_name_len(tname_raw));
> +	if (!tname)
> +		return -ENOMEM;
> +
>   	kern_type_id = find_ksym_btf_id(obj, tname, BTF_KIND_STRUCT,
>   					&btf, mod_btf);
>   	if (kern_type_id < 0) {
>   		pr_warn("struct_ops init_kern: struct %s is not found in kernel BTF\n",
>   			tname);
> -		return kern_type_id;
> +		err = kern_type_id;
> +		goto err_out;
>   	}
>   	kern_type = btf__type_by_id(btf, kern_type_id);
>   
> @@ -979,7 +985,8 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>   	if (kern_vtype_id < 0) {
>   		pr_warn("struct_ops init_kern: struct %s%s is not found in kernel BTF\n",
>   			STRUCT_OPS_VALUE_PREFIX, tname);
> -		return kern_vtype_id;
> +		err = kern_vtype_id;
> +		goto err_out;
>   	}
>   	kern_vtype = btf__type_by_id(btf, kern_vtype_id);
>   
> @@ -997,7 +1004,8 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>   	if (i == btf_vlen(kern_vtype)) {
>   		pr_warn("struct_ops init_kern: struct %s data is not found in struct %s%s\n",
>   			tname, STRUCT_OPS_VALUE_PREFIX, tname);
> -		return -EINVAL;
> +		err = -EINVAL;
> +		goto err_out;
>   	}
>   
>   	*type = kern_type;
> @@ -1007,6 +1015,10 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>   	*data_member = kern_data_member;

Where is going to free tname when it successes?

>   
>   	return 0;
> +
> +err_out:
> +	free(tname);
> +	return err;
>   }
>   
>   static bool bpf_map__is_struct_ops(const struct bpf_map *map)

Re: [PATCH bpf-next v1 1/8] libbpf: allow version suffixes (___smth) for struct_ops types

[Eduard Zingerman]

On Tue, 2024-02-27 at 13:47 -0800, Kui-Feng Lee wrote:
[...]

> > @@ -997,7 +1004,8 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
> >   	if (i == btf_vlen(kern_vtype)) {
> >   		pr_warn("struct_ops init_kern: struct %s data is not found in struct %s%s\n",
> >   			tname, STRUCT_OPS_VALUE_PREFIX, tname);
> > -		return -EINVAL;
> > +		err = -EINVAL;
> > +		goto err_out;
> >   	}
> >   
> >   	*type = kern_type;
> > @@ -1007,6 +1015,10 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
> >   	*data_member = kern_data_member;
> 
> Where is going to free tname when it successes?

My bad, thank you for spotting this.

> >   
> >   	return 0;
> > +
> > +err_out:
> > +	free(tname);
> > +	return err;
> >   }
> >   
> >   static bool bpf_map__is_struct_ops(const struct bpf_map *map)

Re: [PATCH bpf-next v1 1/8] libbpf: allow version suffixes (___smth) for struct_ops types

[David Vernet]

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

On Tue, Feb 27, 2024 at 10:45:49PM +0200, Eduard Zingerman wrote:
> E.g. allow the following struct_ops definitions:
> 
>     struct bpf_testmod_ops___v1 { int (*test)(void); };
>     struct bpf_testmod_ops___v2 { int (*test)(void); };
> 
>     SEC(".struct_ops.link")
>     struct bpf_testmod_ops___v1 a = { .test = ... }
>     SEC(".struct_ops.link")
>     struct bpf_testmod_ops___v2 b = { .test = ... }
> 
> Where both bpf_testmod_ops__v1 and bpf_testmod_ops__v2 would be
> resolved as 'struct bpf_testmod_ops' from kernel BTF.
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>

Acked-by: David Vernet <void@manifault.com>

Modulo the leak pointed out by Kui-Feng in another thread. It would be nice if
we could just do this on the stack, but I guess there's no static max size for
a tname.

> ---
>  tools/lib/bpf/libbpf.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 01f407591a92..abe663927013 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -948,7 +948,7 @@ static int find_btf_by_prefix_kind(const struct btf *btf, const char *prefix,
>  				   const char *name, __u32 kind);
>  
>  static int
> -find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
> +find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
>  			   struct module_btf **mod_btf,
>  			   const struct btf_type **type, __u32 *type_id,
>  			   const struct btf_type **vtype, __u32 *vtype_id,
> @@ -957,15 +957,21 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>  	const struct btf_type *kern_type, *kern_vtype;
>  	const struct btf_member *kern_data_member;
>  	struct btf *btf;
> -	__s32 kern_vtype_id, kern_type_id;
> +	__s32 kern_vtype_id, kern_type_id, err;
> +	char *tname;
>  	__u32 i;
>  
> +	tname = strndup(tname_raw, bpf_core_essential_name_len(tname_raw));
> +	if (!tname)
> +		return -ENOMEM;
> +
>  	kern_type_id = find_ksym_btf_id(obj, tname, BTF_KIND_STRUCT,
>  					&btf, mod_btf);
>  	if (kern_type_id < 0) {
>  		pr_warn("struct_ops init_kern: struct %s is not found in kernel BTF\n",
>  			tname);
> -		return kern_type_id;
> +		err = kern_type_id;
> +		goto err_out;
>  	}
>  	kern_type = btf__type_by_id(btf, kern_type_id);
>  
> @@ -979,7 +985,8 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>  	if (kern_vtype_id < 0) {
>  		pr_warn("struct_ops init_kern: struct %s%s is not found in kernel BTF\n",
>  			STRUCT_OPS_VALUE_PREFIX, tname);
> -		return kern_vtype_id;
> +		err = kern_vtype_id;
> +		goto err_out;
>  	}
>  	kern_vtype = btf__type_by_id(btf, kern_vtype_id);
>  
> @@ -997,7 +1004,8 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>  	if (i == btf_vlen(kern_vtype)) {
>  		pr_warn("struct_ops init_kern: struct %s data is not found in struct %s%s\n",
>  			tname, STRUCT_OPS_VALUE_PREFIX, tname);
> -		return -EINVAL;
> +		err = -EINVAL;
> +		goto err_out;
>  	}
>  
>  	*type = kern_type;
> @@ -1007,6 +1015,10 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>  	*data_member = kern_data_member;
>  
>  	return 0;
> +
> +err_out:
> +	free(tname);
> +	return err;
>  }
>  
>  static bool bpf_map__is_struct_ops(const struct bpf_map *map)
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH bpf-next v1 1/8] libbpf: allow version suffixes (___smth) for struct_ops types

[Eduard Zingerman]

On Wed, 2024-02-28 at 10:29 -0600, David Vernet wrote:
[...]

> Modulo the leak pointed out by Kui-Feng in another thread. It would be nice if
> we could just do this on the stack, but I guess there's no static max size for
> a tname.

GCC documents [0] that it does not impose name length limits.
Skimming through libbpf's btf.c it looks like it does not impose limits either.
I can add a name buffer and a fallback to strdup logic if tname is too long,
but I don't think this code would ever be on the hot path.

[0] https://gcc.gnu.org/onlinedocs/gcc/Identifiers-implementation.html#Identifiers-implementation

Re: [PATCH bpf-next v1 1/8] libbpf: allow version suffixes (___smth) for struct_ops types

[David Vernet]

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

On Wed, Feb 28, 2024 at 07:28:49PM +0200, Eduard Zingerman wrote:
> On Wed, 2024-02-28 at 10:29 -0600, David Vernet wrote:
> [...]
> 
> > Modulo the leak pointed out by Kui-Feng in another thread. It would be nice if
> > we could just do this on the stack, but I guess there's no static max size for
> > a tname.
> 
> GCC documents [0] that it does not impose name length limits.
> Skimming through libbpf's btf.c it looks like it does not impose limits either.
> I can add a name buffer and a fallback to strdup logic if tname is too long,
> but I don't think this code would ever be on the hot path.
> 
> [0] https://gcc.gnu.org/onlinedocs/gcc/Identifiers-implementation.html#Identifiers-implementation

Yeah, definitely not worth it. Thanks for looking at the documentation just in
case.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH bpf-next v1 1/8] libbpf: allow version suffixes (___smth) for struct_ops types

[Andrii Nakryiko]

On Wed, Feb 28, 2024 at 9:29 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2024-02-28 at 10:29 -0600, David Vernet wrote:
> [...]
>
> > Modulo the leak pointed out by Kui-Feng in another thread. It would be nice if
> > we could just do this on the stack, but I guess there's no static max size for
> > a tname.
>
> GCC documents [0] that it does not impose name length limits.
> Skimming through libbpf's btf.c it looks like it does not impose limits either.
> I can add a name buffer and a fallback to strdup logic if tname is too long,
> but I don't think this code would ever be on the hot path.

It would still be nice to avoid allocation even if for the sake of
simplifying error handling. I think it's reasonable to have `char
name[256]` on the stack and snprintf() into it. Let's keep it simple.

>
> [0] https://gcc.gnu.org/onlinedocs/gcc/Identifiers-implementation.html#Identifiers-implementation

Re: [PATCH bpf-next v1 1/8] libbpf: allow version suffixes (___smth) for struct_ops types

[Eduard Zingerman]

On Wed, 2024-02-28 at 15:21 -0800, Andrii Nakryiko wrote:

[...]

> It would still be nice to avoid allocation even if for the sake of
> simplifying error handling. I think it's reasonable to have `char
> name[256]` on the stack and snprintf() into it. Let's keep it simple.

Ok
> 

[PATCH bpf-next v1 2/8] libbpf: tie struct_ops programs to kernel BTF ids, not to local ids

[Eduard Zingerman]

Enforce the following existing limitation on struct_ops programs based
on kernel BTF id instead of program-local BTF id:

    struct_ops BPF prog can be re-used between multiple .struct_ops &
    .struct_ops.link as long as it's the same struct_ops struct
    definition and the same function pointer field

This allows reusing same BPF program for versioned struct_ops map
definitions, e.g.:

    SEC("struct_ops/test")
    int BPF_PROG(foo) { ... }

    struct some_ops___v1 { int (*test)(void); };
    struct some_ops___v2 { int (*test)(void); };

    SEC(".struct_ops.link") struct some_ops___v1 a = { .test = foo }
    SEC(".struct_ops.link") struct some_ops___v2 b = { .test = foo }

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/lib/bpf/libbpf.c | 44 ++++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index abe663927013..c239b75d5816 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1134,8 +1134,27 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
 
 			if (mod_btf)
 				prog->attach_btf_obj_fd = mod_btf->fd;
-			prog->attach_btf_id = kern_type_id;
-			prog->expected_attach_type = kern_member_idx;
+
+			/* if we haven't yet processed this BPF program, record proper
+			 * attach_btf_id and member_idx
+			 */
+			if (!prog->attach_btf_id) {
+				prog->attach_btf_id = kern_type_id;
+				prog->expected_attach_type = kern_member_idx;
+			}
+
+			/* struct_ops BPF prog can be re-used between multiple
+			 * .struct_ops & .struct_ops.link as long as it's the
+			 * same struct_ops struct definition and the same
+			 * function pointer field
+			 */
+			if (prog->attach_btf_id != kern_type_id ||
+			    prog->expected_attach_type != kern_member_idx) {
+				pr_warn("struct_ops reloc %s: cannot use prog %s in sec %s with type %u attach_btf_id %u expected_attach_type %u for func ptr %s\n",
+					map->name, prog->name, prog->sec_name, prog->type,
+					prog->attach_btf_id, prog->expected_attach_type, mname);
+				return -EINVAL;
+			}
 
 			st_ops->kern_func_off[i] = kern_data_off + kern_moff;
 
@@ -9409,27 +9428,6 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
 			return -EINVAL;
 		}
 
-		/* if we haven't yet processed this BPF program, record proper
-		 * attach_btf_id and member_idx
-		 */
-		if (!prog->attach_btf_id) {
-			prog->attach_btf_id = st_ops->type_id;
-			prog->expected_attach_type = member_idx;
-		}
-
-		/* struct_ops BPF prog can be re-used between multiple
-		 * .struct_ops & .struct_ops.link as long as it's the
-		 * same struct_ops struct definition and the same
-		 * function pointer field
-		 */
-		if (prog->attach_btf_id != st_ops->type_id ||
-		    prog->expected_attach_type != member_idx) {
-			pr_warn("struct_ops reloc %s: cannot use prog %s in sec %s with type %u attach_btf_id %u expected_attach_type %u for func ptr %s\n",
-				map->name, prog->name, prog->sec_name, prog->type,
-				prog->attach_btf_id, prog->expected_attach_type, name);
-			return -EINVAL;
-		}
-
 		st_ops->progs[member_idx] = prog;
 	}
 
-- 
2.43.0

Re: [PATCH bpf-next v1 2/8] libbpf: tie struct_ops programs to kernel BTF ids, not to local ids

[Martin KaFai Lau]

On 2/27/24 12:45 PM, Eduard Zingerman wrote:
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index abe663927013..c239b75d5816 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1134,8 +1134,27 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>   
>   			if (mod_btf)
>   				prog->attach_btf_obj_fd = mod_btf->fd;
> -			prog->attach_btf_id = kern_type_id;
> -			prog->expected_attach_type = kern_member_idx;
> +
> +			/* if we haven't yet processed this BPF program, record proper
> +			 * attach_btf_id and member_idx
> +			 */
> +			if (!prog->attach_btf_id) {
> +				prog->attach_btf_id = kern_type_id;
> +				prog->expected_attach_type = kern_member_idx;
> +			}
> +
> +			/* struct_ops BPF prog can be re-used between multiple
> +			 * .struct_ops & .struct_ops.link as long as it's the
> +			 * same struct_ops struct definition and the same
> +			 * function pointer field
> +			 */
> +			if (prog->attach_btf_id != kern_type_id ||
> +			    prog->expected_attach_type != kern_member_idx) {
> +				pr_warn("struct_ops reloc %s: cannot use prog %s in sec %s with type %u attach_btf_id %u expected_attach_type %u for func ptr %s\n",

The patch lgtm. A nit is s/reloc/init_kern/.

> +					map->name, prog->name, prog->sec_name, prog->type,
> +					prog->attach_btf_id, prog->expected_attach_type, mname);
> +				return -EINVAL;
> +			}
>   
>   			st_ops->kern_func_off[i] = kern_data_off + kern_moff;

Re: [PATCH bpf-next v1 2/8] libbpf: tie struct_ops programs to kernel BTF ids, not to local ids

[David Vernet]

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

On Tue, Feb 27, 2024 at 10:45:50PM +0200, Eduard Zingerman wrote:
> Enforce the following existing limitation on struct_ops programs based
> on kernel BTF id instead of program-local BTF id:
> 
>     struct_ops BPF prog can be re-used between multiple .struct_ops &
>     .struct_ops.link as long as it's the same struct_ops struct
>     definition and the same function pointer field

Am I correct in understanding the code that the prog also has to be at the same
offset in the new type? So if we have for example:

SEC("struct_ops/test")
int BPF_PROG(foo) { ... }

struct some_ops___v1 {
	int (*test)(void);
};

struct some_ops___v2 {
	int (*init)(void);
	int (*test)(void);
};

Then this wouldn't work? If so, would it be possible for libbpf to do something
like invisibly duplicate the prog and create a separate one for each struct_ops
map where it's encountered? It feels like a rather awkward restriction to
impose given that the idea behind the feature is to enable loading one of
multiple possible definitions of a struct_ops type.

> 
> This allows reusing same BPF program for versioned struct_ops map
> definitions, e.g.:
> 
>     SEC("struct_ops/test")
>     int BPF_PROG(foo) { ... }
> 
>     struct some_ops___v1 { int (*test)(void); };
>     struct some_ops___v2 { int (*test)(void); };
> 
>     SEC(".struct_ops.link") struct some_ops___v1 a = { .test = foo }
>     SEC(".struct_ops.link") struct some_ops___v2 b = { .test = foo }
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  tools/lib/bpf/libbpf.c | 44 ++++++++++++++++++++----------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index abe663927013..c239b75d5816 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1134,8 +1134,27 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>  
>  			if (mod_btf)
>  				prog->attach_btf_obj_fd = mod_btf->fd;
> -			prog->attach_btf_id = kern_type_id;
> -			prog->expected_attach_type = kern_member_idx;
> +
> +			/* if we haven't yet processed this BPF program, record proper
> +			 * attach_btf_id and member_idx
> +			 */
> +			if (!prog->attach_btf_id) {
> +				prog->attach_btf_id = kern_type_id;
> +				prog->expected_attach_type = kern_member_idx;
> +			}
> +
> +			/* struct_ops BPF prog can be re-used between multiple
> +			 * .struct_ops & .struct_ops.link as long as it's the
> +			 * same struct_ops struct definition and the same
> +			 * function pointer field
> +			 */
> +			if (prog->attach_btf_id != kern_type_id ||
> +			    prog->expected_attach_type != kern_member_idx) {
> +				pr_warn("struct_ops reloc %s: cannot use prog %s in sec %s with type %u attach_btf_id %u expected_attach_type %u for func ptr %s\n",
> +					map->name, prog->name, prog->sec_name, prog->type,
> +					prog->attach_btf_id, prog->expected_attach_type, mname);
> +				return -EINVAL;
> +			}
>  
>  			st_ops->kern_func_off[i] = kern_data_off + kern_moff;
>  
> @@ -9409,27 +9428,6 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
>  			return -EINVAL;
>  		}
>  
> -		/* if we haven't yet processed this BPF program, record proper
> -		 * attach_btf_id and member_idx
> -		 */
> -		if (!prog->attach_btf_id) {
> -			prog->attach_btf_id = st_ops->type_id;
> -			prog->expected_attach_type = member_idx;
> -		}
> -
> -		/* struct_ops BPF prog can be re-used between multiple
> -		 * .struct_ops & .struct_ops.link as long as it's the
> -		 * same struct_ops struct definition and the same
> -		 * function pointer field
> -		 */
> -		if (prog->attach_btf_id != st_ops->type_id ||
> -		    prog->expected_attach_type != member_idx) {
> -			pr_warn("struct_ops reloc %s: cannot use prog %s in sec %s with type %u attach_btf_id %u expected_attach_type %u for func ptr %s\n",
> -				map->name, prog->name, prog->sec_name, prog->type,
> -				prog->attach_btf_id, prog->expected_attach_type, name);
> -			return -EINVAL;
> -		}
> -
>  		st_ops->progs[member_idx] = prog;
>  	}
>  
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH bpf-next v1 2/8] libbpf: tie struct_ops programs to kernel BTF ids, not to local ids

[Eduard Zingerman]

On Wed, 2024-02-28 at 11:23 -0600, David Vernet wrote:
> On Tue, Feb 27, 2024 at 10:45:50PM +0200, Eduard Zingerman wrote:
> > Enforce the following existing limitation on struct_ops programs based
> > on kernel BTF id instead of program-local BTF id:
> > 
> >     struct_ops BPF prog can be re-used between multiple .struct_ops &
> >     .struct_ops.link as long as it's the same struct_ops struct
> >     definition and the same function pointer field
> 
> Am I correct in understanding the code that the prog also has to be at the same
> offset in the new type?

Yes, but after this patch it would be offset in current kernel BTF type,
not local BTF type.

> So if we have for example:
> 
> SEC("struct_ops/test")
> int BPF_PROG(foo) { ... }
> 
> struct some_ops___v1 {
> 	int (*test)(void);
> };
> 
> struct some_ops___v2 {
> 	int (*init)(void);
> 	int (*test)(void);
> };

From pov of kernel BTF there is only one 'struct some_ops'.
 
> Then this wouldn't work? If so, would it be possible for libbpf to do something
> like invisibly duplicate the prog and create a separate one for each struct_ops
> map where it's encountered? It feels like a rather awkward restriction to
> impose given that the idea behind the feature is to enable loading one of
> multiple possible definitions of a struct_ops type. 

In combination with the next patch, the idea is to not assign offset
in struct_ops maps which have autocreate == false.

If object corresponding to program above would be opened and
autocreate would be disabled either for some_ops___v1 or some_ops___v2
before load, the program 'test' would get it's offset entry only from
one map. Thus no program duplication would be necessary.

For example, see test case in patch #6:

    struct bpf_testmod_ops___v1 {
    	int (*test_1)(void);
    };

    struct bpf_testmod_ops___v2 {
    	int (*test_1)(void);
    	int (*does_not_exist)(void);
    };

    SEC(".struct_ops.link")
    struct bpf_testmod_ops___v1 testmod_1 = {
    	.test_1 = (void *)test_1
    };

    SEC(".struct_ops.link")
    struct bpf_testmod_ops___v2 testmod_2 = {
    	.test_1 = (void *)test_1,
    	.does_not_exist = (void *)test_2
    };


static void can_load_partial_object(void)
{
	...
	skel = struct_ops_autocreate__open_opts(&opts);
	bpf_program__set_autoload(skel->progs.test_2, false);
	bpf_map__set_autocreate(skel->maps.testmod_2, false);
	struct_ops_autocreate__load(skel);
        ...
}

This should handle your example as well.
Do you find this sufficient or would you still like to have implicit
program duplication logic?

Re: [PATCH bpf-next v1 2/8] libbpf: tie struct_ops programs to kernel BTF ids, not to local ids

[David Vernet]

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

On Wed, Feb 28, 2024 at 07:40:33PM +0200, Eduard Zingerman wrote:
> On Wed, 2024-02-28 at 11:23 -0600, David Vernet wrote:
> > On Tue, Feb 27, 2024 at 10:45:50PM +0200, Eduard Zingerman wrote:
> > > Enforce the following existing limitation on struct_ops programs based
> > > on kernel BTF id instead of program-local BTF id:
> > > 
> > >     struct_ops BPF prog can be re-used between multiple .struct_ops &
> > >     .struct_ops.link as long as it's the same struct_ops struct
> > >     definition and the same function pointer field
> > 
> > Am I correct in understanding the code that the prog also has to be at the same
> > offset in the new type?
> 
> Yes, but after this patch it would be offset in current kernel BTF type,
> not local BTF type.

Yes, indeed. I didn't mean to imply that your patch was changing that. I was
asking more generally -- sorry for the confusion.

> > So if we have for example:
> > 
> > SEC("struct_ops/test")
> > int BPF_PROG(foo) { ... }
> > 
> > struct some_ops___v1 {
> > 	int (*test)(void);
> > };
> > 
> > struct some_ops___v2 {
> > 	int (*init)(void);
> > 	int (*test)(void);
> > };
> 
> From pov of kernel BTF there is only one 'struct some_ops'.

Ack

> > Then this wouldn't work? If so, would it be possible for libbpf to do something
> > like invisibly duplicate the prog and create a separate one for each struct_ops
> > map where it's encountered? It feels like a rather awkward restriction to
> > impose given that the idea behind the feature is to enable loading one of
> > multiple possible definitions of a struct_ops type. 
> 
> In combination with the next patch, the idea is to not assign offset
> in struct_ops maps which have autocreate == false.
> 
> If object corresponding to program above would be opened and
> autocreate would be disabled either for some_ops___v1 or some_ops___v2
> before load, the program 'test' would get it's offset entry only from
> one map. Thus no program duplication would be necessary.
> 
> For example, see test case in patch #6:
> 
>     struct bpf_testmod_ops___v1 {
>     	int (*test_1)(void);
>     };
> 
>     struct bpf_testmod_ops___v2 {
>     	int (*test_1)(void);
>     	int (*does_not_exist)(void);
>     };
> 
>     SEC(".struct_ops.link")
>     struct bpf_testmod_ops___v1 testmod_1 = {
>     	.test_1 = (void *)test_1
>     };
> 
>     SEC(".struct_ops.link")
>     struct bpf_testmod_ops___v2 testmod_2 = {
>     	.test_1 = (void *)test_1,
>     	.does_not_exist = (void *)test_2
>     };
> 
> 
> static void can_load_partial_object(void)
> {
> 	...
> 	skel = struct_ops_autocreate__open_opts(&opts);
> 	bpf_program__set_autoload(skel->progs.test_2, false);
> 	bpf_map__set_autocreate(skel->maps.testmod_2, false);
> 	struct_ops_autocreate__load(skel);
>         ...
> }
> 
> This should handle your example as well.
> Do you find this sufficient or would you still like to have implicit
> program duplication logic?

It's definitely fine for now, but IMO it's something to keep in mind for the
future as a usability improvement. Ideally libbpf could internally handle just
creating and loading the type that's actually present on the system, and handle
applying the prog to the correct map, etc on the caller's behalf. Given that
there's only ever a single instance of a struct_ops type on the system, this
seems like a reasonable feature for the library to provide.

Note that this doesn't necessarily require duplicating the prog either, if
libbpf can instead _deduplicate_ the struct_ops maps to only create and load
the one that matches the type on the system.

Not a blocker by any means, but possibly a nice to have down the road.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH bpf-next v1 2/8] libbpf: tie struct_ops programs to kernel BTF ids, not to local ids

[Andrii Nakryiko]

On Tue, Feb 27, 2024 at 12:46 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Enforce the following existing limitation on struct_ops programs based
> on kernel BTF id instead of program-local BTF id:
>
>     struct_ops BPF prog can be re-used between multiple .struct_ops &
>     .struct_ops.link as long as it's the same struct_ops struct
>     definition and the same function pointer field
>
> This allows reusing same BPF program for versioned struct_ops map
> definitions, e.g.:
>
>     SEC("struct_ops/test")
>     int BPF_PROG(foo) { ... }
>
>     struct some_ops___v1 { int (*test)(void); };
>     struct some_ops___v2 { int (*test)(void); };
>
>     SEC(".struct_ops.link") struct some_ops___v1 a = { .test = foo }
>     SEC(".struct_ops.link") struct some_ops___v2 b = { .test = foo }
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  tools/lib/bpf/libbpf.c | 44 ++++++++++++++++++++----------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index abe663927013..c239b75d5816 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1134,8 +1134,27 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>
>                         if (mod_btf)
>                                 prog->attach_btf_obj_fd = mod_btf->fd;
> -                       prog->attach_btf_id = kern_type_id;
> -                       prog->expected_attach_type = kern_member_idx;
> +
> +                       /* if we haven't yet processed this BPF program, record proper
> +                        * attach_btf_id and member_idx
> +                        */
> +                       if (!prog->attach_btf_id) {
> +                               prog->attach_btf_id = kern_type_id;
> +                               prog->expected_attach_type = kern_member_idx;
> +                       }
> +
> +                       /* struct_ops BPF prog can be re-used between multiple
> +                        * .struct_ops & .struct_ops.link as long as it's the
> +                        * same struct_ops struct definition and the same
> +                        * function pointer field
> +                        */
> +                       if (prog->attach_btf_id != kern_type_id ||
> +                           prog->expected_attach_type != kern_member_idx) {
> +                               pr_warn("struct_ops reloc %s: cannot use prog %s in sec %s with type %u attach_btf_id %u expected_attach_type %u for func ptr %s\n",

Martin already pointed out s/reloc/init_kern/, but I also find "cannot
use prog" a bit too unactionable. Maybe "invalid reuse of prog"?
"reuse" is the key here to point out that this program is used at
least twice, and that in some incompatible way?

> +                                       map->name, prog->name, prog->sec_name, prog->type,
> +                                       prog->attach_btf_id, prog->expected_attach_type, mname);
> +                               return -EINVAL;
> +                       }
>
>                         st_ops->kern_func_off[i] = kern_data_off + kern_moff;
>
> @@ -9409,27 +9428,6 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
>                         return -EINVAL;
>                 }
>
> -               /* if we haven't yet processed this BPF program, record proper
> -                * attach_btf_id and member_idx
> -                */
> -               if (!prog->attach_btf_id) {
> -                       prog->attach_btf_id = st_ops->type_id;
> -                       prog->expected_attach_type = member_idx;
> -               }
> -
> -               /* struct_ops BPF prog can be re-used between multiple
> -                * .struct_ops & .struct_ops.link as long as it's the
> -                * same struct_ops struct definition and the same
> -                * function pointer field
> -                */
> -               if (prog->attach_btf_id != st_ops->type_id ||
> -                   prog->expected_attach_type != member_idx) {
> -                       pr_warn("struct_ops reloc %s: cannot use prog %s in sec %s with type %u attach_btf_id %u expected_attach_type %u for func ptr %s\n",
> -                               map->name, prog->name, prog->sec_name, prog->type,
> -                               prog->attach_btf_id, prog->expected_attach_type, name);
> -                       return -EINVAL;
> -               }
> -
>                 st_ops->progs[member_idx] = prog;
>         }
>
> --
> 2.43.0
>

Re: [PATCH bpf-next v1 2/8] libbpf: tie struct_ops programs to kernel BTF ids, not to local ids

[Eduard Zingerman]

On Wed, 2024-02-28 at 15:28 -0800, Andrii Nakryiko wrote:
[...]

> > @@ -1134,8 +1134,27 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
> > 
> >                         if (mod_btf)
> >                                 prog->attach_btf_obj_fd = mod_btf->fd;
> > -                       prog->attach_btf_id = kern_type_id;
> > -                       prog->expected_attach_type = kern_member_idx;
> > +
> > +                       /* if we haven't yet processed this BPF program, record proper
> > +                        * attach_btf_id and member_idx
> > +                        */
> > +                       if (!prog->attach_btf_id) {
> > +                               prog->attach_btf_id = kern_type_id;
> > +                               prog->expected_attach_type = kern_member_idx;
> > +                       }
> > +
> > +                       /* struct_ops BPF prog can be re-used between multiple
> > +                        * .struct_ops & .struct_ops.link as long as it's the
> > +                        * same struct_ops struct definition and the same
> > +                        * function pointer field
> > +                        */
> > +                       if (prog->attach_btf_id != kern_type_id ||
> > +                           prog->expected_attach_type != kern_member_idx) {
> > +                               pr_warn("struct_ops reloc %s: cannot use prog %s in sec %s with type %u attach_btf_id %u expected_attach_type %u for func ptr %s\n",
> 
> Martin already pointed out s/reloc/init_kern/, but I also find "cannot
> use prog" a bit too unactionable. Maybe "invalid reuse of prog"?
> "reuse" is the key here to point out that this program is used at
> least twice, and that in some incompatible way?

Ok, I'll change the wording.
And split the line despite the coding standards.

Re: [PATCH bpf-next v1 2/8] libbpf: tie struct_ops programs to kernel BTF ids, not to local ids

[Andrii Nakryiko]

On Wed, Feb 28, 2024 at 3:31 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2024-02-28 at 15:28 -0800, Andrii Nakryiko wrote:
> [...]
>
> > > @@ -1134,8 +1134,27 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
> > >
> > >                         if (mod_btf)
> > >                                 prog->attach_btf_obj_fd = mod_btf->fd;
> > > -                       prog->attach_btf_id = kern_type_id;
> > > -                       prog->expected_attach_type = kern_member_idx;
> > > +
> > > +                       /* if we haven't yet processed this BPF program, record proper
> > > +                        * attach_btf_id and member_idx
> > > +                        */
> > > +                       if (!prog->attach_btf_id) {
> > > +                               prog->attach_btf_id = kern_type_id;
> > > +                               prog->expected_attach_type = kern_member_idx;
> > > +                       }
> > > +
> > > +                       /* struct_ops BPF prog can be re-used between multiple
> > > +                        * .struct_ops & .struct_ops.link as long as it's the
> > > +                        * same struct_ops struct definition and the same
> > > +                        * function pointer field
> > > +                        */
> > > +                       if (prog->attach_btf_id != kern_type_id ||
> > > +                           prog->expected_attach_type != kern_member_idx) {
> > > +                               pr_warn("struct_ops reloc %s: cannot use prog %s in sec %s with type %u attach_btf_id %u expected_attach_type %u for func ptr %s\n",
> >
> > Martin already pointed out s/reloc/init_kern/, but I also find "cannot
> > use prog" a bit too unactionable. Maybe "invalid reuse of prog"?
> > "reuse" is the key here to point out that this program is used at
> > least twice, and that in some incompatible way?
>
> Ok, I'll change the wording.
> And split the line despite the coding standards.

please don't split format strings, it makes grepping harder

[PATCH bpf-next v1 3/8] libbpf: honor autocreate flag for struct_ops maps

[Eduard Zingerman]

Skip load steps for struct_ops maps not marked for automatic creation.
This should allow to load bpf object in situations like below:

    SEC("struct_ops/foo") int BPF_PROG(foo) { ... }
    SEC("struct_ops/bar") int BPF_PROG(bar) { ... }

    struct test_ops___v1 {
    	int (*foo)(void);
    };

    struct test_ops___v2 {
    	int (*foo)(void);
    	int (*does_not_exist)(void);
    };

    SEC(".struct_ops.link")
    struct test_ops___v1 map_for_old = {
    	.test_1 = (void *)foo
    };

    SEC(".struct_ops.link")
    struct test_ops___v2 map_for_new = {
    	.test_1 = (void *)foo,
    	.does_not_exist = (void *)bar
    };

Suppose program is loaded on old kernel that does not have definition
for 'does_not_exist' struct_ops member. After this commit it would be
possible to load such object file after the following tweaks:

    bpf_program__set_autoload(skel->progs.bar, false);
    bpf_map__set_autocreate(skel->maps.map_for_new, false);

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/lib/bpf/libbpf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index c239b75d5816..b39d3f2898a1 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1192,7 +1192,7 @@ static int bpf_object__init_kern_struct_ops_maps(struct bpf_object *obj)
 	for (i = 0; i < obj->nr_maps; i++) {
 		map = &obj->maps[i];
 
-		if (!bpf_map__is_struct_ops(map))
+		if (!bpf_map__is_struct_ops(map) || !map->autocreate)
 			continue;
 
 		err = bpf_map__init_kern_struct_ops(map);
@@ -8114,7 +8114,7 @@ static int bpf_object_prepare_struct_ops(struct bpf_object *obj)
 	int i;
 
 	for (i = 0; i < obj->nr_maps; i++)
-		if (bpf_map__is_struct_ops(&obj->maps[i]))
+		if (bpf_map__is_struct_ops(&obj->maps[i]) && obj->maps[i].autocreate)
 			bpf_map_prepare_vdata(&obj->maps[i]);
 
 	return 0;
-- 
2.43.0

Re: [PATCH bpf-next v1 3/8] libbpf: honor autocreate flag for struct_ops maps

[David Vernet]

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

On Tue, Feb 27, 2024 at 10:45:51PM +0200, Eduard Zingerman wrote:
> Skip load steps for struct_ops maps not marked for automatic creation.
> This should allow to load bpf object in situations like below:
> 
>     SEC("struct_ops/foo") int BPF_PROG(foo) { ... }
>     SEC("struct_ops/bar") int BPF_PROG(bar) { ... }
> 
>     struct test_ops___v1 {
>     	int (*foo)(void);
>     };
> 
>     struct test_ops___v2 {
>     	int (*foo)(void);
>     	int (*does_not_exist)(void);
>     };
> 
>     SEC(".struct_ops.link")
>     struct test_ops___v1 map_for_old = {
>     	.test_1 = (void *)foo
>     };
> 
>     SEC(".struct_ops.link")
>     struct test_ops___v2 map_for_new = {
>     	.test_1 = (void *)foo,
>     	.does_not_exist = (void *)bar
>     };
> 
> Suppose program is loaded on old kernel that does not have definition
> for 'does_not_exist' struct_ops member. After this commit it would be
> possible to load such object file after the following tweaks:
> 
>     bpf_program__set_autoload(skel->progs.bar, false);
>     bpf_map__set_autocreate(skel->maps.map_for_new, false);
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>

Is this technically a bug fix? We were already skipping creating the map in
bpf_object__create_maps(), so initializing the struct_ops map even when
autocreate isn't set just seems like an oversight.

Either way:

Acked-by: David Vernet <void@manifault.com>

> ---
>  tools/lib/bpf/libbpf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index c239b75d5816..b39d3f2898a1 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1192,7 +1192,7 @@ static int bpf_object__init_kern_struct_ops_maps(struct bpf_object *obj)
>  	for (i = 0; i < obj->nr_maps; i++) {
>  		map = &obj->maps[i];
>  
> -		if (!bpf_map__is_struct_ops(map))
> +		if (!bpf_map__is_struct_ops(map) || !map->autocreate)
>  			continue;
>  
>  		err = bpf_map__init_kern_struct_ops(map);
> @@ -8114,7 +8114,7 @@ static int bpf_object_prepare_struct_ops(struct bpf_object *obj)
>  	int i;
>  
>  	for (i = 0; i < obj->nr_maps; i++)
> -		if (bpf_map__is_struct_ops(&obj->maps[i]))
> +		if (bpf_map__is_struct_ops(&obj->maps[i]) && obj->maps[i].autocreate)
>  			bpf_map_prepare_vdata(&obj->maps[i]);
>  
>  	return 0;
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH bpf-next v1 4/8] selftests/bpf: test struct_ops map definition with type suffix

[Eduard Zingerman]

Extend struct_ops_module test case to check if it is possible to use
'___' suffixes for struct_ops type specification.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  1 +
 .../bpf/prog_tests/test_struct_ops_module.c   | 32 +++++++++++++------
 .../selftests/bpf/progs/struct_ops_module.c   | 21 +++++++++++-
 3 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 66787e99ba1b..0d8437e05f64 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -555,6 +555,7 @@ static int bpf_dummy_reg(void *kdata)
 {
 	struct bpf_testmod_ops *ops = kdata;
 
+	ops->test_1();
 	/* Some test cases (ex. struct_ops_maybe_null) may not have test_2
 	 * initialized, so we need to check for NULL.
 	 */
diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
index 8d833f0c7580..7bc80d2755f1 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
@@ -30,12 +30,30 @@ static void check_map_info(struct bpf_map_info *info)
 	close(fd);
 }
 
+static int attach_ops_and_check(struct struct_ops_module *skel,
+				struct bpf_map *map,
+				int expected_test_2_result)
+{
+	struct bpf_link *link;
+
+	link = bpf_map__attach_struct_ops(map);
+	ASSERT_OK_PTR(link, "attach_test_mod_1");
+	if (!link)
+		return -1;
+
+	/* test_{1,2}() would be called from bpf_dummy_reg() in bpf_testmod.c */
+	ASSERT_EQ(skel->bss->test_1_result, 0xdeadbeef, "test_1_result");
+	ASSERT_EQ(skel->bss->test_2_result, expected_test_2_result, "test_2_result");
+
+	bpf_link__destroy(link);
+	return 0;
+}
+
 static void test_struct_ops_load(void)
 {
 	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
 	struct struct_ops_module *skel;
 	struct bpf_map_info info = {};
-	struct bpf_link *link;
 	int err;
 	u32 len;
 
@@ -53,15 +71,11 @@ static void test_struct_ops_load(void)
 	if (!ASSERT_OK(err, "bpf_map_get_info_by_fd"))
 		goto cleanup;
 
-	link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
-	ASSERT_OK_PTR(link, "attach_test_mod_1");
-
-	/* test_2() will be called from bpf_dummy_reg() in bpf_testmod.c */
-	ASSERT_EQ(skel->bss->test_2_result, 7, "test_2_result");
-
-	bpf_link__destroy(link);
-
 	check_map_info(&info);
+	if (!attach_ops_and_check(skel, skel->maps.testmod_1, 7))
+		goto cleanup;
+	if (!attach_ops_and_check(skel, skel->maps.testmod_2, 12))
+		goto cleanup;
 
 cleanup:
 	struct_ops_module__destroy(skel);
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_module.c b/tools/testing/selftests/bpf/progs/struct_ops_module.c
index b78746b3cef3..e91426dc51af 100644
--- a/tools/testing/selftests/bpf/progs/struct_ops_module.c
+++ b/tools/testing/selftests/bpf/progs/struct_ops_module.c
@@ -7,12 +7,14 @@
 
 char _license[] SEC("license") = "GPL";
 
+int test_1_result = 0;
 int test_2_result = 0;
 
 SEC("struct_ops/test_1")
 int BPF_PROG(test_1)
 {
-	return 0xdeadbeef;
+	test_1_result = 0xdeadbeef;
+	return 0;
 }
 
 SEC("struct_ops/test_2")
@@ -27,3 +29,20 @@ struct bpf_testmod_ops testmod_1 = {
 	.test_2 = (void *)test_2,
 };
 
+SEC("struct_ops/test_2")
+void BPF_PROG(test_2_v2, int a, int b)
+{
+	test_2_result = a * b;
+}
+
+struct bpf_testmod_ops___v2 {
+	int (*test_1)(void);
+	void (*test_2)(int a, int b);
+	int (*test_maybe_null)(int dummy, struct task_struct *task);
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops___v2 testmod_2 = {
+	.test_1 = (void *)test_1,
+	.test_2 = (void *)test_2_v2,
+};
-- 
2.43.0

Re: [PATCH bpf-next v1 4/8] selftests/bpf: test struct_ops map definition with type suffix

[David Vernet]

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

On Tue, Feb 27, 2024 at 10:45:52PM +0200, Eduard Zingerman wrote:
> Extend struct_ops_module test case to check if it is possible to use
> '___' suffixes for struct_ops type specification.
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>

Acked-by: David Vernet <void@manifault.com>

> ---
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  1 +
>  .../bpf/prog_tests/test_struct_ops_module.c   | 32 +++++++++++++------
>  .../selftests/bpf/progs/struct_ops_module.c   | 21 +++++++++++-
>  3 files changed, 44 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index 66787e99ba1b..0d8437e05f64 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -555,6 +555,7 @@ static int bpf_dummy_reg(void *kdata)
>  {
>  	struct bpf_testmod_ops *ops = kdata;
>  
> +	ops->test_1();
>  	/* Some test cases (ex. struct_ops_maybe_null) may not have test_2
>  	 * initialized, so we need to check for NULL.
>  	 */
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> index 8d833f0c7580..7bc80d2755f1 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> @@ -30,12 +30,30 @@ static void check_map_info(struct bpf_map_info *info)
>  	close(fd);
>  }
>  
> +static int attach_ops_and_check(struct struct_ops_module *skel,
> +				struct bpf_map *map,
> +				int expected_test_2_result)
> +{
> +	struct bpf_link *link;
> +
> +	link = bpf_map__attach_struct_ops(map);
> +	ASSERT_OK_PTR(link, "attach_test_mod_1");
> +	if (!link)
> +		return -1;
> +
> +	/* test_{1,2}() would be called from bpf_dummy_reg() in bpf_testmod.c */
> +	ASSERT_EQ(skel->bss->test_1_result, 0xdeadbeef, "test_1_result");
> +	ASSERT_EQ(skel->bss->test_2_result, expected_test_2_result, "test_2_result");
> +
> +	bpf_link__destroy(link);
> +	return 0;
> +}
> +
>  static void test_struct_ops_load(void)
>  {
>  	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
>  	struct struct_ops_module *skel;
>  	struct bpf_map_info info = {};
> -	struct bpf_link *link;
>  	int err;
>  	u32 len;
>  
> @@ -53,15 +71,11 @@ static void test_struct_ops_load(void)
>  	if (!ASSERT_OK(err, "bpf_map_get_info_by_fd"))
>  		goto cleanup;
>  
> -	link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
> -	ASSERT_OK_PTR(link, "attach_test_mod_1");
> -
> -	/* test_2() will be called from bpf_dummy_reg() in bpf_testmod.c */
> -	ASSERT_EQ(skel->bss->test_2_result, 7, "test_2_result");
> -
> -	bpf_link__destroy(link);
> -
>  	check_map_info(&info);
> +	if (!attach_ops_and_check(skel, skel->maps.testmod_1, 7))
> +		goto cleanup;
> +	if (!attach_ops_and_check(skel, skel->maps.testmod_2, 12))
> +		goto cleanup;
>  
>  cleanup:
>  	struct_ops_module__destroy(skel);
> diff --git a/tools/testing/selftests/bpf/progs/struct_ops_module.c b/tools/testing/selftests/bpf/progs/struct_ops_module.c
> index b78746b3cef3..e91426dc51af 100644
> --- a/tools/testing/selftests/bpf/progs/struct_ops_module.c
> +++ b/tools/testing/selftests/bpf/progs/struct_ops_module.c
> @@ -7,12 +7,14 @@
>  
>  char _license[] SEC("license") = "GPL";
>  
> +int test_1_result = 0;
>  int test_2_result = 0;
>  
>  SEC("struct_ops/test_1")
>  int BPF_PROG(test_1)
>  {
> -	return 0xdeadbeef;
> +	test_1_result = 0xdeadbeef;
> +	return 0;
>  }
>  
>  SEC("struct_ops/test_2")
> @@ -27,3 +29,20 @@ struct bpf_testmod_ops testmod_1 = {
>  	.test_2 = (void *)test_2,
>  };
>  
> +SEC("struct_ops/test_2")
> +void BPF_PROG(test_2_v2, int a, int b)
> +{
> +	test_2_result = a * b;
> +}
> +
> +struct bpf_testmod_ops___v2 {
> +	int (*test_1)(void);
> +	void (*test_2)(int a, int b);
> +	int (*test_maybe_null)(int dummy, struct task_struct *task);
> +};
> +
> +SEC(".struct_ops.link")
> +struct bpf_testmod_ops___v2 testmod_2 = {
> +	.test_1 = (void *)test_1,
> +	.test_2 = (void *)test_2_v2,
> +};
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH bpf-next v1 5/8] selftests/bpf: bad_struct_ops test

[Eduard Zingerman]

When loading struct_ops programs kernel requires BTF id of the
struct_ops type and member index for attachment point inside that
type. This makes it not possible to have same BPF program used in
struct_ops maps that have different struct_ops type.
Check if libbpf rejects such BPF objects files.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 24 +++++++++++
 .../selftests/bpf/bpf_testmod/bpf_testmod.h   |  4 ++
 .../selftests/bpf/prog_tests/bad_struct_ops.c | 42 +++++++++++++++++++
 .../selftests/bpf/progs/bad_struct_ops.c      | 17 ++++++++
 4 files changed, 87 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
 create mode 100644 tools/testing/selftests/bpf/progs/bad_struct_ops.c

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 0d8437e05f64..69f5eb9ad546 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -601,6 +601,29 @@ struct bpf_struct_ops bpf_bpf_testmod_ops = {
 	.owner = THIS_MODULE,
 };
 
+static int bpf_dummy_reg2(void *kdata)
+{
+	struct bpf_testmod_ops2 *ops = kdata;
+
+	ops->test_1();
+	return 0;
+}
+
+static struct bpf_testmod_ops2 __bpf_testmod_ops2 = {
+	.test_1 = bpf_testmod_test_1,
+};
+
+struct bpf_struct_ops bpf_testmod_ops2 = {
+	.verifier_ops = &bpf_testmod_verifier_ops,
+	.init = bpf_testmod_ops_init,
+	.init_member = bpf_testmod_ops_init_member,
+	.reg = bpf_dummy_reg2,
+	.unreg = bpf_dummy_unreg,
+	.cfi_stubs = &__bpf_testmod_ops2,
+	.name = "bpf_testmod_ops2",
+	.owner = THIS_MODULE,
+};
+
 extern int bpf_fentry_test1(int a);
 
 static int bpf_testmod_init(void)
@@ -612,6 +635,7 @@ static int bpf_testmod_init(void)
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_testmod_kfunc_set);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_testmod_kfunc_set);
 	ret = ret ?: register_bpf_struct_ops(&bpf_bpf_testmod_ops, bpf_testmod_ops);
+	ret = ret ?: register_bpf_struct_ops(&bpf_testmod_ops2, bpf_testmod_ops2);
 	if (ret < 0)
 		return ret;
 	if (bpf_fentry_test1(0) < 0)
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
index c3b0cf788f9f..3183fff7f246 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
@@ -37,4 +37,8 @@ struct bpf_testmod_ops {
 	int (*test_maybe_null)(int dummy, struct task_struct *task);
 };
 
+struct bpf_testmod_ops2 {
+	int (*test_1)(void);
+};
+
 #endif /* _BPF_TESTMOD_H */
diff --git a/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c b/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
new file mode 100644
index 000000000000..9c689db4b05b
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+#include "bad_struct_ops.skel.h"
+
+#define EXPECTED_MSG "libbpf: struct_ops reloc"
+
+static libbpf_print_fn_t old_print_cb;
+static bool msg_found;
+
+static int print_cb(enum libbpf_print_level level, const char *fmt, va_list args)
+{
+	old_print_cb(level, fmt, args);
+	if (level == LIBBPF_WARN && strncmp(fmt, EXPECTED_MSG, strlen(EXPECTED_MSG)) == 0)
+		msg_found = true;
+
+	return 0;
+}
+
+static void test_bad_struct_ops(void)
+{
+	struct bad_struct_ops *skel;
+	int err;
+
+	old_print_cb = libbpf_set_print(print_cb);
+	skel = bad_struct_ops__open_and_load();
+	err = errno;
+	libbpf_set_print(old_print_cb);
+	if (!ASSERT_NULL(skel, "bad_struct_ops__open_and_load"))
+		return;
+
+	ASSERT_EQ(err, EINVAL, "errno should be EINVAL");
+	ASSERT_TRUE(msg_found, "expected message");
+
+	bad_struct_ops__destroy(skel);
+}
+
+void serial_test_bad_struct_ops(void)
+{
+	if (test__start_subtest("test_bad_struct_ops"))
+		test_bad_struct_ops();
+}
diff --git a/tools/testing/selftests/bpf/progs/bad_struct_ops.c b/tools/testing/selftests/bpf/progs/bad_struct_ops.c
new file mode 100644
index 000000000000..9c103afbfdb1
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bad_struct_ops.c
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "../bpf_testmod/bpf_testmod.h"
+
+char _license[] SEC("license") = "GPL";
+
+SEC("struct_ops/test_1")
+int BPF_PROG(test_1) { return 0; }
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops testmod_1 = { .test_1 = (void *)test_1 };
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops2 testmod_2 = { .test_1 = (void *)test_1 };
-- 
2.43.0

Re: [PATCH bpf-next v1 5/8] selftests/bpf: bad_struct_ops test

[David Vernet]

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

On Tue, Feb 27, 2024 at 10:45:53PM +0200, Eduard Zingerman wrote:
> When loading struct_ops programs kernel requires BTF id of the
> struct_ops type and member index for attachment point inside that
> type. This makes it not possible to have same BPF program used in
> struct_ops maps that have different struct_ops type.
> Check if libbpf rejects such BPF objects files.
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 24 +++++++++++
>  .../selftests/bpf/bpf_testmod/bpf_testmod.h   |  4 ++
>  .../selftests/bpf/prog_tests/bad_struct_ops.c | 42 +++++++++++++++++++
>  .../selftests/bpf/progs/bad_struct_ops.c      | 17 ++++++++
>  4 files changed, 87 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
>  create mode 100644 tools/testing/selftests/bpf/progs/bad_struct_ops.c
> 
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index 0d8437e05f64..69f5eb9ad546 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -601,6 +601,29 @@ struct bpf_struct_ops bpf_bpf_testmod_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> +static int bpf_dummy_reg2(void *kdata)
> +{
> +	struct bpf_testmod_ops2 *ops = kdata;
> +
> +	ops->test_1();
> +	return 0;
> +}
> +
> +static struct bpf_testmod_ops2 __bpf_testmod_ops2 = {
> +	.test_1 = bpf_testmod_test_1,
> +};
> +
> +struct bpf_struct_ops bpf_testmod_ops2 = {
> +	.verifier_ops = &bpf_testmod_verifier_ops,
> +	.init = bpf_testmod_ops_init,
> +	.init_member = bpf_testmod_ops_init_member,
> +	.reg = bpf_dummy_reg2,
> +	.unreg = bpf_dummy_unreg,
> +	.cfi_stubs = &__bpf_testmod_ops2,
> +	.name = "bpf_testmod_ops2",
> +	.owner = THIS_MODULE,
> +};
> +
>  extern int bpf_fentry_test1(int a);
>  
>  static int bpf_testmod_init(void)
> @@ -612,6 +635,7 @@ static int bpf_testmod_init(void)
>  	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_testmod_kfunc_set);
>  	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_testmod_kfunc_set);
>  	ret = ret ?: register_bpf_struct_ops(&bpf_bpf_testmod_ops, bpf_testmod_ops);
> +	ret = ret ?: register_bpf_struct_ops(&bpf_testmod_ops2, bpf_testmod_ops2);
>  	if (ret < 0)
>  		return ret;
>  	if (bpf_fentry_test1(0) < 0)
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
> index c3b0cf788f9f..3183fff7f246 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
> @@ -37,4 +37,8 @@ struct bpf_testmod_ops {
>  	int (*test_maybe_null)(int dummy, struct task_struct *task);
>  };
>  
> +struct bpf_testmod_ops2 {
> +	int (*test_1)(void);
> +};
> +
>  #endif /* _BPF_TESTMOD_H */
> diff --git a/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c b/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
> new file mode 100644
> index 000000000000..9c689db4b05b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include "bad_struct_ops.skel.h"
> +
> +#define EXPECTED_MSG "libbpf: struct_ops reloc"
> +
> +static libbpf_print_fn_t old_print_cb;
> +static bool msg_found;
> +
> +static int print_cb(enum libbpf_print_level level, const char *fmt, va_list args)
> +{
> +	old_print_cb(level, fmt, args);
> +	if (level == LIBBPF_WARN && strncmp(fmt, EXPECTED_MSG, strlen(EXPECTED_MSG)) == 0)
> +		msg_found = true;
> +
> +	return 0;
> +}

Not necessary at all for this patch set / just an observation, but it would be
nice to have this be something offered by the core prog_tests framework
(meaning, the ability to assert libbpf output for a testcase).

> +
> +static void test_bad_struct_ops(void)
> +{
> +	struct bad_struct_ops *skel;
> +	int err;
> +
> +	old_print_cb = libbpf_set_print(print_cb);
> +	skel = bad_struct_ops__open_and_load();
> +	err = errno;
> +	libbpf_set_print(old_print_cb);
> +	if (!ASSERT_NULL(skel, "bad_struct_ops__open_and_load"))
> +		return;
> +
> +	ASSERT_EQ(err, EINVAL, "errno should be EINVAL");
> +	ASSERT_TRUE(msg_found, "expected message");
> +
> +	bad_struct_ops__destroy(skel);
> +}
> +
> +void serial_test_bad_struct_ops(void)
> +{
> +	if (test__start_subtest("test_bad_struct_ops"))
> +		test_bad_struct_ops();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/bad_struct_ops.c b/tools/testing/selftests/bpf/progs/bad_struct_ops.c
> new file mode 100644
> index 000000000000..9c103afbfdb1
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/bad_struct_ops.c
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <vmlinux.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include "../bpf_testmod/bpf_testmod.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +SEC("struct_ops/test_1")
> +int BPF_PROG(test_1) { return 0; }
> +
> +SEC(".struct_ops.link")
> +struct bpf_testmod_ops testmod_1 = { .test_1 = (void *)test_1 };

Just to make be 100% sure that we're isolating the issue under test, should we
also add a .test_2 prog and add it to the struct bpf_testmod_ops map?

> +
> +SEC(".struct_ops.link")
> +struct bpf_testmod_ops2 testmod_2 = { .test_1 = (void *)test_1 };
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH bpf-next v1 5/8] selftests/bpf: bad_struct_ops test

[Eduard Zingerman]

On Wed, 2024-02-28 at 12:15 -0600, David Vernet wrote:
[...]

> > +static libbpf_print_fn_t old_print_cb;
> > +static bool msg_found;
> > +
> > +static int print_cb(enum libbpf_print_level level, const char *fmt, va_list args)
> > +{
> > +	old_print_cb(level, fmt, args);
> > +	if (level == LIBBPF_WARN && strncmp(fmt, EXPECTED_MSG, strlen(EXPECTED_MSG)) == 0)
> > +		msg_found = true;
> > +
> > +	return 0;
> > +}
> 
> Not necessary at all for this patch set / just an observation, but it would be
> nice to have this be something offered by the core prog_tests framework
> (meaning, the ability to assert libbpf output for a testcase).

This might be useful, I will add a utility function for it (probably two).

[...]

> > diff --git a/tools/testing/selftests/bpf/progs/bad_struct_ops.c b/tools/testing/selftests/bpf/progs/bad_struct_ops.c
> > new file mode 100644
> > index 000000000000..9c103afbfdb1
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/bad_struct_ops.c
> > @@ -0,0 +1,17 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <vmlinux.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +#include "../bpf_testmod/bpf_testmod.h"
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +SEC("struct_ops/test_1")
> > +int BPF_PROG(test_1) { return 0; }
> > +
> > +SEC(".struct_ops.link")
> > +struct bpf_testmod_ops testmod_1 = { .test_1 = (void *)test_1 };
> 
> Just to make be 100% sure that we're isolating the issue under test, should we
> also add a .test_2 prog and add it to the struct bpf_testmod_ops map?

You are concerned that error might be confused with libbpf insisting
that '.test_2' should be present, right?
libbpf allows NULL members but I can add '.test_2' here, no problem.

[...]

Re: [PATCH bpf-next v1 5/8] selftests/bpf: bad_struct_ops test

[David Vernet]

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

On Wed, Feb 28, 2024 at 10:06:21PM +0200, Eduard Zingerman wrote:
> On Wed, 2024-02-28 at 12:15 -0600, David Vernet wrote:
> [...]
> 
> > > +static libbpf_print_fn_t old_print_cb;
> > > +static bool msg_found;
> > > +
> > > +static int print_cb(enum libbpf_print_level level, const char *fmt, va_list args)
> > > +{
> > > +	old_print_cb(level, fmt, args);
> > > +	if (level == LIBBPF_WARN && strncmp(fmt, EXPECTED_MSG, strlen(EXPECTED_MSG)) == 0)
> > > +		msg_found = true;
> > > +
> > > +	return 0;
> > > +}
> > 
> > Not necessary at all for this patch set / just an observation, but it would be
> > nice to have this be something offered by the core prog_tests framework
> > (meaning, the ability to assert libbpf output for a testcase).
> 
> This might be useful, I will add a utility function for it (probably two).
> 
> [...]
> 
> > > diff --git a/tools/testing/selftests/bpf/progs/bad_struct_ops.c b/tools/testing/selftests/bpf/progs/bad_struct_ops.c
> > > new file mode 100644
> > > index 000000000000..9c103afbfdb1
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/progs/bad_struct_ops.c
> > > @@ -0,0 +1,17 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +#include <vmlinux.h>
> > > +#include <bpf/bpf_helpers.h>
> > > +#include <bpf/bpf_tracing.h>
> > > +#include "../bpf_testmod/bpf_testmod.h"
> > > +
> > > +char _license[] SEC("license") = "GPL";
> > > +
> > > +SEC("struct_ops/test_1")
> > > +int BPF_PROG(test_1) { return 0; }
> > > +
> > > +SEC(".struct_ops.link")
> > > +struct bpf_testmod_ops testmod_1 = { .test_1 = (void *)test_1 };
> > 
> > Just to make be 100% sure that we're isolating the issue under test, should we
> > also add a .test_2 prog and add it to the struct bpf_testmod_ops map?
> 
> You are concerned that error might be confused with libbpf insisting
> that '.test_2' should be present, right?
> libbpf allows NULL members but I can add '.test_2' here, no problem.

Correct, and yes that's true. Feel free to ignore if you think it's cleaner
without, totally up to you.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH bpf-next v1 5/8] selftests/bpf: bad_struct_ops test

[Andrii Nakryiko]

On Tue, Feb 27, 2024 at 12:46 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> When loading struct_ops programs kernel requires BTF id of the
> struct_ops type and member index for attachment point inside that
> type. This makes it not possible to have same BPF program used in
> struct_ops maps that have different struct_ops type.
> Check if libbpf rejects such BPF objects files.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 24 +++++++++++
>  .../selftests/bpf/bpf_testmod/bpf_testmod.h   |  4 ++
>  .../selftests/bpf/prog_tests/bad_struct_ops.c | 42 +++++++++++++++++++
>  .../selftests/bpf/progs/bad_struct_ops.c      | 17 ++++++++
>  4 files changed, 87 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
>  create mode 100644 tools/testing/selftests/bpf/progs/bad_struct_ops.c
>
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index 0d8437e05f64..69f5eb9ad546 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -601,6 +601,29 @@ struct bpf_struct_ops bpf_bpf_testmod_ops = {
>         .owner = THIS_MODULE,
>  };
>
> +static int bpf_dummy_reg2(void *kdata)
> +{
> +       struct bpf_testmod_ops2 *ops = kdata;
> +
> +       ops->test_1();
> +       return 0;
> +}
> +
> +static struct bpf_testmod_ops2 __bpf_testmod_ops2 = {
> +       .test_1 = bpf_testmod_test_1,
> +};
> +
> +struct bpf_struct_ops bpf_testmod_ops2 = {
> +       .verifier_ops = &bpf_testmod_verifier_ops,
> +       .init = bpf_testmod_ops_init,
> +       .init_member = bpf_testmod_ops_init_member,
> +       .reg = bpf_dummy_reg2,
> +       .unreg = bpf_dummy_unreg,
> +       .cfi_stubs = &__bpf_testmod_ops2,
> +       .name = "bpf_testmod_ops2",
> +       .owner = THIS_MODULE,
> +};
> +
>  extern int bpf_fentry_test1(int a);
>
>  static int bpf_testmod_init(void)
> @@ -612,6 +635,7 @@ static int bpf_testmod_init(void)
>         ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_testmod_kfunc_set);
>         ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_testmod_kfunc_set);
>         ret = ret ?: register_bpf_struct_ops(&bpf_bpf_testmod_ops, bpf_testmod_ops);
> +       ret = ret ?: register_bpf_struct_ops(&bpf_testmod_ops2, bpf_testmod_ops2);
>         if (ret < 0)
>                 return ret;
>         if (bpf_fentry_test1(0) < 0)
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
> index c3b0cf788f9f..3183fff7f246 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
> @@ -37,4 +37,8 @@ struct bpf_testmod_ops {
>         int (*test_maybe_null)(int dummy, struct task_struct *task);
>  };
>
> +struct bpf_testmod_ops2 {
> +       int (*test_1)(void);
> +};
> +
>  #endif /* _BPF_TESTMOD_H */
> diff --git a/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c b/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
> new file mode 100644
> index 000000000000..9c689db4b05b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include "bad_struct_ops.skel.h"
> +
> +#define EXPECTED_MSG "libbpf: struct_ops reloc"
> +
> +static libbpf_print_fn_t old_print_cb;
> +static bool msg_found;
> +
> +static int print_cb(enum libbpf_print_level level, const char *fmt, va_list args)
> +{
> +       old_print_cb(level, fmt, args);
> +       if (level == LIBBPF_WARN && strncmp(fmt, EXPECTED_MSG, strlen(EXPECTED_MSG)) == 0)
> +               msg_found = true;
> +
> +       return 0;
> +}
> +
> +static void test_bad_struct_ops(void)
> +{
> +       struct bad_struct_ops *skel;
> +       int err;
> +
> +       old_print_cb = libbpf_set_print(print_cb);
> +       skel = bad_struct_ops__open_and_load();

we want to check that the load step failed specifically, right? So
please split open from load, make sure that open succeeds, but load
fails

> +       err = errno;
> +       libbpf_set_print(old_print_cb);
> +       if (!ASSERT_NULL(skel, "bad_struct_ops__open_and_load"))
> +               return;
> +
> +       ASSERT_EQ(err, EINVAL, "errno should be EINVAL");
> +       ASSERT_TRUE(msg_found, "expected message");
> +
> +       bad_struct_ops__destroy(skel);
> +}
> +
> +void serial_test_bad_struct_ops(void)

why does it have to be a serial test?

> +{
> +       if (test__start_subtest("test_bad_struct_ops"))
> +               test_bad_struct_ops();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/bad_struct_ops.c b/tools/testing/selftests/bpf/progs/bad_struct_ops.c
> new file mode 100644
> index 000000000000..9c103afbfdb1
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/bad_struct_ops.c
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <vmlinux.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include "../bpf_testmod/bpf_testmod.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +SEC("struct_ops/test_1")
> +int BPF_PROG(test_1) { return 0; }
> +
> +SEC(".struct_ops.link")
> +struct bpf_testmod_ops testmod_1 = { .test_1 = (void *)test_1 };
> +
> +SEC(".struct_ops.link")
> +struct bpf_testmod_ops2 testmod_2 = { .test_1 = (void *)test_1 };
> --
> 2.43.0
>

Re: [PATCH bpf-next v1 5/8] selftests/bpf: bad_struct_ops test

[Eduard Zingerman]

On Wed, 2024-02-28 at 15:40 -0800, Andrii Nakryiko wrote:
[...]

> > +static libbpf_print_fn_t old_print_cb;
> > +static bool msg_found;
> > +
> > +static int print_cb(enum libbpf_print_level level, const char *fmt, va_list args)
> > +{
> > +       old_print_cb(level, fmt, args);
> > +       if (level == LIBBPF_WARN && strncmp(fmt, EXPECTED_MSG, strlen(EXPECTED_MSG)) == 0)
> > +               msg_found = true;
> > +
> > +       return 0;
> > +}
> > +
> > +static void test_bad_struct_ops(void)
> > +{
> > +       struct bad_struct_ops *skel;
> > +       int err;
> > +
> > +       old_print_cb = libbpf_set_print(print_cb);
> > +       skel = bad_struct_ops__open_and_load();
> 
> we want to check that the load step failed specifically, right? So
> please split open from load, make sure that open succeeds, but load
> fails

Ok

> 
> > +       err = errno;
> > +       libbpf_set_print(old_print_cb);
> > +       if (!ASSERT_NULL(skel, "bad_struct_ops__open_and_load"))
> > +               return;
> > +
> > +       ASSERT_EQ(err, EINVAL, "errno should be EINVAL");
> > +       ASSERT_TRUE(msg_found, "expected message");
> > +
> > +       bad_struct_ops__destroy(skel);
> > +}
> > +
> > +void serial_test_bad_struct_ops(void)
> 
> why does it have to be a serial test?

Because it hijacks libbpf print callback.

[...]

Re: [PATCH bpf-next v1 5/8] selftests/bpf: bad_struct_ops test

[Andrii Nakryiko]

On Wed, Feb 28, 2024 at 3:44 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2024-02-28 at 15:40 -0800, Andrii Nakryiko wrote:
> [...]
>
> > > +static libbpf_print_fn_t old_print_cb;
> > > +static bool msg_found;
> > > +
> > > +static int print_cb(enum libbpf_print_level level, const char *fmt, va_list args)
> > > +{
> > > +       old_print_cb(level, fmt, args);
> > > +       if (level == LIBBPF_WARN && strncmp(fmt, EXPECTED_MSG, strlen(EXPECTED_MSG)) == 0)
> > > +               msg_found = true;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static void test_bad_struct_ops(void)
> > > +{
> > > +       struct bad_struct_ops *skel;
> > > +       int err;
> > > +
> > > +       old_print_cb = libbpf_set_print(print_cb);
> > > +       skel = bad_struct_ops__open_and_load();
> >
> > we want to check that the load step failed specifically, right? So
> > please split open from load, make sure that open succeeds, but load
> > fails
>
> Ok
>
> >
> > > +       err = errno;
> > > +       libbpf_set_print(old_print_cb);
> > > +       if (!ASSERT_NULL(skel, "bad_struct_ops__open_and_load"))
> > > +               return;
> > > +
> > > +       ASSERT_EQ(err, EINVAL, "errno should be EINVAL");
> > > +       ASSERT_TRUE(msg_found, "expected message");
> > > +
> > > +       bad_struct_ops__destroy(skel);
> > > +}
> > > +
> > > +void serial_test_bad_struct_ops(void)
> >
> > why does it have to be a serial test?
>
> Because it hijacks libbpf print callback.

each non-serial test runs in its own *process*, there is no
multi-threading here, so it's fine if non-serial test temporarily
hijacks print callback, as long as it restores it properly before
finishing

>
> [...]

Re: [PATCH bpf-next v1 5/8] selftests/bpf: bad_struct_ops test

[Eduard Zingerman]

On Wed, 2024-02-28 at 15:56 -0800, Andrii Nakryiko wrote:
[...]

> each non-serial test runs in its own *process*, there is no
> multi-threading here, so it's fine if non-serial test temporarily
> hijacks print callback, as long as it restores it properly before
> finishing

I missed this detail, thanks.

[PATCH bpf-next v1 6/8] selftests/bpf: test autocreate behavior for struct_ops maps

[Eduard Zingerman]

Check that bpf_map__set_autocreate() can be used to disable automatic
creation for struct_ops maps.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 .../bpf/prog_tests/struct_ops_autocreate.c    | 79 +++++++++++++++++++
 .../bpf/progs/struct_ops_autocreate.c         | 42 ++++++++++
 2 files changed, 121 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_autocreate.c

diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
new file mode 100644
index 000000000000..b21b10f94fc2
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+#include "struct_ops_autocreate.skel.h"
+
+#define EXPECTED_MSG "libbpf: struct_ops init_kern"
+
+static libbpf_print_fn_t old_print_cb;
+static bool msg_found;
+
+static int print_cb(enum libbpf_print_level level, const char *fmt, va_list args)
+{
+	old_print_cb(level, fmt, args);
+	if (level == LIBBPF_WARN && strncmp(fmt, EXPECTED_MSG, strlen(EXPECTED_MSG)) == 0)
+		msg_found = true;
+
+	return 0;
+}
+
+static void cant_load_full_object(void)
+{
+	struct struct_ops_autocreate *skel;
+	int err;
+
+	old_print_cb = libbpf_set_print(print_cb);
+	skel = struct_ops_autocreate__open_and_load();
+	err = errno;
+	libbpf_set_print(old_print_cb);
+	if (!ASSERT_NULL(skel, "struct_ops_autocreate__open_and_load"))
+		return;
+
+	ASSERT_EQ(err, ENOTSUP, "errno should be ENOTSUP");
+	ASSERT_TRUE(msg_found, "expected message");
+
+	struct_ops_autocreate__destroy(skel);
+}
+
+static void can_load_partial_object(void)
+{
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+	struct struct_ops_autocreate *skel;
+	struct bpf_link *link = NULL;
+	int err;
+
+	skel = struct_ops_autocreate__open_opts(&opts);
+	if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open_opts"))
+		return;
+
+	err = bpf_program__set_autoload(skel->progs.test_2, false);
+	if (!ASSERT_OK(err, "bpf_program__set_autoload"))
+		goto cleanup;
+
+	err = bpf_map__set_autocreate(skel->maps.testmod_2, false);
+	if (!ASSERT_OK(err, "bpf_map__set_autocreate"))
+		goto cleanup;
+
+	err = struct_ops_autocreate__load(skel);
+	if (ASSERT_OK(err, "struct_ops_autocreate__load"))
+		goto cleanup;
+
+	link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
+	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops"))
+		goto cleanup;
+
+	/* test_1() would be called from bpf_dummy_reg2() in bpf_testmod.c */
+	ASSERT_EQ(skel->bss->test_1_result, 42, "test_1_result");
+
+cleanup:
+	bpf_link__destroy(link);
+	struct_ops_autocreate__destroy(skel);
+}
+
+void serial_test_struct_ops_autocreate(void)
+{
+	if (test__start_subtest("cant_load_full_object"))
+		cant_load_full_object();
+	if (test__start_subtest("can_load_partial_object"))
+		can_load_partial_object();
+}
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
new file mode 100644
index 000000000000..294d48bb8e3c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+int test_1_result = 0;
+
+SEC("struct_ops/test_1")
+int BPF_PROG(test_1)
+{
+	test_1_result = 42;
+	return 0;
+}
+
+SEC("struct_ops/test_1")
+int BPF_PROG(test_2)
+{
+	return 0;
+}
+
+struct bpf_testmod_ops___v1 {
+	int (*test_1)(void);
+};
+
+struct bpf_testmod_ops___v2 {
+	int (*test_1)(void);
+	int (*does_not_exist)(void);
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops___v1 testmod_1 = {
+	.test_1 = (void *)test_1
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops___v2 testmod_2 = {
+	.test_1 = (void *)test_1,
+	.does_not_exist = (void *)test_2
+};
-- 
2.43.0

Re: [PATCH bpf-next v1 6/8] selftests/bpf: test autocreate behavior for struct_ops maps

[David Vernet]

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

On Tue, Feb 27, 2024 at 10:45:54PM +0200, Eduard Zingerman wrote:
> Check that bpf_map__set_autocreate() can be used to disable automatic
> creation for struct_ops maps.
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  .../bpf/prog_tests/struct_ops_autocreate.c    | 79 +++++++++++++++++++
>  .../bpf/progs/struct_ops_autocreate.c         | 42 ++++++++++
>  2 files changed, 121 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
>  create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> new file mode 100644
> index 000000000000..b21b10f94fc2
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include "struct_ops_autocreate.skel.h"
> +
> +#define EXPECTED_MSG "libbpf: struct_ops init_kern"
> +
> +static libbpf_print_fn_t old_print_cb;
> +static bool msg_found;
> +
> +static int print_cb(enum libbpf_print_level level, const char *fmt, va_list args)
> +{
> +	old_print_cb(level, fmt, args);
> +	if (level == LIBBPF_WARN && strncmp(fmt, EXPECTED_MSG, strlen(EXPECTED_MSG)) == 0)
> +		msg_found = true;
> +
> +	return 0;
> +}
> +
> +static void cant_load_full_object(void)
> +{
> +	struct struct_ops_autocreate *skel;
> +	int err;
> +
> +	old_print_cb = libbpf_set_print(print_cb);
> +	skel = struct_ops_autocreate__open_and_load();

Optional suggestion: It might be useful to add a comment here explaining
exactly why we expect this to fail? Something like:

	/* The testmod_2 map BTF type (struct bpf_testmod_ops___v2) doesn't
	 * match the BTF of the actual struct bpf_testmod_ops defined in the
	 * kernel, so we should fail to load it if we don't disable autocreate
	 * for the map.
	 */

Feel free to ignore -- I recognize that some might just consider that
unnecessary noise.

> +	err = errno;
> +	libbpf_set_print(old_print_cb);
> +	if (!ASSERT_NULL(skel, "struct_ops_autocreate__open_and_load"))
> +		return;
> +
> +	ASSERT_EQ(err, ENOTSUP, "errno should be ENOTSUP");
> +	ASSERT_TRUE(msg_found, "expected message");
> +
> +	struct_ops_autocreate__destroy(skel);
> +}
> +
> +static void can_load_partial_object(void)
> +{
> +	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
> +	struct struct_ops_autocreate *skel;
> +	struct bpf_link *link = NULL;
> +	int err;
> +
> +	skel = struct_ops_autocreate__open_opts(&opts);
> +	if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open_opts"))
> +		return;
> +
> +	err = bpf_program__set_autoload(skel->progs.test_2, false);
> +	if (!ASSERT_OK(err, "bpf_program__set_autoload"))
> +		goto cleanup;

It feels a bit awkward to have to specify that a struct_ops prog isn't
autoloaded if it's not associated with an autoloaded / autocreated struct_ops
map. Would it be possible to teach libbpf to not autoload such progs by
default?

> +	err = bpf_map__set_autocreate(skel->maps.testmod_2, false);
> +	if (!ASSERT_OK(err, "bpf_map__set_autocreate"))
> +		goto cleanup;
> +
> +	err = struct_ops_autocreate__load(skel);
> +	if (ASSERT_OK(err, "struct_ops_autocreate__load"))
> +		goto cleanup;
> +
> +	link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
> +	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops"))
> +		goto cleanup;
> +
> +	/* test_1() would be called from bpf_dummy_reg2() in bpf_testmod.c */
> +	ASSERT_EQ(skel->bss->test_1_result, 42, "test_1_result");
> +
> +cleanup:
> +	bpf_link__destroy(link);
> +	struct_ops_autocreate__destroy(skel);
> +}
> +
> +void serial_test_struct_ops_autocreate(void)
> +{
> +	if (test__start_subtest("cant_load_full_object"))
> +		cant_load_full_object();
> +	if (test__start_subtest("can_load_partial_object"))
> +		can_load_partial_object();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
> new file mode 100644
> index 000000000000..294d48bb8e3c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <vmlinux.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +int test_1_result = 0;
> +
> +SEC("struct_ops/test_1")
> +int BPF_PROG(test_1)
> +{
> +	test_1_result = 42;
> +	return 0;
> +}
> +
> +SEC("struct_ops/test_1")
> +int BPF_PROG(test_2)
> +{
> +	return 0;
> +}
> +
> +struct bpf_testmod_ops___v1 {
> +	int (*test_1)(void);
> +};
> +
> +struct bpf_testmod_ops___v2 {
> +	int (*test_1)(void);
> +	int (*does_not_exist)(void);
> +};
> +
> +SEC(".struct_ops.link")
> +struct bpf_testmod_ops___v1 testmod_1 = {
> +	.test_1 = (void *)test_1
> +};
> +
> +SEC(".struct_ops.link")
> +struct bpf_testmod_ops___v2 testmod_2 = {
> +	.test_1 = (void *)test_1,
> +	.does_not_exist = (void *)test_2
> +};
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH bpf-next v1 6/8] selftests/bpf: test autocreate behavior for struct_ops maps

[David Vernet]

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

On Wed, Feb 28, 2024 at 12:29:49PM -0600, David Vernet wrote:
> On Tue, Feb 27, 2024 at 10:45:54PM +0200, Eduard Zingerman wrote:
> > Check that bpf_map__set_autocreate() can be used to disable automatic
> > creation for struct_ops maps.
> > 
> > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > ---
> >  .../bpf/prog_tests/struct_ops_autocreate.c    | 79 +++++++++++++++++++
> >  .../bpf/progs/struct_ops_autocreate.c         | 42 ++++++++++
> >  2 files changed, 121 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
> > 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> > new file mode 100644
> > index 000000000000..b21b10f94fc2
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> > @@ -0,0 +1,79 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <test_progs.h>
> > +#include "struct_ops_autocreate.skel.h"
> > +
> > +#define EXPECTED_MSG "libbpf: struct_ops init_kern"
> > +
> > +static libbpf_print_fn_t old_print_cb;
> > +static bool msg_found;
> > +
> > +static int print_cb(enum libbpf_print_level level, const char *fmt, va_list args)
> > +{
> > +	old_print_cb(level, fmt, args);
> > +	if (level == LIBBPF_WARN && strncmp(fmt, EXPECTED_MSG, strlen(EXPECTED_MSG)) == 0)
> > +		msg_found = true;
> > +
> > +	return 0;
> > +}
> > +
> > +static void cant_load_full_object(void)
> > +{
> > +	struct struct_ops_autocreate *skel;
> > +	int err;
> > +
> > +	old_print_cb = libbpf_set_print(print_cb);
> > +	skel = struct_ops_autocreate__open_and_load();
> 
> Optional suggestion: It might be useful to add a comment here explaining
> exactly why we expect this to fail? Something like:
> 
> 	/* The testmod_2 map BTF type (struct bpf_testmod_ops___v2) doesn't
> 	 * match the BTF of the actual struct bpf_testmod_ops defined in the
> 	 * kernel, so we should fail to load it if we don't disable autocreate
> 	 * for the map.
> 	 */
> 
> Feel free to ignore -- I recognize that some might just consider that
> unnecessary noise.
> 
> > +	err = errno;
> > +	libbpf_set_print(old_print_cb);
> > +	if (!ASSERT_NULL(skel, "struct_ops_autocreate__open_and_load"))
> > +		return;
> > +
> > +	ASSERT_EQ(err, ENOTSUP, "errno should be ENOTSUP");
> > +	ASSERT_TRUE(msg_found, "expected message");
> > +
> > +	struct_ops_autocreate__destroy(skel);
> > +}
> > +
> > +static void can_load_partial_object(void)
> > +{
> > +	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
> > +	struct struct_ops_autocreate *skel;
> > +	struct bpf_link *link = NULL;
> > +	int err;
> > +
> > +	skel = struct_ops_autocreate__open_opts(&opts);
> > +	if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open_opts"))
> > +		return;
> > +
> > +	err = bpf_program__set_autoload(skel->progs.test_2, false);
> > +	if (!ASSERT_OK(err, "bpf_program__set_autoload"))
> > +		goto cleanup;
> 
> It feels a bit awkward to have to specify that a struct_ops prog isn't
> autoloaded if it's not associated with an autoloaded / autocreated struct_ops
> map. Would it be possible to teach libbpf to not autoload such progs by
> default?

I see you already added that in the next patch. Nice!!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH bpf-next v1 6/8] selftests/bpf: test autocreate behavior for struct_ops maps

[Eduard Zingerman]

On Wed, 2024-02-28 at 12:29 -0600, David Vernet wrote:

[...]

> > +static void cant_load_full_object(void)
> > +{
> > +	struct struct_ops_autocreate *skel;
> > +	int err;
> > +
> > +	old_print_cb = libbpf_set_print(print_cb);
> > +	skel = struct_ops_autocreate__open_and_load();
> 
> Optional suggestion: It might be useful to add a comment here explaining
> exactly why we expect this to fail? Something like:
> 
> 	/* The testmod_2 map BTF type (struct bpf_testmod_ops___v2) doesn't
> 	 * match the BTF of the actual struct bpf_testmod_ops defined in the
> 	 * kernel, so we should fail to load it if we don't disable autocreate
> 	 * for the map.
> 	 */
> 
> Feel free to ignore -- I recognize that some might just consider that
> unnecessary noise.

I will add this comment, agree that it helps when tests explain what happens.

[...]

Re: [PATCH bpf-next v1 6/8] selftests/bpf: test autocreate behavior for struct_ops maps

[Andrii Nakryiko]

On Tue, Feb 27, 2024 at 12:46 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Check that bpf_map__set_autocreate() can be used to disable automatic
> creation for struct_ops maps.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  .../bpf/prog_tests/struct_ops_autocreate.c    | 79 +++++++++++++++++++
>  .../bpf/progs/struct_ops_autocreate.c         | 42 ++++++++++
>  2 files changed, 121 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
>  create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> new file mode 100644
> index 000000000000..b21b10f94fc2
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include "struct_ops_autocreate.skel.h"
> +
> +#define EXPECTED_MSG "libbpf: struct_ops init_kern"
> +
> +static libbpf_print_fn_t old_print_cb;
> +static bool msg_found;
> +
> +static int print_cb(enum libbpf_print_level level, const char *fmt, va_list args)
> +{
> +       old_print_cb(level, fmt, args);
> +       if (level == LIBBPF_WARN && strncmp(fmt, EXPECTED_MSG, strlen(EXPECTED_MSG)) == 0)
> +               msg_found = true;
> +
> +       return 0;
> +}
> +
> +static void cant_load_full_object(void)
> +{
> +       struct struct_ops_autocreate *skel;
> +       int err;
> +
> +       old_print_cb = libbpf_set_print(print_cb);
> +       skel = struct_ops_autocreate__open_and_load();
> +       err = errno;
> +       libbpf_set_print(old_print_cb);
> +       if (!ASSERT_NULL(skel, "struct_ops_autocreate__open_and_load"))
> +               return;
> +
> +       ASSERT_EQ(err, ENOTSUP, "errno should be ENOTSUP");
> +       ASSERT_TRUE(msg_found, "expected message");
> +
> +       struct_ops_autocreate__destroy(skel);
> +}
> +
> +static void can_load_partial_object(void)
> +{
> +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);

nit: prefer LIBBPF_OPTS() over DECLARE_LIBBPF_OPTS()

> +       struct struct_ops_autocreate *skel;
> +       struct bpf_link *link = NULL;
> +       int err;
> +
> +       skel = struct_ops_autocreate__open_opts(&opts);
> +       if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open_opts"))
> +               return;
> +
> +       err = bpf_program__set_autoload(skel->progs.test_2, false);
> +       if (!ASSERT_OK(err, "bpf_program__set_autoload"))
> +               goto cleanup;
> +
> +       err = bpf_map__set_autocreate(skel->maps.testmod_2, false);
> +       if (!ASSERT_OK(err, "bpf_map__set_autocreate"))
> +               goto cleanup;
> +
> +       err = struct_ops_autocreate__load(skel);
> +       if (ASSERT_OK(err, "struct_ops_autocreate__load"))
> +               goto cleanup;
> +
> +       link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
> +       if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops"))
> +               goto cleanup;
> +
> +       /* test_1() would be called from bpf_dummy_reg2() in bpf_testmod.c */
> +       ASSERT_EQ(skel->bss->test_1_result, 42, "test_1_result");
> +
> +cleanup:
> +       bpf_link__destroy(link);
> +       struct_ops_autocreate__destroy(skel);
> +}
> +
> +void serial_test_struct_ops_autocreate(void)

same as in the previous patch, why serial?

> +{
> +       if (test__start_subtest("cant_load_full_object"))
> +               cant_load_full_object();
> +       if (test__start_subtest("can_load_partial_object"))
> +               can_load_partial_object();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
> new file mode 100644
> index 000000000000..294d48bb8e3c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <vmlinux.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +int test_1_result = 0;
> +
> +SEC("struct_ops/test_1")
> +int BPF_PROG(test_1)
> +{
> +       test_1_result = 42;
> +       return 0;
> +}
> +
> +SEC("struct_ops/test_1")
> +int BPF_PROG(test_2)
> +{
> +       return 0;
> +}
> +
> +struct bpf_testmod_ops___v1 {
> +       int (*test_1)(void);
> +};
> +
> +struct bpf_testmod_ops___v2 {
> +       int (*test_1)(void);
> +       int (*does_not_exist)(void);
> +};
> +
> +SEC(".struct_ops.link")
> +struct bpf_testmod_ops___v1 testmod_1 = {
> +       .test_1 = (void *)test_1
> +};
> +
> +SEC(".struct_ops.link")

can you please also have a test where we use SEC("?.struct_ops.link")
which set autoload to false by default?

> +struct bpf_testmod_ops___v2 testmod_2 = {
> +       .test_1 = (void *)test_1,
> +       .does_not_exist = (void *)test_2
> +};
> --
> 2.43.0
>

Re: [PATCH bpf-next v1 6/8] selftests/bpf: test autocreate behavior for struct_ops maps

[Eduard Zingerman]

On Wed, 2024-02-28 at 15:43 -0800, Andrii Nakryiko wrote:
[...]

> > +static void can_load_partial_object(void)
> > +{
> > +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
> 
> nit: prefer LIBBPF_OPTS() over DECLARE_LIBBPF_OPTS()

Ok

[...]

> > +void serial_test_struct_ops_autocreate(void)
> 
> same as in the previous patch, why serial?

Because of the print callback hijacking.
Also, what would happen when two tests would set struct_ops for same
attachment point?

[...]

> > +SEC(".struct_ops.link")
> 
> can you please also have a test where we use SEC("?.struct_ops.link")
> which set autoload to false by default?

As far as I understand, that would be a new behavior, currently '?'
works only for programs. I'll add a separate patch to support this.

[...]

Re: [PATCH bpf-next v1 6/8] selftests/bpf: test autocreate behavior for struct_ops maps

[Andrii Nakryiko]

On Wed, Feb 28, 2024 at 3:55 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2024-02-28 at 15:43 -0800, Andrii Nakryiko wrote:
> [...]
>
> > > +static void can_load_partial_object(void)
> > > +{
> > > +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
> >
> > nit: prefer LIBBPF_OPTS() over DECLARE_LIBBPF_OPTS()
>
> Ok
>
> [...]
>
> > > +void serial_test_struct_ops_autocreate(void)
> >
> > same as in the previous patch, why serial?
>
> Because of the print callback hijacking.
> Also, what would happen when two tests would set struct_ops for same
> attachment point?

I'm not sure, Martin?

But if this is a problem, then perhaps it's best to combine all
struct_ops tests that use bpf_testmod into a single test as multiple
subtests. And this use non-serial overall test approach.

>
> [...]
>
> > > +SEC(".struct_ops.link")
> >
> > can you please also have a test where we use SEC("?.struct_ops.link")
> > which set autoload to false by default?
>
> As far as I understand, that would be a new behavior, currently '?'
> works only for programs. I'll add a separate patch to support this.
>

Yep, thanks!

> [...]

Re: [PATCH bpf-next v1 6/8] selftests/bpf: test autocreate behavior for struct_ops maps

[Martin KaFai Lau]

On 2/28/24 4:02 PM, Andrii Nakryiko wrote:
>> Also, what would happen when two tests would set struct_ops for same
>> attachment point?
> I'm not sure, Martin?

The kernel bpf struct_ops infra can handle it. For example, different userspace 
processes may register bpf-tcp-cc in parallel also. It is a matter of the 
subsystem (tcp in the bpf-tcp-cc case) can handle it or not.

In the testmod case, it will be bpf_dummy_reg[2] and nothing in there is 
stopping parallel run afaict.

Re: [PATCH bpf-next v1 6/8] selftests/bpf: test autocreate behavior for struct_ops maps

[Eduard Zingerman]

On Wed, 2024-02-28 at 16:02 -0800, Andrii Nakryiko wrote:
> On Wed, Feb 28, 2024 at 3:55 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
[...]
> > On Wed, 2024-02-28 at 15:43 -0800, Andrii Nakryiko wrote:
> > > > +SEC(".struct_ops.link")
> > > 
> > > can you please also have a test where we use SEC("?.struct_ops.link")
> > > which set autoload to false by default?
> > 
> > As far as I understand, that would be a new behavior, currently '?'
> > works only for programs. I'll add a separate patch to support this.
> > 
> 
> Yep, thanks!

So, I have a draft for v2 with support for this feature in [0].
But there is a gotcha:

    libbpf: BTF loading error: -22
    libbpf: -- BEGIN BTF LOAD LOG ---
    ...
    [23] DATASEC ?.struct_ops size=8 vlen=1 Invalid name
    
    -- END BTF LOAD LOG --
    libbpf: Error loading .BTF into kernel: -22. BTF is mandatory, can't proceed.

Kernel rejects DATASEC name with '?'.
The options are:
1. Tweak kernel to allow '?' as a first char in DATASEC names;
2. Use some different name, e.g. ".struct_ops.opt";
3. Do some kind of BTF rewrite in libbpf to merge
   "?.struct_ops" and ".struct_ops" DATASECs as a single DATASEC.
  
(1) is simple, but downside is requirement for kernel upgrade;
(2) is simple, but goes against current convention for program section names;
(3) idk, will check if that is feasible tomorrow.

[0] https://github.com/eddyz87/bpf/tree/structops-multi-version

Re: [PATCH bpf-next v1 6/8] selftests/bpf: test autocreate behavior for struct_ops maps

[Andrii Nakryiko]

On Thu, Feb 29, 2024 at 5:28 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2024-02-28 at 16:02 -0800, Andrii Nakryiko wrote:
> > On Wed, Feb 28, 2024 at 3:55 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > >
> [...]
> > > On Wed, 2024-02-28 at 15:43 -0800, Andrii Nakryiko wrote:
> > > > > +SEC(".struct_ops.link")
> > > >
> > > > can you please also have a test where we use SEC("?.struct_ops.link")
> > > > which set autoload to false by default?
> > >
> > > As far as I understand, that would be a new behavior, currently '?'
> > > works only for programs. I'll add a separate patch to support this.
> > >
> >
> > Yep, thanks!
>
> So, I have a draft for v2 with support for this feature in [0].
> But there is a gotcha:
>
>     libbpf: BTF loading error: -22
>     libbpf: -- BEGIN BTF LOAD LOG ---
>     ...
>     [23] DATASEC ?.struct_ops size=8 vlen=1 Invalid name
>
>     -- END BTF LOAD LOG --
>     libbpf: Error loading .BTF into kernel: -22. BTF is mandatory, can't proceed.
>
> Kernel rejects DATASEC name with '?'.
> The options are:
> 1. Tweak kernel to allow '?' as a first char in DATASEC names;
> 2. Use some different name, e.g. ".struct_ops.opt";
> 3. Do some kind of BTF rewrite in libbpf to merge
>    "?.struct_ops" and ".struct_ops" DATASECs as a single DATASEC.
>
> (1) is simple, but downside is requirement for kernel upgrade;
> (2) is simple, but goes against current convention for program section names;
> (3) idk, will check if that is feasible tomorrow.
>
> [0] https://github.com/eddyz87/bpf/tree/structops-multi-version
>
>

Let's do 1) for sure (ELF allows pretty much any name for DATASEC).
And then detect this in libbpf, and if the kernel is old and doesn't
yet support it, change DATASEC name from "?.struct_ops" to just
".struct_ops". Merging DATASECs seems like an unnecessary
complication.

>

Re: [PATCH bpf-next v1 6/8] selftests/bpf: test autocreate behavior for struct_ops maps

[Eduard Zingerman]

On Fri, 2024-03-01 at 10:03 -0800, Andrii Nakryiko wrote:
[...]

> > Kernel rejects DATASEC name with '?'.
> > The options are:
> > 1. Tweak kernel to allow '?' as a first char in DATASEC names;
> > 2. Use some different name, e.g. ".struct_ops.opt";
> > 3. Do some kind of BTF rewrite in libbpf to merge
> >    "?.struct_ops" and ".struct_ops" DATASECs as a single DATASEC.
> > 
> > (1) is simple, but downside is requirement for kernel upgrade;
> > (2) is simple, but goes against current convention for program section names;
> > (3) idk, will check if that is feasible tomorrow.
> > 
> > [0] https://github.com/eddyz87/bpf/tree/structops-multi-version
> > 
> > 
> 
> Let's do 1) for sure (ELF allows pretty much any name for DATASEC).
> And then detect this in libbpf, and if the kernel is old and doesn't
> yet support it, change DATASEC name from "?.struct_ops" to just
> ".struct_ops". Merging DATASECs seems like an unnecessary
> complication.

Yes, makes sense, half way there already, thank you.

[PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Eduard Zingerman]

Make bpf_map__set_autocreate() for struct_ops maps toggle autoload
state for referenced programs.

E.g. for the BPF code below:

    SEC("struct_ops/test_1") int BPF_PROG(foo) { ... }
    SEC("struct_ops/test_2") int BPF_PROG(bar) { ... }

    SEC(".struct_ops.link")
    struct test_ops___v1 A = {
        .foo = (void *)foo
    };

    SEC(".struct_ops.link")
    struct test_ops___v2 B = {
        .foo = (void *)foo,
        .bar = (void *)bar,
    };

And the following libbpf API calls:

    bpf_map__set_autocreate(skel->maps.A, true);
    bpf_map__set_autocreate(skel->maps.B, false);

The autoload would be enabled for program 'foo' and disabled for
program 'bar'.

Do not apply such toggling if program autoload state is set by a call
to bpf_program__set_autoload().

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index b39d3f2898a1..1ea3046724f8 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -446,13 +446,18 @@ struct bpf_program {
 	struct bpf_object *obj;
 
 	int fd;
-	bool autoload;
+	bool autoload:1;
+	bool autoload_user_set:1;
 	bool autoattach;
 	bool sym_global;
 	bool mark_btf_static;
 	enum bpf_prog_type type;
 	enum bpf_attach_type expected_attach_type;
 	int exception_cb_idx;
+	/* total number of struct_ops maps with autocreate == true
+	 * that reference this program
+	 */
+	__u32 struct_ops_refs;
 
 	int prog_ifindex;
 	__u32 attach_btf_obj_fd;
@@ -4509,6 +4514,28 @@ static int bpf_get_map_info_from_fdinfo(int fd, struct bpf_map_info *info)
 	return 0;
 }
 
+/* Sync autoload and autocreate state between struct_ops map and
+ * referenced programs.
+ */
+static void bpf_map__struct_ops_toggle_progs_autoload(struct bpf_map *map, bool autocreate)
+{
+	struct bpf_program *prog;
+	int i;
+
+	for (i = 0; i < btf_vlen(map->st_ops->type); ++i) {
+		prog = map->st_ops->progs[i];
+
+		if (!prog || prog->autoload_user_set)
+			continue;
+
+		if (autocreate)
+			prog->struct_ops_refs++;
+		else
+			prog->struct_ops_refs--;
+		prog->autoload = prog->struct_ops_refs != 0;
+	}
+}
+
 bool bpf_map__autocreate(const struct bpf_map *map)
 {
 	return map->autocreate;
@@ -4519,6 +4546,9 @@ int bpf_map__set_autocreate(struct bpf_map *map, bool autocreate)
 	if (map->obj->loaded)
 		return libbpf_err(-EBUSY);
 
+	if (map->st_ops && map->autocreate != autocreate)
+		bpf_map__struct_ops_toggle_progs_autoload(map, autocreate);
+
 	map->autocreate = autocreate;
 	return 0;
 }
@@ -8801,6 +8831,7 @@ int bpf_program__set_autoload(struct bpf_program *prog, bool autoload)
 		return libbpf_err(-EINVAL);
 
 	prog->autoload = autoload;
+	prog->autoload_user_set = 1;
 	return 0;
 }
 
@@ -9428,6 +9459,8 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
 			return -EINVAL;
 		}
 
+		if (map->autocreate)
+			prog->struct_ops_refs++;
 		st_ops->progs[member_idx] = prog;
 	}
 
-- 
2.43.0

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Kui-Feng Lee]

On 2/27/24 12:45, Eduard Zingerman wrote:
> Make bpf_map__set_autocreate() for struct_ops maps toggle autoload
> state for referenced programs.
> 
> E.g. for the BPF code below:
> 
>      SEC("struct_ops/test_1") int BPF_PROG(foo) { ... }
>      SEC("struct_ops/test_2") int BPF_PROG(bar) { ... }
> 
>      SEC(".struct_ops.link")
>      struct test_ops___v1 A = {
>          .foo = (void *)foo
>      };
> 
>      SEC(".struct_ops.link")
>      struct test_ops___v2 B = {
>          .foo = (void *)foo,
>          .bar = (void *)bar,
>      };
> 
> And the following libbpf API calls:
> 
>      bpf_map__set_autocreate(skel->maps.A, true);
>      bpf_map__set_autocreate(skel->maps.B, false);
> 
> The autoload would be enabled for program 'foo' and disabled for
> program 'bar'.
> 
> Do not apply such toggling if program autoload state is set by a call
> to bpf_program__set_autoload().
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>   tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index b39d3f2898a1..1ea3046724f8 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -446,13 +446,18 @@ struct bpf_program {
>   	struct bpf_object *obj;
>   
>   	int fd;
> -	bool autoload;
> +	bool autoload:1;
> +	bool autoload_user_set:1;
>   	bool autoattach;
>   	bool sym_global;
>   	bool mark_btf_static;
>   	enum bpf_prog_type type;
>   	enum bpf_attach_type expected_attach_type;
>   	int exception_cb_idx;
> +	/* total number of struct_ops maps with autocreate == true
> +	 * that reference this program
> +	 */
> +	__u32 struct_ops_refs;
>   
>   	int prog_ifindex;
>   	__u32 attach_btf_obj_fd;
> @@ -4509,6 +4514,28 @@ static int bpf_get_map_info_from_fdinfo(int fd, struct bpf_map_info *info)
>   	return 0;
>   }
>   
> +/* Sync autoload and autocreate state between struct_ops map and
> + * referenced programs.
> + */
> +static void bpf_map__struct_ops_toggle_progs_autoload(struct bpf_map *map, bool autocreate)
> +{
> +	struct bpf_program *prog;
> +	int i;
> +
> +	for (i = 0; i < btf_vlen(map->st_ops->type); ++i) {
> +		prog = map->st_ops->progs[i];
> +
> +		if (!prog || prog->autoload_user_set)
> +			continue;
> +
> +		if (autocreate)
> +			prog->struct_ops_refs++;
> +		else
> +			prog->struct_ops_refs--;
> +		prog->autoload = prog->struct_ops_refs != 0;
> +	}
> +}
> +

This part is related to the other patch [1], which allows
a user to change the value of a function pointer field. The behavior of
autocreate and autoload may suprise a user if the user call
bpf_map__set_autocreate() after changing the value of a function pointer
field.

[1] 
https://lore.kernel.org/all/20240227010432.714127-1-thinker.li@gmail.com/


>   bool bpf_map__autocreate(const struct bpf_map *map)
>   {
>   	return map->autocreate;
> @@ -4519,6 +4546,9 @@ int bpf_map__set_autocreate(struct bpf_map *map, bool autocreate)
>   	if (map->obj->loaded)
>   		return libbpf_err(-EBUSY);
>   
> +	if (map->st_ops && map->autocreate != autocreate)
> +		bpf_map__struct_ops_toggle_progs_autoload(map, autocreate);
> +
>   	map->autocreate = autocreate;
>   	return 0;
>   }
> @@ -8801,6 +8831,7 @@ int bpf_program__set_autoload(struct bpf_program *prog, bool autoload)
>   		return libbpf_err(-EINVAL);
>   
>   	prog->autoload = autoload;
> +	prog->autoload_user_set = 1;
>   	return 0;
>   }
>   
> @@ -9428,6 +9459,8 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
>   			return -EINVAL;
>   		}
>   
> +		if (map->autocreate)
> +			prog->struct_ops_refs++;
>   		st_ops->progs[member_idx] = prog;
>   	}
>   

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Eduard Zingerman]

On Tue, 2024-02-27 at 14:55 -0800, Kui-Feng Lee wrote:
[...]

> > @@ -4509,6 +4514,28 @@ static int bpf_get_map_info_from_fdinfo(int fd, struct bpf_map_info *info)
> >   	return 0;
> >   }
> >   
> > +/* Sync autoload and autocreate state between struct_ops map and
> > + * referenced programs.
> > + */
> > +static void bpf_map__struct_ops_toggle_progs_autoload(struct bpf_map *map, bool autocreate)
> > +{
> > +	struct bpf_program *prog;
> > +	int i;
> > +
> > +	for (i = 0; i < btf_vlen(map->st_ops->type); ++i) {
> > +		prog = map->st_ops->progs[i];
> > +
> > +		if (!prog || prog->autoload_user_set)
> > +			continue;
> > +
> > +		if (autocreate)
> > +			prog->struct_ops_refs++;
> > +		else
> > +			prog->struct_ops_refs--;
> > +		prog->autoload = prog->struct_ops_refs != 0;
> > +	}
> > +}
> > +
> 
> This part is related to the other patch [1], which allows
> a user to change the value of a function pointer field. The behavior of
> autocreate and autoload may suprise a user if the user call
> bpf_map__set_autocreate() after changing the value of a function pointer
> field.
> 
> [1] 
> https://lore.kernel.org/all/20240227010432.714127-1-thinker.li@gmail.com/

So, it appears that with shadow types users would have more or less
convenient way to disable / enable related BPF programs
(the references to programs are available, but reference counting
 would have to be implemented by user using some additional data
 structure, if needed).

I don't see a way to reconcile shadow types with this autoload/autocreate toggling
=> my last two patches would have to be dropped.

Wdyt?

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Kui-Feng Lee]

On 2/27/24 15:09, Eduard Zingerman wrote:
> On Tue, 2024-02-27 at 14:55 -0800, Kui-Feng Lee wrote:
> [...]
> 
>>> @@ -4509,6 +4514,28 @@ static int bpf_get_map_info_from_fdinfo(int fd, struct bpf_map_info *info)
>>>    	return 0;
>>>    }
>>>    
>>> +/* Sync autoload and autocreate state between struct_ops map and
>>> + * referenced programs.
>>> + */
>>> +static void bpf_map__struct_ops_toggle_progs_autoload(struct bpf_map *map, bool autocreate)
>>> +{
>>> +	struct bpf_program *prog;
>>> +	int i;
>>> +
>>> +	for (i = 0; i < btf_vlen(map->st_ops->type); ++i) {
>>> +		prog = map->st_ops->progs[i];
>>> +
>>> +		if (!prog || prog->autoload_user_set)
>>> +			continue;
>>> +
>>> +		if (autocreate)
>>> +			prog->struct_ops_refs++;
>>> +		else
>>> +			prog->struct_ops_refs--;
>>> +		prog->autoload = prog->struct_ops_refs != 0;
>>> +	}
>>> +}
>>> +
>>
>> This part is related to the other patch [1], which allows
>> a user to change the value of a function pointer field. The behavior of
>> autocreate and autoload may suprise a user if the user call
>> bpf_map__set_autocreate() after changing the value of a function pointer
>> field.
>>
>> [1]
>> https://lore.kernel.org/all/20240227010432.714127-1-thinker.li@gmail.com/
> 
> So, it appears that with shadow types users would have more or less
> convenient way to disable / enable related BPF programs
> (the references to programs are available, but reference counting
>   would have to be implemented by user using some additional data
>   structure, if needed).
> 
> I don't see a way to reconcile shadow types with this autoload/autocreate toggling
> => my last two patches would have to be dropped.

How about to update autoload according to the value of autocreate of
maps before loading the programs? For example, update autoload in
bpf_map__init_kern_struct_ops()?

> 
> Wdyt?

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Eduard Zingerman]

On Tue, 2024-02-27 at 15:16 -0800, Kui-Feng Lee wrote:
[...]

> > So, it appears that with shadow types users would have more or less
> > convenient way to disable / enable related BPF programs
> > (the references to programs are available, but reference counting
> >   would have to be implemented by user using some additional data
> >   structure, if needed).
> > 
> > I don't see a way to reconcile shadow types with this autoload/autocreate toggling
> > => my last two patches would have to be dropped.
> 
> How about to update autoload according to the value of autocreate of
> maps before loading the programs? For example, update autoload in
> bpf_map__init_kern_struct_ops()?

This can be done, but it would have to be a separate pass:
first scanning all maps and setting up reference counters for programs,
then scanning all programs and disabling those unused.
I can do that in v2, thank you for the suggestion.

Still, this overlaps a bit with shadow types.
Do you have an idea if such auto-toggling would be helpful from libbpf
users point of view?

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Kui-Feng Lee]

On 2/27/24 15:30, Eduard Zingerman wrote:
> On Tue, 2024-02-27 at 15:16 -0800, Kui-Feng Lee wrote:
> [...]
> 
>>> So, it appears that with shadow types users would have more or less
>>> convenient way to disable / enable related BPF programs
>>> (the references to programs are available, but reference counting
>>>    would have to be implemented by user using some additional data
>>>    structure, if needed).
>>>
>>> I don't see a way to reconcile shadow types with this autoload/autocreate toggling
>>> => my last two patches would have to be dropped.
>>
>> How about to update autoload according to the value of autocreate of
>> maps before loading the programs? For example, update autoload in
>> bpf_map__init_kern_struct_ops()?
> 
> This can be done, but it would have to be a separate pass:
> first scanning all maps and setting up reference counters for programs,
> then scanning all programs and disabling those unused.
> I can do that in v2, thank you for the suggestion.
> 
> Still, this overlaps a bit with shadow types.
> Do you have an idea if such auto-toggling would be helpful from libbpf
> users point of view?

For me, it is useful. For minor adjustments, shadow types is easier and
simpler; for example, change a flag or a function.  But, the features
presented here are useful when a user want to switch between several
very different configurations. They may not want to change a large
number of fields.

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Eduard Zingerman]

On Tue, 2024-02-27 at 15:40 -0800, Kui-Feng Lee wrote:
[...]

> For me, it is useful. For minor adjustments, shadow types is easier and
> simpler; for example, change a flag or a function.  But, the features
> presented here are useful when a user want to switch between several
> very different configurations. They may not want to change a large
> number of fields.

Great, thank you for reviewing the patch-set.
I'll wait a bit for additional comments and post v2 tomorrow.

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Kui-Feng Lee]

On 2/27/24 15:30, Eduard Zingerman wrote:
> On Tue, 2024-02-27 at 15:16 -0800, Kui-Feng Lee wrote:
> [...]
> 
>>> So, it appears that with shadow types users would have more or less
>>> convenient way to disable / enable related BPF programs
>>> (the references to programs are available, but reference counting
>>>    would have to be implemented by user using some additional data
>>>    structure, if needed).
>>>
>>> I don't see a way to reconcile shadow types with this autoload/autocreate toggling
>>> => my last two patches would have to be dropped.
>>
>> How about to update autoload according to the value of autocreate of
>> maps before loading the programs? For example, update autoload in
>> bpf_map__init_kern_struct_ops()?
> 
> This can be done, but it would have to be a separate pass:
> first scanning all maps and setting up reference counters for programs,
> then scanning all programs and disabling those unused.
> I can do that in v2, thank you for the suggestion.
> 

It only has to scan once with an additional flag.
The value of the autoload of a prog should be
true if its autoload_user_set is false and autocreate of any one of
struct_ops maps pointing to the prog is true.

Let's say the flag is autoload_autocreate.
In bpf_map__init_kern_struct_ops(), it has to check
prog->autoload_user_set, and do prog->autoload |= map->autocreate if
prog->autoload_user_set is false and autoload_autocreate is true. Do 
prog->autoload = map->autocreate if autoload_autocreate is false I think 
it is enough, right?

if (!prog->autoload_user_set) {
     if (!prog->autoload_autocreate)
         prog->autoload = map->autocreate;
     else
         prog->autoload |= map->autocreate;
     prog->autoload_autocreate = true;
}

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Eduard Zingerman]

On Tue, 2024-02-27 at 16:12 -0800, Kui-Feng Lee wrote:
[...]

> It only has to scan once with an additional flag.
> The value of the autoload of a prog should be
> true if its autoload_user_set is false and autocreate of any one of
> struct_ops maps pointing to the prog is true.
> 
> Let's say the flag is autoload_autocreate.
> In bpf_map__init_kern_struct_ops(), it has to check
> prog->autoload_user_set, and do prog->autoload |= map->autocreate if
> prog->autoload_user_set is false and autoload_autocreate is true. Do 
> prog->autoload = map->autocreate if autoload_autocreate is false I think 
> it is enough, right?
> 
> if (!prog->autoload_user_set) {
>      if (!prog->autoload_autocreate)
>          prog->autoload = map->autocreate;
>      else
>          prog->autoload |= map->autocreate;
>      prog->autoload_autocreate = true;
> }

Since the full thing is moved to load phase I was hoping to make do
w/o changes to struct bpf_program. To have it more contained.
E.g. the code below apperas to work (needs more testing):

--- 8< -------------------------------------

static int bpf_object__adjust_struct_ops_autoload(struct bpf_object *obj)
{
	struct bpf_program *prog;
	struct bpf_map *map;
	int i, j, k, vlen;
	struct {
		__u8 autoload:1;
		/* only change autoload for programs that are
		 * referenced from some struct_ops maps
		 */
		__u8 used:1;
	} *refs;

	refs = calloc(obj->nr_programs, sizeof(refs[0]));
	if (!refs)
		return -ENOMEM;

	for (i = 0; i < obj->nr_maps; i++) {
		map = &obj->maps[i];
		if (!bpf_map__is_struct_ops(map))
			continue;

		vlen = btf_vlen(map->st_ops->type);
		for (j = 0; j < vlen; ++j) {
			prog = map->st_ops->progs[j];
			if (!prog)
				continue;

			k = prog - obj->programs;
			refs[k].used = true;
			refs[k].autoload |= map->autocreate;
		}
	}

	for (i = 0; i < obj->nr_programs; ++i) {
		prog = &obj->programs[i];
		if (prog->autoload_user_set || !refs[i].used)
			continue;

		prog->autoload = refs[i].autoload;
	}

	free(refs);
	return 0;
}

------------------------------------- >8 ---

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Martin KaFai Lau]

On 2/27/24 12:45 PM, Eduard Zingerman wrote:
> Make bpf_map__set_autocreate() for struct_ops maps toggle autoload
> state for referenced programs.
> 
> E.g. for the BPF code below:
> 
>      SEC("struct_ops/test_1") int BPF_PROG(foo) { ... }
>      SEC("struct_ops/test_2") int BPF_PROG(bar) { ... }
> 
>      SEC(".struct_ops.link")
>      struct test_ops___v1 A = {
>          .foo = (void *)foo
>      };
> 
>      SEC(".struct_ops.link")
>      struct test_ops___v2 B = {
>          .foo = (void *)foo,
>          .bar = (void *)bar,
>      };
> 
> And the following libbpf API calls:
> 
>      bpf_map__set_autocreate(skel->maps.A, true);
>      bpf_map__set_autocreate(skel->maps.B, false);
> 
> The autoload would be enabled for program 'foo' and disabled for
> program 'bar'.
> 
> Do not apply such toggling if program autoload state is set by a call
> to bpf_program__set_autoload().
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>   tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index b39d3f2898a1..1ea3046724f8 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -446,13 +446,18 @@ struct bpf_program {
>   	struct bpf_object *obj;
>   
>   	int fd;
> -	bool autoload;
> +	bool autoload:1;
> +	bool autoload_user_set:1;
>   	bool autoattach;
>   	bool sym_global;
>   	bool mark_btf_static;
>   	enum bpf_prog_type type;
>   	enum bpf_attach_type expected_attach_type;
>   	int exception_cb_idx;
> +	/* total number of struct_ops maps with autocreate == true
> +	 * that reference this program
> +	 */
> +	__u32 struct_ops_refs;

Instead of adding struct_ops_refs and autoload_user_set,

for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking 
prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at 
load time and is only set if it is used by at least one autocreate map, if I 
read patch 2 & 3 correctly.

Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used 
by one struct_ops map with autocreate == true.

If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be 
loaded even the autoload is set. If bpf prog is used in a struct_ops map and its 
autoload is set to false, the struct_ops map will be in broken state. Thus, 
prog->autoload does not fit very well with struct_ops prog and may as well 
depend on whether the struct_ops prog is used by a struct_ops map alone?

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Eduard Zingerman]

On Tue, 2024-02-27 at 18:10 -0800, Martin KaFai Lau wrote:
[...]

> Instead of adding struct_ops_refs and autoload_user_set,
> 
> for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking 
> prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at 
> load time and is only set if it is used by at least one autocreate map, if I 
> read patch 2 & 3 correctly.
> 
> Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used 
> by one struct_ops map with autocreate == true.
> 
> If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be 
> loaded even the autoload is set. If bpf prog is used in a struct_ops map and its 
> autoload is set to false, the struct_ops map will be in broken state. Thus, 
> prog->autoload does not fit very well with struct_ops prog and may as well 
> depend on whether the struct_ops prog is used by a struct_ops map alone?

This makes sense.
The drawback is that introspection capability to query which programs
would be loaded is lost, maybe that is not a big deal.
It could be put back by adding an ugly loop iterating over all maps in
bpf_program__autoload() for struct_ops programs.

I think I'll post v2 with changes you suggest and see what others have to say.

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Andrii Nakryiko]

On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 2/27/24 12:45 PM, Eduard Zingerman wrote:
> > Make bpf_map__set_autocreate() for struct_ops maps toggle autoload
> > state for referenced programs.
> >
> > E.g. for the BPF code below:
> >
> >      SEC("struct_ops/test_1") int BPF_PROG(foo) { ... }
> >      SEC("struct_ops/test_2") int BPF_PROG(bar) { ... }
> >
> >      SEC(".struct_ops.link")
> >      struct test_ops___v1 A = {
> >          .foo = (void *)foo
> >      };
> >
> >      SEC(".struct_ops.link")
> >      struct test_ops___v2 B = {
> >          .foo = (void *)foo,
> >          .bar = (void *)bar,
> >      };
> >
> > And the following libbpf API calls:
> >
> >      bpf_map__set_autocreate(skel->maps.A, true);
> >      bpf_map__set_autocreate(skel->maps.B, false);
> >
> > The autoload would be enabled for program 'foo' and disabled for
> > program 'bar'.
> >
> > Do not apply such toggling if program autoload state is set by a call
> > to bpf_program__set_autoload().
> >
> > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > ---
> >   tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++-
> >   1 file changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index b39d3f2898a1..1ea3046724f8 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -446,13 +446,18 @@ struct bpf_program {
> >       struct bpf_object *obj;
> >
> >       int fd;
> > -     bool autoload;
> > +     bool autoload:1;
> > +     bool autoload_user_set:1;
> >       bool autoattach;
> >       bool sym_global;
> >       bool mark_btf_static;
> >       enum bpf_prog_type type;
> >       enum bpf_attach_type expected_attach_type;
> >       int exception_cb_idx;
> > +     /* total number of struct_ops maps with autocreate == true
> > +      * that reference this program
> > +      */
> > +     __u32 struct_ops_refs;
>
> Instead of adding struct_ops_refs and autoload_user_set,
>
> for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking
> prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at
> load time and is only set if it is used by at least one autocreate map, if I
> read patch 2 & 3 correctly.
>
> Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used
> by one struct_ops map with autocreate == true.
>
> If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be
> loaded even the autoload is set. If bpf prog is used in a struct_ops map and its
> autoload is set to false, the struct_ops map will be in broken state. Thus,

We can easily detect this condition and report meaningful error.

> prog->autoload does not fit very well with struct_ops prog and may as well
> depend on whether the struct_ops prog is used by a struct_ops map alone?

I think it's probably fine from a usability standpoint to disable
loading the BPF program if its struct_ops map was explicitly set to
not auto-create. It's a bit of deviation from other program types, but
in practice this logic will make it easier for users.

One question I have, though, is whether we should report as an error a
stand-alone struct_ops BPF program that is not used from any
struct_ops map? Or should we load it nevertheless? Or should we
silently not load it?

I feel like silently not loading is the worst behavior here. So either
loading it anyway or reporting an error would be my preference,
probably.

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Eduard Zingerman]

On Wed, 2024-02-28 at 15:55 -0800, Andrii Nakryiko wrote:
> On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:

[...]

> > Instead of adding struct_ops_refs and autoload_user_set,
> > 
> > for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking
> > prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at
> > load time and is only set if it is used by at least one autocreate map, if I
> > read patch 2 & 3 correctly.
> > 
> > Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used
> > by one struct_ops map with autocreate == true.
> > 
> > If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be
> > loaded even the autoload is set. If bpf prog is used in a struct_ops map and its
> > autoload is set to false, the struct_ops map will be in broken state. Thus,
> 
> We can easily detect this condition and report meaningful error.
> 
> > prog->autoload does not fit very well with struct_ops prog and may as well
> > depend on whether the struct_ops prog is used by a struct_ops map alone?
> 
> I think it's probably fine from a usability standpoint to disable
> loading the BPF program if its struct_ops map was explicitly set to
> not auto-create. It's a bit of deviation from other program types, but
> in practice this logic will make it easier for users.
> 
> One question I have, though, is whether we should report as an error a
> stand-alone struct_ops BPF program that is not used from any
> struct_ops map? Or should we load it nevertheless? Or should we
> silently not load it?
> 
> I feel like silently not loading is the worst behavior here. So either
> loading it anyway or reporting an error would be my preference,
> probably.

The following properties of the struct_ops program are set based on
the corresponding struct_ops map:
- attach_btf_id - BTF id of the kernel struct_ops type;
- expected_attach_type - member index of function pointer inside
  the kernel type.

No corresponding map means above fields are not set,
means program fails to load with error report.

So I think it is fine to try loading such programs w/o any additional
processing.

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Andrii Nakryiko]

On Wed, Feb 28, 2024 at 4:04 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2024-02-28 at 15:55 -0800, Andrii Nakryiko wrote:
> > On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> [...]
>
> > > Instead of adding struct_ops_refs and autoload_user_set,
> > >
> > > for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking
> > > prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at
> > > load time and is only set if it is used by at least one autocreate map, if I
> > > read patch 2 & 3 correctly.
> > >
> > > Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used
> > > by one struct_ops map with autocreate == true.
> > >
> > > If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be
> > > loaded even the autoload is set. If bpf prog is used in a struct_ops map and its
> > > autoload is set to false, the struct_ops map will be in broken state. Thus,
> >
> > We can easily detect this condition and report meaningful error.
> >
> > > prog->autoload does not fit very well with struct_ops prog and may as well
> > > depend on whether the struct_ops prog is used by a struct_ops map alone?
> >
> > I think it's probably fine from a usability standpoint to disable
> > loading the BPF program if its struct_ops map was explicitly set to
> > not auto-create. It's a bit of deviation from other program types, but
> > in practice this logic will make it easier for users.
> >
> > One question I have, though, is whether we should report as an error a
> > stand-alone struct_ops BPF program that is not used from any
> > struct_ops map? Or should we load it nevertheless? Or should we
> > silently not load it?
> >
> > I feel like silently not loading is the worst behavior here. So either
> > loading it anyway or reporting an error would be my preference,
> > probably.
>
> The following properties of the struct_ops program are set based on
> the corresponding struct_ops map:
> - attach_btf_id - BTF id of the kernel struct_ops type;
> - expected_attach_type - member index of function pointer inside
>   the kernel type.
>
> No corresponding map means above fields are not set,
> means program fails to load with error report.
>
> So I think it is fine to try loading such programs w/o any additional
> processing.

But Martin is proposing to *not load* programs if their attach_btf_id
is not set. Which is why I'm asking if we should distinguish
situations where a BPF program is stand-alone (never was referenced
from struct_ops map) vs auto-disabling it because struct_ops map that
it was referenced from was explicitly disabled.

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Martin KaFai Lau]

On 2/28/24 3:55 PM, Andrii Nakryiko wrote:
> On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 2/27/24 12:45 PM, Eduard Zingerman wrote:
>>> Make bpf_map__set_autocreate() for struct_ops maps toggle autoload
>>> state for referenced programs.
>>>
>>> E.g. for the BPF code below:
>>>
>>>       SEC("struct_ops/test_1") int BPF_PROG(foo) { ... }
>>>       SEC("struct_ops/test_2") int BPF_PROG(bar) { ... }
>>>
>>>       SEC(".struct_ops.link")
>>>       struct test_ops___v1 A = {
>>>           .foo = (void *)foo
>>>       };
>>>
>>>       SEC(".struct_ops.link")
>>>       struct test_ops___v2 B = {
>>>           .foo = (void *)foo,
>>>           .bar = (void *)bar,
>>>       };
>>>
>>> And the following libbpf API calls:
>>>
>>>       bpf_map__set_autocreate(skel->maps.A, true);
>>>       bpf_map__set_autocreate(skel->maps.B, false);
>>>
>>> The autoload would be enabled for program 'foo' and disabled for
>>> program 'bar'.
>>>
>>> Do not apply such toggling if program autoload state is set by a call
>>> to bpf_program__set_autoload().
>>>
>>> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
>>> ---
>>>    tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 34 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>> index b39d3f2898a1..1ea3046724f8 100644
>>> --- a/tools/lib/bpf/libbpf.c
>>> +++ b/tools/lib/bpf/libbpf.c
>>> @@ -446,13 +446,18 @@ struct bpf_program {
>>>        struct bpf_object *obj;
>>>
>>>        int fd;
>>> -     bool autoload;
>>> +     bool autoload:1;
>>> +     bool autoload_user_set:1;
>>>        bool autoattach;
>>>        bool sym_global;
>>>        bool mark_btf_static;
>>>        enum bpf_prog_type type;
>>>        enum bpf_attach_type expected_attach_type;
>>>        int exception_cb_idx;
>>> +     /* total number of struct_ops maps with autocreate == true
>>> +      * that reference this program
>>> +      */
>>> +     __u32 struct_ops_refs;
>>
>> Instead of adding struct_ops_refs and autoload_user_set,
>>
>> for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking
>> prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at
>> load time and is only set if it is used by at least one autocreate map, if I
>> read patch 2 & 3 correctly.
>>
>> Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used
>> by one struct_ops map with autocreate == true.
>>
>> If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be
>> loaded even the autoload is set. If bpf prog is used in a struct_ops map and its
>> autoload is set to false, the struct_ops map will be in broken state. Thus,
> 
> We can easily detect this condition and report meaningful error.
> 
>> prog->autoload does not fit very well with struct_ops prog and may as well
>> depend on whether the struct_ops prog is used by a struct_ops map alone?
> 
> I think it's probably fine from a usability standpoint to disable
> loading the BPF program if its struct_ops map was explicitly set to
> not auto-create. It's a bit of deviation from other program types, but
> in practice this logic will make it easier for users.
> 
> One question I have, though, is whether we should report as an error a
> stand-alone struct_ops BPF program that is not used from any
> struct_ops map? Or should we load it nevertheless? Or should we
> silently not load it?

I think the libbpf could report an error if the prog is not used in any 
struct_ops map at the source code level, not sure if it is useful.

However, it probably should not report error if that struct_ops map (where the 
prog is resided) does not have autocreate set to true.

If a BPF program is not used in any struct_ops map, it cannot be loaded anyway 
because the prog->attach_btf_id is not set. If libbpf tries to load the prog, 
the kernel will reject it also. I think it may be a question on whether it is 
the user intention of not loading the prog if the prog is not used in any 
struct_ops map. I tend to think it is the user intention of not loading it in 
this case.

SEC("struct_ops/test1")
int BPF_PROG(test1) { ... }

SEC("struct_ops/test2")
int BPF_PROG(test2) { ... }

SEC("?.struct_ops.link") struct some_ops___v1 a = { .test1 = test1 }
SEC("?.struct_ops.link") struct some_ops___v2 b = { .test1 = test1,
						   .test2 = test2, }

In the above, the userspace may try to load with a newer some_ops___v2 first, 
failed and then try a lower version some_ops___v1 and then succeeded. The test2 
prog will not be used and not expected to be loaded.

> 
> I feel like silently not loading is the worst behavior here. So either
> loading it anyway or reporting an error would be my preference,
> probably.

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Andrii Nakryiko]

On Wed, Feb 28, 2024 at 4:25 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 2/28/24 3:55 PM, Andrii Nakryiko wrote:
> > On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
> >>
> >> On 2/27/24 12:45 PM, Eduard Zingerman wrote:
> >>> Make bpf_map__set_autocreate() for struct_ops maps toggle autoload
> >>> state for referenced programs.
> >>>
> >>> E.g. for the BPF code below:
> >>>
> >>>       SEC("struct_ops/test_1") int BPF_PROG(foo) { ... }
> >>>       SEC("struct_ops/test_2") int BPF_PROG(bar) { ... }
> >>>
> >>>       SEC(".struct_ops.link")
> >>>       struct test_ops___v1 A = {
> >>>           .foo = (void *)foo
> >>>       };
> >>>
> >>>       SEC(".struct_ops.link")
> >>>       struct test_ops___v2 B = {
> >>>           .foo = (void *)foo,
> >>>           .bar = (void *)bar,
> >>>       };
> >>>
> >>> And the following libbpf API calls:
> >>>
> >>>       bpf_map__set_autocreate(skel->maps.A, true);
> >>>       bpf_map__set_autocreate(skel->maps.B, false);
> >>>
> >>> The autoload would be enabled for program 'foo' and disabled for
> >>> program 'bar'.
> >>>
> >>> Do not apply such toggling if program autoload state is set by a call
> >>> to bpf_program__set_autoload().
> >>>
> >>> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> >>> ---
> >>>    tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++-
> >>>    1 file changed, 34 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >>> index b39d3f2898a1..1ea3046724f8 100644
> >>> --- a/tools/lib/bpf/libbpf.c
> >>> +++ b/tools/lib/bpf/libbpf.c
> >>> @@ -446,13 +446,18 @@ struct bpf_program {
> >>>        struct bpf_object *obj;
> >>>
> >>>        int fd;
> >>> -     bool autoload;
> >>> +     bool autoload:1;
> >>> +     bool autoload_user_set:1;
> >>>        bool autoattach;
> >>>        bool sym_global;
> >>>        bool mark_btf_static;
> >>>        enum bpf_prog_type type;
> >>>        enum bpf_attach_type expected_attach_type;
> >>>        int exception_cb_idx;
> >>> +     /* total number of struct_ops maps with autocreate == true
> >>> +      * that reference this program
> >>> +      */
> >>> +     __u32 struct_ops_refs;
> >>
> >> Instead of adding struct_ops_refs and autoload_user_set,
> >>
> >> for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking
> >> prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at
> >> load time and is only set if it is used by at least one autocreate map, if I
> >> read patch 2 & 3 correctly.
> >>
> >> Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used
> >> by one struct_ops map with autocreate == true.
> >>
> >> If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be
> >> loaded even the autoload is set. If bpf prog is used in a struct_ops map and its
> >> autoload is set to false, the struct_ops map will be in broken state. Thus,
> >
> > We can easily detect this condition and report meaningful error.
> >
> >> prog->autoload does not fit very well with struct_ops prog and may as well
> >> depend on whether the struct_ops prog is used by a struct_ops map alone?
> >
> > I think it's probably fine from a usability standpoint to disable
> > loading the BPF program if its struct_ops map was explicitly set to
> > not auto-create. It's a bit of deviation from other program types, but
> > in practice this logic will make it easier for users.
> >
> > One question I have, though, is whether we should report as an error a
> > stand-alone struct_ops BPF program that is not used from any
> > struct_ops map? Or should we load it nevertheless? Or should we
> > silently not load it?
>
> I think the libbpf could report an error if the prog is not used in any
> struct_ops map at the source code level, not sure if it is useful.
>
> However, it probably should not report error if that struct_ops map (where the
> prog is resided) does not have autocreate set to true.
>
> If a BPF program is not used in any struct_ops map, it cannot be loaded anyway
> because the prog->attach_btf_id is not set. If libbpf tries to load the prog,
> the kernel will reject it also. I think it may be a question on whether it is
> the user intention of not loading the prog if the prog is not used in any
> struct_ops map. I tend to think it is the user intention of not loading it in
> this case.
>
> SEC("struct_ops/test1")
> int BPF_PROG(test1) { ... }
>
> SEC("struct_ops/test2")
> int BPF_PROG(test2) { ... }
>
> SEC("?.struct_ops.link") struct some_ops___v1 a = { .test1 = test1 }
> SEC("?.struct_ops.link") struct some_ops___v2 b = { .test1 = test1,
>                                                    .test2 = test2, }
>
> In the above, the userspace may try to load with a newer some_ops___v2 first,
> failed and then try a lower version some_ops___v1 and then succeeded. The test2
> prog will not be used and not expected to be loaded.
>

Yes, it's all sane in the above example. But imagine a stand-alone
struct_ops program with no SEC(".struct_ops") at all:


SEC("struct_ops/test1")
int BPF_PROG(test1) { ... }

/* nothing else */

Currently this will fail, right?

And with your proposal it will succeed without actually even
attempting to load the BPF program. Or am I misunderstanding?


> >
> > I feel like silently not loading is the worst behavior here. So either
> > loading it anyway or reporting an error would be my preference,
> > probably.
>

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Martin KaFai Lau]

On 2/28/24 4:30 PM, Andrii Nakryiko wrote:
> On Wed, Feb 28, 2024 at 4:25 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 2/28/24 3:55 PM, Andrii Nakryiko wrote:
>>> On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>>>
>>>> On 2/27/24 12:45 PM, Eduard Zingerman wrote:
>>>>> Make bpf_map__set_autocreate() for struct_ops maps toggle autoload
>>>>> state for referenced programs.
>>>>>
>>>>> E.g. for the BPF code below:
>>>>>
>>>>>        SEC("struct_ops/test_1") int BPF_PROG(foo) { ... }
>>>>>        SEC("struct_ops/test_2") int BPF_PROG(bar) { ... }
>>>>>
>>>>>        SEC(".struct_ops.link")
>>>>>        struct test_ops___v1 A = {
>>>>>            .foo = (void *)foo
>>>>>        };
>>>>>
>>>>>        SEC(".struct_ops.link")
>>>>>        struct test_ops___v2 B = {
>>>>>            .foo = (void *)foo,
>>>>>            .bar = (void *)bar,
>>>>>        };
>>>>>
>>>>> And the following libbpf API calls:
>>>>>
>>>>>        bpf_map__set_autocreate(skel->maps.A, true);
>>>>>        bpf_map__set_autocreate(skel->maps.B, false);
>>>>>
>>>>> The autoload would be enabled for program 'foo' and disabled for
>>>>> program 'bar'.
>>>>>
>>>>> Do not apply such toggling if program autoload state is set by a call
>>>>> to bpf_program__set_autoload().
>>>>>
>>>>> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
>>>>> ---
>>>>>     tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++-
>>>>>     1 file changed, 34 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>>>> index b39d3f2898a1..1ea3046724f8 100644
>>>>> --- a/tools/lib/bpf/libbpf.c
>>>>> +++ b/tools/lib/bpf/libbpf.c
>>>>> @@ -446,13 +446,18 @@ struct bpf_program {
>>>>>         struct bpf_object *obj;
>>>>>
>>>>>         int fd;
>>>>> -     bool autoload;
>>>>> +     bool autoload:1;
>>>>> +     bool autoload_user_set:1;
>>>>>         bool autoattach;
>>>>>         bool sym_global;
>>>>>         bool mark_btf_static;
>>>>>         enum bpf_prog_type type;
>>>>>         enum bpf_attach_type expected_attach_type;
>>>>>         int exception_cb_idx;
>>>>> +     /* total number of struct_ops maps with autocreate == true
>>>>> +      * that reference this program
>>>>> +      */
>>>>> +     __u32 struct_ops_refs;
>>>>
>>>> Instead of adding struct_ops_refs and autoload_user_set,
>>>>
>>>> for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking
>>>> prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at
>>>> load time and is only set if it is used by at least one autocreate map, if I
>>>> read patch 2 & 3 correctly.
>>>>
>>>> Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used
>>>> by one struct_ops map with autocreate == true.
>>>>
>>>> If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be
>>>> loaded even the autoload is set. If bpf prog is used in a struct_ops map and its
>>>> autoload is set to false, the struct_ops map will be in broken state. Thus,
>>>
>>> We can easily detect this condition and report meaningful error.
>>>
>>>> prog->autoload does not fit very well with struct_ops prog and may as well
>>>> depend on whether the struct_ops prog is used by a struct_ops map alone?
>>>
>>> I think it's probably fine from a usability standpoint to disable
>>> loading the BPF program if its struct_ops map was explicitly set to
>>> not auto-create. It's a bit of deviation from other program types, but
>>> in practice this logic will make it easier for users.
>>>
>>> One question I have, though, is whether we should report as an error a
>>> stand-alone struct_ops BPF program that is not used from any
>>> struct_ops map? Or should we load it nevertheless? Or should we
>>> silently not load it?
>>
>> I think the libbpf could report an error if the prog is not used in any
>> struct_ops map at the source code level, not sure if it is useful.
>>
>> However, it probably should not report error if that struct_ops map (where the
>> prog is resided) does not have autocreate set to true.
>>
>> If a BPF program is not used in any struct_ops map, it cannot be loaded anyway
>> because the prog->attach_btf_id is not set. If libbpf tries to load the prog,
>> the kernel will reject it also. I think it may be a question on whether it is
>> the user intention of not loading the prog if the prog is not used in any
>> struct_ops map. I tend to think it is the user intention of not loading it in
>> this case.
>>
>> SEC("struct_ops/test1")
>> int BPF_PROG(test1) { ... }
>>
>> SEC("struct_ops/test2")
>> int BPF_PROG(test2) { ... }
>>
>> SEC("?.struct_ops.link") struct some_ops___v1 a = { .test1 = test1 }
>> SEC("?.struct_ops.link") struct some_ops___v2 b = { .test1 = test1,
>>                                                     .test2 = test2, }
>>
>> In the above, the userspace may try to load with a newer some_ops___v2 first,
>> failed and then try a lower version some_ops___v1 and then succeeded. The test2
>> prog will not be used and not expected to be loaded.
>>
> 
> Yes, it's all sane in the above example. But imagine a stand-alone
> struct_ops program with no SEC(".struct_ops") at all:
> 
> 
> SEC("struct_ops/test1")
> int BPF_PROG(test1) { ... }
> 
> /* nothing else */
> 
> Currently this will fail, right?
> 
> And with your proposal it will succeed without actually even
> attempting to load the BPF program. Or am I misunderstanding?

Yep, currently it should fail.

Agree that we need to distinguish this case and prog->attach_btf_id is not 
enough. This probably can be tracked in collect_st_ops_relos at the open phase.

Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Eduard Zingerman]

On Wed, 2024-02-28 at 16:37 -0800, Martin KaFai Lau wrote:
[...]

> > Yes, it's all sane in the above example. But imagine a stand-alone
> > struct_ops program with no SEC(".struct_ops") at all:
> > 
> > 
> > SEC("struct_ops/test1")
> > int BPF_PROG(test1) { ... }
> > 
> > /* nothing else */
> > 
> > Currently this will fail, right?
> > 
> > And with your proposal it will succeed without actually even
> > attempting to load the BPF program. Or am I misunderstanding?
> 
> Yep, currently it should fail.
> 
> Agree that we need to distinguish this case and prog->attach_btf_id is not 
> enough. This probably can be tracked in collect_st_ops_relos at the open phase.

collect_st_ops_relos() should work, I'll add a flag to track this.

[PATCH bpf-next v1 8/8] selftests/bpf: tests for struct_ops autoload/autocreate toggling

[Eduard Zingerman]

Verify automatic interaction between struct_ops map autocreate flag
and struct_ops programs autoload flags.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 .../bpf/prog_tests/struct_ops_autocreate.c    | 65 +++++++++++++++++--
 1 file changed, 61 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
index b21b10f94fc2..ace296aae8c4 100644
--- a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
+++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
@@ -46,10 +46,6 @@ static void can_load_partial_object(void)
 	if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open_opts"))
 		return;
 
-	err = bpf_program__set_autoload(skel->progs.test_2, false);
-	if (!ASSERT_OK(err, "bpf_program__set_autoload"))
-		goto cleanup;
-
 	err = bpf_map__set_autocreate(skel->maps.testmod_2, false);
 	if (!ASSERT_OK(err, "bpf_map__set_autocreate"))
 		goto cleanup;
@@ -70,8 +66,69 @@ static void can_load_partial_object(void)
 	struct_ops_autocreate__destroy(skel);
 }
 
+static void autoload_toggles(void)
+{
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+	struct bpf_map *testmod_1, *testmod_2;
+	struct bpf_program *test_1, *test_2;
+	struct struct_ops_autocreate *skel;
+
+	skel = struct_ops_autocreate__open_opts(&opts);
+	if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open_opts"))
+		return;
+
+	testmod_1 = skel->maps.testmod_1;
+	testmod_2 = skel->maps.testmod_2;
+	test_1 = skel->progs.test_1;
+	test_2 = skel->progs.test_2;
+
+	/* testmod_1 on, testmod_2 on */
+	ASSERT_TRUE(bpf_program__autoload(test_1), "autoload(test_1) #1");
+	ASSERT_TRUE(bpf_program__autoload(test_2), "autoload(test_2) #1");
+
+	/* testmod_1 off, testmod_2 on */
+	bpf_map__set_autocreate(testmod_1, false);
+	ASSERT_TRUE(bpf_program__autoload(test_1), "autoload(test_1) #2");
+	ASSERT_TRUE(bpf_program__autoload(test_2), "autoload(test_2) #2");
+
+	/* testmod_1 off, testmod_2 off,
+	 * setting same state several times should not confuse internal state.
+	 */
+	bpf_map__set_autocreate(testmod_2, false);
+	bpf_map__set_autocreate(testmod_2, false);
+	ASSERT_FALSE(bpf_program__autoload(test_1), "autoload(test_1) #3");
+	ASSERT_FALSE(bpf_program__autoload(test_2), "autoload(test_2) #3");
+
+	/* testmod_1 on, testmod_2 off */
+	bpf_map__set_autocreate(testmod_1, true);
+	bpf_map__set_autocreate(testmod_1, true);
+	ASSERT_TRUE(bpf_program__autoload(test_1), "autoload(test_1) #4");
+	ASSERT_FALSE(bpf_program__autoload(test_2), "autoload(test_2) #4");
+
+	/* testmod_1 on, testmod_2 on */
+	bpf_map__set_autocreate(testmod_2, true);
+	ASSERT_TRUE(bpf_program__autoload(test_1), "autoload(test_1) #5");
+	ASSERT_TRUE(bpf_program__autoload(test_2), "autoload(test_2) #5");
+
+	/* testmod_1 on, testmod_2 off */
+	bpf_map__set_autocreate(testmod_2, false);
+	ASSERT_TRUE(bpf_program__autoload(test_1), "autoload(test_1) #6");
+	ASSERT_FALSE(bpf_program__autoload(test_2), "autoload(test_2) #6");
+
+	/* setting autoload manually overrides automatic toggling */
+	bpf_program__set_autoload(test_2, false);
+	/* testmod_1 on, testmod_2 off */
+	bpf_map__set_autocreate(testmod_2, true);
+	ASSERT_TRUE(bpf_program__autoload(test_1), "autoload(test_1) #7");
+	ASSERT_FALSE(bpf_program__autoload(test_2), "autoload(test_2) #7");
+
+	struct_ops_autocreate__destroy(skel);
+}
+
 void serial_test_struct_ops_autocreate(void)
 {
+	if (test__start_subtest("autoload_toggles"))
+		autoload_toggles();
 	if (test__start_subtest("cant_load_full_object"))
 		cant_load_full_object();
 	if (test__start_subtest("can_load_partial_object"))
-- 
2.43.0

Re: [PATCH bpf-next v1 8/8] selftests/bpf: tests for struct_ops autoload/autocreate toggling

[David Vernet]

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

On Tue, Feb 27, 2024 at 10:45:56PM +0200, Eduard Zingerman wrote:
> Verify automatic interaction between struct_ops map autocreate flag
> and struct_ops programs autoload flags.
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  .../bpf/prog_tests/struct_ops_autocreate.c    | 65 +++++++++++++++++--
>  1 file changed, 61 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> index b21b10f94fc2..ace296aae8c4 100644
> --- a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> +++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> @@ -46,10 +46,6 @@ static void can_load_partial_object(void)
>  	if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open_opts"))
>  		return;
>  
> -	err = bpf_program__set_autoload(skel->progs.test_2, false);
> -	if (!ASSERT_OK(err, "bpf_program__set_autoload"))
> -		goto cleanup;
> -
>  	err = bpf_map__set_autocreate(skel->maps.testmod_2, false);
>  	if (!ASSERT_OK(err, "bpf_map__set_autocreate"))
>  		goto cleanup;
> @@ -70,8 +66,69 @@ static void can_load_partial_object(void)
>  	struct_ops_autocreate__destroy(skel);
>  }
>  
> +static void autoload_toggles(void)
> +{
> +	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
> +	struct bpf_map *testmod_1, *testmod_2;
> +	struct bpf_program *test_1, *test_2;
> +	struct struct_ops_autocreate *skel;
> +
> +	skel = struct_ops_autocreate__open_opts(&opts);
> +	if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open_opts"))
> +		return;
> +
> +	testmod_1 = skel->maps.testmod_1;
> +	testmod_2 = skel->maps.testmod_2;
> +	test_1 = skel->progs.test_1;
> +	test_2 = skel->progs.test_2;
> +
> +	/* testmod_1 on, testmod_2 on */
> +	ASSERT_TRUE(bpf_program__autoload(test_1), "autoload(test_1) #1");
> +	ASSERT_TRUE(bpf_program__autoload(test_2), "autoload(test_2) #1");
> +
> +	/* testmod_1 off, testmod_2 on */
> +	bpf_map__set_autocreate(testmod_1, false);
> +	ASSERT_TRUE(bpf_program__autoload(test_1), "autoload(test_1) #2");
> +	ASSERT_TRUE(bpf_program__autoload(test_2), "autoload(test_2) #2");
> +
> +	/* testmod_1 off, testmod_2 off,
> +	 * setting same state several times should not confuse internal state.
> +	 */
> +	bpf_map__set_autocreate(testmod_2, false);
> +	bpf_map__set_autocreate(testmod_2, false);

Duplicate line

> +	ASSERT_FALSE(bpf_program__autoload(test_1), "autoload(test_1) #3");
> +	ASSERT_FALSE(bpf_program__autoload(test_2), "autoload(test_2) #3");
> +
> +	/* testmod_1 on, testmod_2 off */
> +	bpf_map__set_autocreate(testmod_1, true);
> +	bpf_map__set_autocreate(testmod_1, true);

Here as well

> +	ASSERT_TRUE(bpf_program__autoload(test_1), "autoload(test_1) #4");
> +	ASSERT_FALSE(bpf_program__autoload(test_2), "autoload(test_2) #4");
> +
> +	/* testmod_1 on, testmod_2 on */
> +	bpf_map__set_autocreate(testmod_2, true);
> +	ASSERT_TRUE(bpf_program__autoload(test_1), "autoload(test_1) #5");
> +	ASSERT_TRUE(bpf_program__autoload(test_2), "autoload(test_2) #5");
> +
> +	/* testmod_1 on, testmod_2 off */
> +	bpf_map__set_autocreate(testmod_2, false);
> +	ASSERT_TRUE(bpf_program__autoload(test_1), "autoload(test_1) #6");
> +	ASSERT_FALSE(bpf_program__autoload(test_2), "autoload(test_2) #6");
> +
> +	/* setting autoload manually overrides automatic toggling */
> +	bpf_program__set_autoload(test_2, false);
> +	/* testmod_1 on, testmod_2 off */
> +	bpf_map__set_autocreate(testmod_2, true);
> +	ASSERT_TRUE(bpf_program__autoload(test_1), "autoload(test_1) #7");
> +	ASSERT_FALSE(bpf_program__autoload(test_2), "autoload(test_2) #7");
> +
> +	struct_ops_autocreate__destroy(skel);
> +}
> +
>  void serial_test_struct_ops_autocreate(void)
>  {
> +	if (test__start_subtest("autoload_toggles"))
> +		autoload_toggles();
>  	if (test__start_subtest("cant_load_full_object"))
>  		cant_load_full_object();
>  	if (test__start_subtest("can_load_partial_object"))
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH bpf-next v1 8/8] selftests/bpf: tests for struct_ops autoload/autocreate toggling

[Eduard Zingerman]

On Wed, 2024-02-28 at 12:36 -0600, David Vernet wrote:
[...]

> > +	/* testmod_1 off, testmod_2 off,
> > +	 * setting same state several times should not confuse internal state.
> > +	 */
> > +	bpf_map__set_autocreate(testmod_2, false);
> > +	bpf_map__set_autocreate(testmod_2, false);
> 
> Duplicate line

This was intended, to check that reference counter is not over/under-saturated.

But this does not matter, as the test would not be in v2 if I take
Martin's suggestion in [0].

[0] https://lore.kernel.org/bpf/1e95162a-a8d7-44e6-bc63-999df8cae987@linux.dev/

[...]