libbpf: type suffixes and autocreate flag for struct_ops maps

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 <[email protected]>

---
  tools/lib/bpf/libbpf.c | 22 +++++++++++++++++-----
  1 file changed, 17 insertions(+), 5 deletions(-)

tools/lib/bpf/libbpf.c +13 -5

index 01f407591a92..abe663927013 100644
@@ -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;

tools/lib/bpf/libbpf.c +4 -0

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

Quoted 9 lines

tools/lib/bpf/libbpf.c +2 -1

@@ -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;
  	}
 

tools/lib/bpf/libbpf.c find_struct_ops_kern_types(struct bpf_object *obj, const char *tname, · line 1015

Quoted 3 lines

tools/lib/bpf/libbpf.c +0 -0

  	*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?

Quoted 9 lines

tools/lib/bpf/libbpf.c +4 -0

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

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 <[email protected]>

---
 tools/lib/bpf/libbpf.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

tools/lib/bpf/libbpf.c +17 -5

index 01f407591a92..abe663927013 100644
@@ -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)

Signature · 1 line

2.43.0

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.

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

Quoted 3 lines

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

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

Quoted 3 lines

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

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.

tools/lib/bpf/libbpf.c +7 -2

index abe663927013..c239b75d5816 100644
@@ -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;

tools/lib/bpf/libbpf.c +4 -0

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

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 <[email protected]>

---
 tools/lib/bpf/libbpf.c | 44 ++++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

tools/lib/bpf/libbpf.c +21 -23

index abe663927013..c239b75d5816 100644
@@ -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;
 	}
 

Signature · 1 line

2.43.0

Quoted 6 lines · 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

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.

Quoted 8 lines · David Vernet

Quoted 6 lines · 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

Quoted 2 lines

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.

Quoted 9 lines

So if we have for example:

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

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

Quoted 4 lines

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

From pov of kernel BTF there is only one 'struct some_ops'.

Quoted 5 lines

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?

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 <[email protected]>

---
 tools/lib/bpf/libbpf.c | 44 ++++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

tools/lib/bpf/libbpf.c +7 -2

index abe663927013..c239b75d5816 100644
@@ -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;

tools/lib/bpf/libbpf.c +4 -22

+                                       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

Quoted 20 lines

tools/lib/bpf/libbpf.c +14 -2

@@ -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
+                        */

tools/lib/bpf/libbpf.c static int bpf_map__init_kern_struct_ops(struct bpf_map *map) · line 1153

Quoted 3 lines

tools/lib/bpf/libbpf.c +3 -0

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

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

Quoted 20 lines

Quoted 20 lines

tools/lib/bpf/libbpf.c +14 -2

@@ -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
+                        */

Quoted 8 lines

tools/lib/bpf/libbpf.c static int bpf_map__init_kern_struct_ops(struct bpf_map *map) · line 1153

Quoted 3 lines

tools/lib/bpf/libbpf.c +3 -0

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

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 <[email protected]>

---
 tools/lib/bpf/libbpf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

tools/lib/bpf/libbpf.c +2 -2

index c239b75d5816..b39d3f2898a1 100644
@@ -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;

Signature · 1 line

2.43.0

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 <[email protected]>

---
 .../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(-)

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +1 -0

index 66787e99ba1b..0d8437e05f64 100644
@@ -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.
 	 */

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c +23 -9

index 8d833f0c7580..7bc80d2755f1 100644
@@ -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);

tools/testing/selftests/bpf/progs/struct_ops_module.c +20 -1

index b78746b3cef3..e91426dc51af 100644
@@ -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,
+};

Signature · 1 line

2.43.0

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 <[email protected]>

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

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +24 -0

index 0d8437e05f64..69f5eb9ad546 100644
@@ -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)

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h +4 -0

index c3b0cf788f9f..3183fff7f246 100644
@@ -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 */

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c new file +8 -0

new file mode 100644
index 000000000000..9c689db4b05b
@@ -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;

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c new file +24 -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();
+}

tools/testing/selftests/bpf/progs/bad_struct_ops.c new file +4 -0

new file mode 100644
index 000000000000..9c103afbfdb1
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>

tools/testing/selftests/bpf/progs/bad_struct_ops.c new file +3 -0

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

Signature · 1 line

2.43.0

Quoted 7 lines

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c new file +7 -0

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

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c line 18

Quoted 4 lines

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c new file +4 -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).

Quoted 15 lines

tools/testing/selftests/bpf/progs/bad_struct_ops.c new file +9 -0

new file mode 100644
index 000000000000..9c103afbfdb1
@@ -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";
+

tools/testing/selftests/bpf/progs/bad_struct_ops.c line 14

Quoted 5 lines

tools/testing/selftests/bpf/progs/bad_struct_ops.c new file +5 -0

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

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

Quoted 7 lines

Quoted 7 lines

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c new file +7 -0

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

Quoted 8 lines

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c line 18

Quoted 4 lines

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c new file +4 -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).

[...]

Quoted 15 lines

Quoted 15 lines

tools/testing/selftests/bpf/progs/bad_struct_ops.c new file +9 -0

new file mode 100644
index 000000000000..9c103afbfdb1
@@ -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";
+

Quoted 8 lines

tools/testing/selftests/bpf/progs/bad_struct_ops.c line 14

Quoted 5 lines

tools/testing/selftests/bpf/progs/bad_struct_ops.c new file +5 -0

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

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 <[email protected]>

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

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +24 -0

index 0d8437e05f64..69f5eb9ad546 100644
@@ -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)

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h +4 -0

index c3b0cf788f9f..3183fff7f246 100644
@@ -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 */

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c new file +16 -0

new file mode 100644
index 000000000000..9c689db4b05b
@@ -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;
+

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c new file +4 -0

+{
+       if (test__start_subtest("test_bad_struct_ops"))
+               test_bad_struct_ops();
+}

tools/testing/selftests/bpf/progs/bad_struct_ops.c new file +17 -1

new file mode 100644
index 000000000000..9c103afbfdb1
@@ -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

Quoted 15 lines

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c new file +15 -0

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

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c line 26

Quoted 4 lines

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c new file +4 -0

+       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

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c line 38

Quoted 13 lines

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c new file +12 -0

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

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

Quoted 15 lines

Quoted 15 lines

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c new file +15 -0

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

Quoted 8 lines

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c line 26

Quoted 4 lines

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c new file +4 -0

+       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

Quoted 15 lines

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c line 38

Quoted 13 lines

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c new file +12 -0

+       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

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

Signed-off-by Eduard Zingerman <[email protected]>

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

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c new file +16 -0

new file mode 100644
index 000000000000..b21b10f94fc2
@@ -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;
+

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c new file +15 -0

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

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c new file +27 -0

+	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();
+}

tools/testing/selftests/bpf/progs/struct_ops_autocreate.c new file +42 -0

new file mode 100644
index 000000000000..294d48bb8e3c
@@ -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
+};

Signature · 1 line

2.43.0

Quoted 33 lines · Eduard Zingerman

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

Signed-off-by Eduard Zingerman <[email protected]>

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

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c new file +16 -0

new file mode 100644
index 000000000000..b21b10f94fc2
@@ -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;
+

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c line 26

Quoted 10 lines

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c new file +10 -0

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

Quoted 22 lines

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c new file +22 -0

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

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c line 51

Quoted 3 lines

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c new file +3 -0

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

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c line 26

Quoted 7 lines

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c new file +7 -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.

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

Signed-off-by Eduard Zingerman <[email protected]>

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

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c new file +30 -0

new file mode 100644
index 000000000000..b21b10f94fc2
@@ -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;

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c new file +23 -0

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

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c new file +6 -0

+{
+       if (test__start_subtest("cant_load_full_object"))
+               cant_load_full_object();
+       if (test__start_subtest("can_load_partial_object"))
+               can_load_partial_object();
+}

tools/testing/selftests/bpf/progs/struct_ops_autocreate.c new file +28 -0

new file mode 100644
index 000000000000..294d48bb8e3c
@@ -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 {

tools/testing/selftests/bpf/progs/struct_ops_autocreate.c new file +4 -1

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

2.43.0

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c line 40

Quoted 3 lines

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c new file +3 -0

+static void can_load_partial_object(void)
+{
+       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);

nit: prefer LIBBPF_OPTS() over DECLARE_LIBBPF_OPTS()

Quoted 1 line

+void serial_test_struct_ops_autocreate(void)

same as in the previous patch, why serial?

Quoted 1 line

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

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

Quoted 5 lines

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c line 40

Quoted 3 lines

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c new file +3 -0

+static void can_load_partial_object(void)
+{
+       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);

nit: prefer LIBBPF_OPTS() over DECLARE_LIBBPF_OPTS()

Ok

[...]

Quoted 3 lines

Quoted 1 line

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

[...]

Quoted 4 lines

Quoted 1 line

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

[...]

Quoted 2 lines

Also, what would happen when two tests would set struct_ops for same attachment point?

I'm not sure, Martin?

Quoted 1 line · Eduard Zingerman

Quoted 9 lines · Andrii Nakryiko

Quoted 4 lines

Quoted 1 line

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

Quoted 2 lines · Andrii Nakryiko

Quoted 1 line · Eduard Zingerman

[...]

Quoted 11 lines

Quoted 9 lines · Andrii Nakryiko

Quoted 4 lines

Quoted 1 line

+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

Quoted 7 lines

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.

Quoted 7 lines

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

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 <[email protected]>

---
  tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++-
  1 file changed, 34 insertions(+), 1 deletion(-)

tools/lib/bpf/libbpf.c +18 -1

index b39d3f2898a1..1ea3046724f8 100644
@@ -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)

tools/lib/bpf/libbpf.c +6 -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;
  	}
 

Quoted 16 lines

tools/lib/bpf/libbpf.c +12 -0

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

tools/lib/bpf/libbpf.c static int bpf_get_map_info_from_fdinfo(int fd, struct bpf_map_info *info) · line 4538

Quoted 10 lines

tools/lib/bpf/libbpf.c +10 -0

+			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/[email protected]/

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

Quoted 33 lines

Quoted 16 lines

tools/lib/bpf/libbpf.c +12 -0

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

tools/lib/bpf/libbpf.c static int bpf_get_map_info_from_fdinfo(int fd, struct bpf_map_info *info) · line 4538

Quoted 10 lines

tools/lib/bpf/libbpf.c +10 -0

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

Quoted 2 lines

[1]
https://lore.kernel.org/all/[email protected]/

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?

Quoted 6 lines

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

Quoted 2 lines

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()?

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

Quoted 6 lines

Quoted 6 lines

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

Quoted 6 lines

Quoted 2 lines

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.

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

Quoted 9 lines

Quoted 8 lines

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.

Quoted 3 lines

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

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 <[email protected]>

---
  tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++-
  1 file changed, 34 insertions(+), 1 deletion(-)

tools/lib/bpf/libbpf.c +2 -1

index b39d3f2898a1..1ea3046724f8 100644
@@ -446,13 +446,18 @@ struct bpf_program {
  	struct bpf_object *obj;
 
  	int fd;
-	bool autoload;
+	bool autoload:1;
+	bool autoload_user_set:1;

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?

Quoted 46 lines · 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 <[email protected]>

---
  tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++-
  1 file changed, 34 insertions(+), 1 deletion(-)

tools/lib/bpf/libbpf.c +2 -1

index b39d3f2898a1..1ea3046724f8 100644
@@ -446,13 +446,18 @@ struct bpf_program {
      struct bpf_object *obj;
 
      int fd;
-     bool autoload;
+     bool autoload:1;
+     bool autoload_user_set:1;

tools/lib/bpf/libbpf.c struct bpf_program { · line 460

Quoted 10 lines

tools/lib/bpf/libbpf.c +4 -0

      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?

On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <[email protected]> wrote:

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 1 line · Andrii Nakryiko

On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <[email protected]> wrote:

[...]

Quoted 24 lines

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

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.

Quoted 66 lines · Martin KaFai Lau

Quoted 46 lines · 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 <[email protected]>

---
   tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++-
   1 file changed, 34 insertions(+), 1 deletion(-)

tools/lib/bpf/libbpf.c +2 -1

index b39d3f2898a1..1ea3046724f8 100644
@@ -446,13 +446,18 @@ struct bpf_program {
       struct bpf_object *obj;
 
       int fd;
-     bool autoload;
+     bool autoload:1;
+     bool autoload_user_set:1;

tools/lib/bpf/libbpf.c struct bpf_program { · line 460

Quoted 10 lines

tools/lib/bpf/libbpf.c +4 -0

       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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 84 lines · Andrii Nakryiko

Quoted 66 lines · Martin KaFai Lau

Quoted 46 lines · 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 <[email protected]>

---
   tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++-
   1 file changed, 34 insertions(+), 1 deletion(-)

tools/lib/bpf/libbpf.c +2 -1

index b39d3f2898a1..1ea3046724f8 100644
@@ -446,13 +446,18 @@ struct bpf_program {
       struct bpf_object *obj;
 
       int fd;
-     bool autoload;
+     bool autoload:1;
+     bool autoload_user_set:1;

tools/lib/bpf/libbpf.c struct bpf_program { · line 460

Quoted 10 lines

tools/lib/bpf/libbpf.c +4 -0

       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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 110 lines · Martin KaFai Lau

Quoted 84 lines · Andrii Nakryiko

Quoted 66 lines · Martin KaFai Lau

Quoted 46 lines · 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 <[email protected]>

---
    tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++-
    1 file changed, 34 insertions(+), 1 deletion(-)

tools/lib/bpf/libbpf.c +2 -1

index b39d3f2898a1..1ea3046724f8 100644
@@ -446,13 +446,18 @@ struct bpf_program {
        struct bpf_object *obj;
 
        int fd;
-     bool autoload;
+     bool autoload:1;
+     bool autoload_user_set:1;

tools/lib/bpf/libbpf.c struct bpf_program { · line 460

Quoted 10 lines

tools/lib/bpf/libbpf.c +4 -0

        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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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) { ... }

Quoted 8 lines

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?

Quoted 11 lines

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?

Quoted 2 lines

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.

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

Signed-off-by Eduard Zingerman <[email protected]>

---
 .../bpf/prog_tests/struct_ops_autocreate.c    | 65 +++++++++++++++++--
 1 file changed, 61 insertions(+), 4 deletions(-)

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c +20 -4

index b21b10f94fc2..ace296aae8c4 100644
@@ -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");
+

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c +25 -0

+	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"))

Signature · 1 line

2.43.0