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.

Acked-by David Vernet <[email protected]>

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

---

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

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

index 6c2979f1b471..e2a4c409980b 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,

@@ -958,8 +958,12 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
        const struct btf_member *kern_data_member;
        struct btf *btf;
        __s32 kern_vtype_id, kern_type_id;
+       char tname[256];
        __u32 i;
 
+       snprintf(tname, sizeof(tname), "%.*s",
+                (int)bpf_core_essential_name_len(tname_raw), tname_raw);
+
        kern_type_id = find_ksym_btf_id(obj, tname, BTF_KIND_STRUCT,
                                        &btf, mod_btf);
        if (kern_type_id < 0) {
--

2.43.0

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 | 49 ++++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 23 deletions(-)

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

Acked-by David Vernet <[email protected]>

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

index 2c0cb72bc7a4..25c452c20d7d 100644

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

                        bpf_map_prepare_vdata(&obj->maps[i]);
 
        return 0;
--

2.43.0

tools/lib/bpf/libbpf.c static int bpf_object__init_kern_struct_ops_maps(struct bpf_object *obj) · line 1212

Quoted 10 lines

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

index 2c0cb72bc7a4..25c452c20d7d 100644
@@ -1209,7 +1209,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)

tbh, I'd keep them as separate checks (they check very different aspects, so feels appropriate to not combine them in one check)

tools/lib/bpf/libbpf.c static int bpf_object_prepare_struct_ops(struct bpf_object *obj) · line 8139

Quoted 9 lines

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

                        continue;
 
                err = bpf_map__init_kern_struct_ops(map);

@@ -8136,7 +8136,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)

especially here, this becomes a bit convoluted

how about we make it a bit more verbose, but also straightforward

map = &obj->maps[i];
if (!bpf_map__is_struct_ops(map))

    continue;
if (!map->autocreate)
    continue;

bpf_map_prepare_vdata(...)

Several test_progs tests already capture libbpf log in order to check for some expected output, e.g bpf_tcp_ca.c, kfunc_dynptr_param.c, log_buf.c and a few others.

This commit provides a, hopefully, simple API to capture libbpf log w/o necessity to define new print callback in each test:

/* Creates a global memstream capturing all output passed to
* libbpf_print_fn.
* Returns 0 on success, negative value on failure.
* On failure the description is printed using PRINT_FAIL and
* current test case is marked as fail.
*/
int start_libbpf_log_capture(void)

/* Destroys global memstream created by start_libbpf_log_capture().
* Returns a pointer to captured data which has to be freed.
* Returned buffer is null terminated.
*/
char *stop_libbpf_log_capture(void)

The intended usage is as follows:

    if (start_libbpf_log_capture())
            return;
    use_libbpf();
    char *log = stop_libbpf_log_capture();
    ASSERT_HAS_SUBSTR(log, "... expected ...", "expected some message");
    free(log);

As a safety measure, free(start_libbpf_log_capture()) is invoked in the epilogue of the test_progs.c:run_one_test().

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

---
 tools/testing/selftests/bpf/test_progs.c | 57 ++++++++++++++++++++++++
 tools/testing/selftests/bpf/test_progs.h |  3 ++
 2 files changed, 60 insertions(+)

tools/testing/selftests/bpf/test_progs.c +7 -0

+/* Destroys global memstream created by start_libbpf_log_capture().
+ * Returns a pointer to captured data which has to be freed.
+ * Returned buffer is null terminated.
+ */
+char *stop_libbpf_log_capture(void)
+{
+       char *buf;

tools/testing/selftests/bpf/test_progs.c +2 -0

+
        if (env.verbosity < VERBOSE_VERY && level == LIBBPF_DEBUG)
                return 0;
        vfprintf(stdout, format, args);

@@ -1081,6 +1137,7 @@ static void run_one_test(int test_num)
                cleanup_cgroup_environment();
 
        stdio_restore();
+       free(stop_libbpf_log_capture());
 
        dump_test_log(test, state, false, false, NULL);
 }

tools/testing/selftests/bpf/test_progs.h +3 -1

index 80df51244886..0ba5a20b19ba 100644
@@ -397,6 +397,9 @@ int test__join_cgroup(const char *path);
                system(cmd);                                            \
        })
 
+int start_libbpf_log_capture(void);
+char *stop_libbpf_log_capture(void);
+
 static inline __u64 ptr_to_u64(const void *ptr)
 {
        return (__u64) (unsigned long) ptr;
--

2.43.0

tools/testing/selftests/bpf/test_progs.c static const struct argp_option opts[] = { · line 731

Quoted 13 lines

tools/testing/selftests/bpf/test_progs.c +13 -0

+char *stop_libbpf_log_capture(void)
+{
+       char *buf;
+
+       if (!libbpf_capture_stream)
+               return NULL;
+
+       fputc(0, libbpf_capture_stream);
+       fclose(libbpf_capture_stream);
+       libbpf_capture_stream = NULL;
+       /* get 'buf' after fclose(), see open_memstream() documentation */
+       buf = libbpf_output_capture.buf;
+       bzero(&libbpf_output_capture, sizeof(libbpf_output_capture));

please use memset():

$ rg -w memset | wc -l
355
$ rg -w bzero | wc -l
12

Quoted 8 lines

tools/testing/selftests/bpf/test_progs.c +5 -0

+       return buf;
+}
+
 static int libbpf_print_fn(enum libbpf_print_level level,
                           const char *format, va_list args)
 {
+       if (libbpf_capture_stream) {
+               va_list args2;

tools/testing/selftests/bpf/test_progs.c static const struct argp_option opts[] = { · line 743

Quoted 4 lines

tools/testing/selftests/bpf/test_progs.c +4 -0

+
+               va_copy(args2, args);
+               vfprintf(libbpf_capture_stream, format, args2);
+       }

should we take into account verbosity settings? capturing LIBBPF_DEBUG logs probably isn't very useful (but will make debugging harder, probably)

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 | 35 +++++++++++++++++++
 .../selftests/bpf/progs/bad_struct_ops.c      | 25 +++++++++++++
 4 files changed, 88 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 +0 -0

index b9ff88e3d463..2de7e80dbb4b 100644
@@ -610,6 +610,29 @@ struct bpf_struct_ops bpf_bpf_testmod_ops = {
        .owner = THIS_MODULE,
 };
 

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

new file mode 100644
index 000000000000..9f5dbefa0dd9
@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+#include "bad_struct_ops.skel.h"
+
+static void invalid_prog_reuse(void)
+{
+       struct bad_struct_ops *skel;
+       char *log = NULL;
+       int err;
+

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

+       ASSERT_HAS_SUBSTR(log,
+               "struct_ops init_kern testmod_2 func ptr test_1: invalid reuse of prog test_1",
+               "expected init_kern message");
+
+cleanup:
+       free(log);
+       bad_struct_ops__destroy(skel);
+}
+
+void test_bad_struct_ops(void)
+{
+       if (test__start_subtest("invalid_prog_reuse"))
+               invalid_prog_reuse();
+}

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

Quoted 9 lines

---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 24 +++++++++++++
 .../selftests/bpf/bpf_testmod/bpf_testmod.h   |  4 +++
 .../selftests/bpf/prog_tests/bad_struct_ops.c | 35 +++++++++++++++++++
 .../selftests/bpf/progs/bad_struct_ops.c      | 25 +++++++++++++
 4 files changed, 88 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

CI reports kernel crashes, so please check that, but overall things look good:

Acked-by Andrii Nakryiko <[email protected]>

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    | 76 +++++++++++++++++++
  .../bpf/progs/struct_ops_autocreate.c         | 42 ++++++++++
  2 files changed, 118 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 +7 -0

new file mode 100644
index 000000000000..883f938d518c
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+#include "struct_ops_autocreate.skel.h"
+
+static void cant_load_full_object(void)
+{

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

+	/* 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 that map.
+	 */
+	err = struct_ops_autocreate__load(skel);
+	log = stop_libbpf_log_capture();
+	if (!ASSERT_ERR(err, "struct_ops_autocreate__load"))
+		goto cleanup;
+
+	ASSERT_HAS_SUBSTR(log, "libbpf: struct_ops init_kern", "init_kern message");
+	ASSERT_EQ(err, -ENOTSUP, "errno should be ENOTSUP");
+
+cleanup:
+	free(log);
+	struct_ops_autocreate__destroy(skel);
+}

Quoted 17 lines

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

new file mode 100644
index 000000000000..883f938d518c
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+#include "struct_ops_autocreate.skel.h"
+
+static void cant_load_full_object(void)
+{
+	struct struct_ops_autocreate *skel;
+	char *log;
+	int err;
+

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

Quoted 6 lines

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

+	skel = struct_ops_autocreate__open();
+	if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open"))
+		return;
+
+	if (start_libbpf_log_capture())
+		goto cleanup;

nit: This cleanup path triggers freeing of uninitialized log pointer.

Automatically select which struct_ops programs to load depending on which struct_ops maps are selected for automatic creation. 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'.

During load, for each struct_ops program P, referenced from some struct_ops map M:
- set P.autoload = true if M.autocreate is true for some M;
- set P.autoload = false if M.autocreate is false for all M;
- don't change P.autoload, if P is not referenced from any map.

Do this after bpf_object__init_kern_struct_ops_maps() to make sure that shadow vars assignment is done.

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

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

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

index 25c452c20d7d..ce39ae34fec0 100644

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

+                       prog = map->st_ops->progs[j];
+                       if (!prog)
+                               continue;
+
+                       k = prog - obj->programs;
+                       if (k < 0 || k > obj->nr_programs)
 

= ? But a clever trick, I like it!

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

+                               continue;
+
+                       refs[k].used_in_struct_ops_map = true;
+                       refs[k].any_map_autocreate |= map->autocreate;
+               }
+       }
+
+       for (i = 0; i < obj->nr_programs; ++i)
+               if (refs[i].used_in_struct_ops_map)
+                       obj->programs[i].autoload = refs[i].any_map_autocreate;
+
+       free(refs);
+       return 0;
+}
+
 /* Init the map's fields that depend on kern_btf */
 static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
 {

@@ -8163,6 +8210,7 @@ static int bpf_object_load(struct bpf_object *obj, int extra_log_level, const ch
        err = err ? : bpf_object__resolve_externs(obj, obj->kconfig);
        err = err ? : bpf_object__sanitize_maps(obj);
        err = err ? : bpf_object__init_kern_struct_ops_maps(obj);
+       err = err ? : bpf_object__adjust_struct_ops_autoload(obj);
        err = err ? : bpf_object__relocate(obj, obj->btf_custom_path ? : target_btf_path);
        err = err ? : bpf_object__sanitize_and_load_btf(obj);
        err = err ? : bpf_object__create_maps(obj);
--

2.43.0

bit masks just for this to save a few bytes?...

but generally, how about we keep all this much dumber:

for (each program) {
    int use_cnt = 0;
    int should_load = false;

    if (!is_struct_ops_program)
        continue;
    for (each map) {
        if (!is_struct_ops_map)
            continue;
        for (each prog entry in map->st_ops->progs[]) {
            if (prog != slot_prog)
                continue;
            use_cnt++;
            if (map->autocreate)
               should_load = true;
        }
    }
    if (use_cnt)
        prog->autocreate = should_load;
}

Sure it does a few more iterations, but we avoid unnecessary allocation/cleanup and keep things literally working how you describe in your comment.

WDYT?

Quoted 8 lines

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

+                       prog = map->st_ops->progs[j];
+                       if (!prog)
+                               continue;
+
+                       k = prog - obj->programs;
+                       if (k < 0 || k > obj->nr_programs)

= ? But a clever trick, I like it!

Check that autocreate flags of struct_ops map cause autoload of struct_ops corresponding programs:
- when struct_ops program is referenced only from a map for which autocreate is set to false, that program should not be loaded;
- when struct_ops program with autoload == false is set to be used from a map with autocreate == true using shadow var, that program should be loaded;
- when struct_ops program is not referenced from any map object load should fail.

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

---
 .../selftests/bpf/prog_tests/bad_struct_ops.c | 32 ++++++++++++++
 .../bpf/prog_tests/struct_ops_autocreate.c    | 43 +++++++++++++++++--
 .../selftests/bpf/progs/bad_struct_ops2.c     | 14 ++++++
 .../bpf/progs/struct_ops_autocreate2.c        | 32 ++++++++++++++
 4 files changed, 117 insertions(+), 4 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/bad_struct_ops2.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_autocreate2.c

tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c +32 -0

index 9f5dbefa0dd9..6a707213e46b 100644
@@ -2,6 +2,7 @@
 
 #include <test_progs.h>
 #include "bad_struct_ops.skel.h"
+#include "bad_struct_ops2.skel.h"
 
 static void invalid_prog_reuse(void)
 {

@@ -28,8 +29,39 @@ static void invalid_prog_reuse(void)
        bad_struct_ops__destroy(skel);
 }
 
+static void unused_program(void)
+{
+       struct bad_struct_ops2 *skel;
+       char *log = NULL;
+       int err;
+
+       skel = bad_struct_ops2__open();
+       if (!ASSERT_OK_PTR(skel, "bad_struct_ops2__open"))
+               return;
+
+       /* struct_ops programs not referenced from any maps are open
+        * with autoload set to true.
+        */
+       ASSERT_TRUE(bpf_program__autoload(skel->progs.foo), "foo autoload == true");
+
+       if (start_libbpf_log_capture())
+               goto cleanup;
+
+       err = bad_struct_ops2__load(skel);
+       ASSERT_ERR(err, "bad_struct_ops2__load should fail");
+       log = stop_libbpf_log_capture();
+       ASSERT_HAS_SUBSTR(log, "prog 'foo': failed to load",
+                         "message about 'foo' failing to load");
+
+cleanup:
+       free(log);
+       bad_struct_ops2__destroy(skel);
+}
+
 void test_bad_struct_ops(void)
 {
        if (test__start_subtest("invalid_prog_reuse"))
                invalid_prog_reuse();
+       if (test__start_subtest("unused_program"))
+               unused_program();
 }

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

index 883f938d518c..35910cbb9ca4 100644
@@ -2,6 +2,7 @@
 
 #include <test_progs.h>
 #include "struct_ops_autocreate.skel.h"
+#include "struct_ops_autocreate2.skel.h"
 
 static void cant_load_full_object(void)
 {

@@ -43,10 +44,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;

@@ -67,10 +64,48 @@ static void can_load_partial_object(void)
        struct_ops_autocreate__destroy(skel);
 }
 
+/* Swap test_mod1->test_1 program from 'bar' to 'foo' using shadow vars.
+ * test_mod1 load should enable autoload for 'foo'.
+ */
+static void autoload_and_shadow_vars(void)
+{
+       struct struct_ops_autocreate2 *skel = NULL;
+       struct bpf_link *link = NULL;
+       int err;
+
+       skel = struct_ops_autocreate2__open();
+       if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open_opts"))
+               return;
+
+       ASSERT_FALSE(bpf_program__autoload(skel->progs.foo), "foo default autoload");

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

+       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 */
+       err = ASSERT_EQ(skel->bss->test_1_result, 42, "test_1_result");
+
+cleanup:
+       bpf_link__destroy(link);
+       struct_ops_autocreate2__destroy(skel);
+}
+
 void 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();
+       if (test__start_subtest("autoload_and_shadow_vars"))
+               autoload_and_shadow_vars();
 }

tools/testing/selftests/bpf/progs/bad_struct_ops2.c new file +14 -0

new file mode 100644
index 000000000000..64a95f6be86d
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+char _license[] SEC("license") = "GPL";
+
+/* This is an unused struct_ops program, it lacks corresponding
+ * struct_ops map, which provides attachment information.
+ * W/o additional configuration attempt to load such
+ * BPF object file would fail.
+ */
+SEC("struct_ops/foo")
+void foo(void) {}

tools/testing/selftests/bpf/progs/struct_ops_autocreate2.c new file +32 -1

new file mode 100644
index 000000000000..6049d9c902d3
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.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(foo)
+{
+       test_1_result = 42;
+       return 0;
+}
+
+SEC("?struct_ops/test_1")
+int BPF_PROG(bar)
+{
+       test_1_result = 24;
+       return 0;
+}
+
+struct bpf_testmod_ops {
+       int (*test_1)(void);
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops testmod_1 = {
+       .test_1 = (void *)bar
+};
--

2.43.0

Quoted 18 lines

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

+/* Swap test_mod1->test_1 program from 'bar' to 'foo' using shadow vars.
+ * test_mod1 load should enable autoload for 'foo'.
+ */
+static void autoload_and_shadow_vars(void)
+{
+       struct struct_ops_autocreate2 *skel = NULL;
+       struct bpf_link *link = NULL;
+       int err;
+
+       skel = struct_ops_autocreate2__open();
+       if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open_opts"))
+               return;
+
+       ASSERT_FALSE(bpf_program__autoload(skel->progs.foo), "foo default autoload");
+       ASSERT_FALSE(bpf_program__autoload(skel->progs.bar), "bar default autoload");
+
+       /* loading map testmod_1 would switch foo's autoload to true */
+       skel->struct_ops.testmod_1->test_1 = skel->progs.foo;

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c static void can_load_partial_object(void) · line 90

Quoted 6 lines

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

+
+       err = struct_ops_autocreate2__load(skel);
+       if (ASSERT_OK(err, "struct_ops_autocreate__load"))
+               goto cleanup;
+
+

let's assert autoload state here as well?

The next patch would add two new section names for struct_ops maps. To make working with multiple struct_ops sections more convenient:
- remove fields like elf_state->st_ops_{shndx,link_shndx};
- mark section descriptions hosting struct_ops as elf_sec_desc->sec_type == SEC_ST_OPS;

After these changes struct_ops sections could be processed uniformly by iterating bpf_object->efile.secs entries.

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

---
 tools/lib/bpf/libbpf.c | 62 ++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

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

@@ -1268,7 +1266,7 @@ static int bpf_object__init_kern_struct_ops_maps(struct bpf_object *obj)
 }
 
 static int init_struct_ops_maps(struct bpf_object *obj, const char *sec_name,
-                               int shndx, Elf_Data *data, __u32 map_flags)
+                               int shndx, Elf_Data *data)
 {
        const struct btf_type *type, *datasec;
        const struct btf_var_secinfo *vsi;

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

                map->st_ops = calloc(1, sizeof(*map->st_ops));
                if (!map->st_ops)

Quoted 10 lines · Eduard Zingerman

The next patch would add two new section names for struct_ops maps. To make working with multiple struct_ops sections more convenient:
- remove fields like elf_state->st_ops_{shndx,link_shndx};
- mark section descriptions hosting struct_ops as elf_sec_desc->sec_type == SEC_ST_OPS;

After these changes struct_ops sections could be processed uniformly by iterating bpf_object->efile.secs entries.

Quoted 5 lines

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

---
 tools/lib/bpf/libbpf.c | 62 ++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

Makes sense and a good generalization!

tools/lib/bpf/libbpf.c static int init_struct_ops_maps(struct bpf_object *obj, const char *sec_name, · line 1332

Quoted 7 lines

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

@@ -1330,7 +1328,8 @@ static int init_struct_ops_maps(struct bpf_object *obj, const char *sec_name,
                map->def.key_size = sizeof(int);
                map->def.value_size = type->size;
                map->def.max_entries = 1;
-               map->def.map_flags = map_flags;
+               map->def.map_flags = strcmp(sec_name, STRUCT_OPS_LINK_SEC) == 0
+                                  ? BPF_F_LINK : 0;

Does it fit in under 100 characters? if yes, please keep on a single line

Allow using two new section names for struct_ops maps:
- SEC("?.struct_ops")
- SEC("?.struct_ops.link")

To specify maps that have bpf_map->autocreate == false after open.

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

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

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

 enum libbpf_map_type {
        LIBBPF_MAP_UNSPEC,

@@ -1324,6 +1326,15 @@ static int init_struct_ops_maps(struct bpf_object *obj, const char *sec_name,
                        return -ENOMEM;
                map->btf_value_type_id = type_id;
 
+               /* Follow same convention as for programs autoload:
+                * SEC("?.struct_ops") means map is not created by default.
+                */
+               if (sec_name[0] == '?') {
+                       map->autocreate = false;
+                       /* from now on forget there was ? in section name */
+                       sec_name++;
+               }
+
                map->def.type = BPF_MAP_TYPE_STRUCT_OPS;
                map->def.key_size = sizeof(int);
                map->def.value_size = type->size;

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

                            strcmp(name, ".rel" MAPS_ELF_SEC)) {
                                pr_info("elf: skipping relo section(%d) %s for section(%d) %s\n",
                                        idx, name, targ_sec_idx,
--

2.43.0

tools/lib/bpf/libbpf.c static int bpf_object__elf_collect(struct bpf_object *obj) · line 3704

Quoted 8 lines

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

@@ -3688,7 +3699,9 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
                                sec_desc->shdr = sh;
                                sec_desc->data = data;
                        } else if (strcmp(name, STRUCT_OPS_SEC) == 0 ||
-                                  strcmp(name, STRUCT_OPS_LINK_SEC) == 0) {
+                                  strcmp(name, STRUCT_OPS_LINK_SEC) == 0 ||
+                                  strcmp(name, OPT_STRUCT_OPS_SEC) == 0 ||
+                                  strcmp(name, OPT_STRUCT_OPS_LINK_SEC) == 0) {

I'd do just

strcmp(name, "?" STRUCT_OPS_SEC) == 0 ||
strcmp(name, "?" STRUCT_OPS_LINK_SEC) == 0 ||

Optional struct_ops maps are defined using question mark at the start of the section name, e.g.:

    SEC("?.struct_ops")
    struct test_ops optional_map = { ... };

This commit teaches libbpf to detect if kernel allows '?' prefix in datasec names, and if it doesn't then to rewrite such names by removing '?' prefix and adding ".optional" suffix. For example:

    DATASEC ?.struct_ops -> DATASEC .struct_ops.optional

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

---
 tools/lib/bpf/features.c        | 22 ++++++++++++++++++++++
 tools/lib/bpf/libbpf.c          | 30 +++++++++++++++++++++++++++++-
 tools/lib/bpf/libbpf_internal.h |  2 ++
 3 files changed, 53 insertions(+), 1 deletion(-)

tools/lib/bpf/features.c +22 -0

index 6b0738ad7063..4e783cc7fc4b 100644
@@ -147,6 +147,25 @@ static int probe_kern_btf_datasec(int token_fd)
                                             strs, sizeof(strs), token_fd));
 }
 
+static int probe_kern_btf_qmark_datasec(int token_fd)
+{
+       static const char strs[] = "\0x\0?.data";
+       /* static int a; */
+       __u32 types[] = {
+               /* int */
+               BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),  /* [1] */
+               /* VAR x */                                     /* [2] */
+               BTF_TYPE_ENC(1, BTF_INFO_ENC(BTF_KIND_VAR, 0, 0), 1),
+               BTF_VAR_STATIC,
+               /* DATASEC ?.data */                            /* [3] */
+               BTF_TYPE_ENC(3, BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 1), 4),
+               BTF_VAR_SECINFO_ENC(2, 0, 4),
+       };
+
+       return probe_fd(libbpf__load_raw_btf((char *)types, sizeof(types),
+                                            strs, sizeof(strs), token_fd));
+}
+
 static int probe_kern_btf_float(int token_fd)
 {
        static const char strs[] = "\0float";

@@ -534,6 +553,9 @@ static struct kern_feature_desc {
        [FEAT_ARG_CTX_TAG] = {
                "kernel-side __arg_ctx tag", probe_kern_arg_ctx_tag,
        },
+       [FEAT_BTF_QMARK_DATASEC] = {
+               "BTF DATASEC names starting from '?'", probe_kern_btf_qmark_datasec,
+       },
 };
 
 bool feat_supported(struct kern_feature_cache *cache, enum kern_feature_id feat_id)

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

index c0212244bdf7..cf60291db8fd 100644
@@ -2874,6 +2874,11 @@ static bool section_have_execinstr(struct bpf_object *obj, int idx)
        return sh->sh_flags & SHF_EXECINSTR;
 }
 
+static bool starts_with_qmark(const char *s)
+{
+       return s && s[0] == '?';
+}
+
 static bool btf_needs_sanitization(struct bpf_object *obj)
 {
        bool has_func_global = kernel_supports(obj, FEAT_BTF_GLOBAL_FUNC);

@@ -2883,9 +2888,10 @@ static bool btf_needs_sanitization(struct bpf_object *obj)
        bool has_decl_tag = kernel_supports(obj, FEAT_BTF_DECL_TAG);
        bool has_type_tag = kernel_supports(obj, FEAT_BTF_TYPE_TAG);
        bool has_enum64 = kernel_supports(obj, FEAT_BTF_ENUM64);
+       bool has_qmark_datasec = kernel_supports(obj, FEAT_BTF_QMARK_DATASEC);
 
        return !has_func || !has_datasec || !has_func_global || !has_float ||
-              !has_decl_tag || !has_type_tag || !has_enum64;
+              !has_decl_tag || !has_type_tag || !has_enum64 || !has_qmark_datasec;
 }
 
 static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)

@@ -2897,6 +2903,7 @@ static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
        bool has_decl_tag = kernel_supports(obj, FEAT_BTF_DECL_TAG);
        bool has_type_tag = kernel_supports(obj, FEAT_BTF_TYPE_TAG);
        bool has_enum64 = kernel_supports(obj, FEAT_BTF_ENUM64);
+       bool has_qmark_datasec = kernel_supports(obj, FEAT_BTF_QMARK_DATASEC);
        int enum64_placeholder_id = 0;
        struct btf_type *t;
        int i, j, vlen;

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

                                        *name = '_';

@@ -2938,6 +2947,25 @@ static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
                                vt = (void *)btf__type_by_id(btf, v->type);
                                m->name_off = vt->name_off;
                        }
+               } else if (!has_qmark_datasec && btf_is_datasec(t) &&
+                          starts_with_qmark(btf__name_by_offset(btf, t->name_off))) {
+                       /* remove '?' prefix and add '.optional' suffix for
+                        * DATASEC names staring from '?':
+                        *
+                        *   DATASEC ?.foo -> DATASEC .foo.optional
+                        */
+                       const char *name;
+                       char buf[256];

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

                } else if (!has_func && btf_is_func_proto(t)) {
                        /* replace FUNC_PROTO with ENUM */
                        vlen = btf_vlen(t);

tools/lib/bpf/libbpf_internal.h +2 -1

index ad936ac5e639..864b36177424 100644
@@ -374,6 +374,8 @@ enum kern_feature_id {
        FEAT_UPROBE_MULTI_LINK,
        /* Kernel supports arg:ctx tag (__arg_ctx) for global subprogs natively */
        FEAT_ARG_CTX_TAG,
+       /* Kernel supports '?' at the front of datasec names */
+       FEAT_BTF_QMARK_DATASEC,
        __FEAT_CNT,
 };
 
--

2.43.0

tools/lib/bpf/libbpf.c static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf) · line 2935

Quoted 8 lines

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

@@ -2922,6 +2929,8 @@ static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
                        char *name;
 
                        name = (char *)btf__name_by_offset(btf, t->name_off);
+                       if (*name == '?')
+                               *name++ = '_';
                        while (*name) {
                                if (*name == '.')

let's just extend this to if (*name == '.' || *name == '?') ?

Quoted 20 lines

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

@@ -2938,6 +2947,25 @@ static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
                                vt = (void *)btf__type_by_id(btf, v->type);
                                m->name_off = vt->name_off;
                        }
+               } else if (!has_qmark_datasec && btf_is_datasec(t) &&
+                          starts_with_qmark(btf__name_by_offset(btf, t->name_off))) {
+                       /* remove '?' prefix and add '.optional' suffix for
+                        * DATASEC names staring from '?':
+                        *
+                        *   DATASEC ?.foo -> DATASEC .foo.optional
+                        */
+                       const char *name;
+                       char buf[256];
+                       int str;
+
+                       name = btf__name_by_offset(btf, t->name_off);
+                       snprintf(buf, sizeof(buf), "%s.optional", &name[1] /* skip '?' */);
+                       str = btf__add_str(btf, buf);
+                       if (str < 0)
+                               return str;

tools/lib/bpf/libbpf.c static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf) · line 2968

Quoted 3 lines

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

+
+                       t = (struct btf_type *)btf__type_by_id(btf, i);
+                       t->name_off = str;

let's keep it simpler, just do in-place name sanitization like we did for !has_datasec case? It's fine if "?.struct_ops" becomes "_.struct_ops", kernel doesn't care and doesn't assign any special meaning to DATASEC names

Check that "?.struct_ops" and "?.struct_ops.link" section names define struct_ops maps with autocreate == false after open.

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

---
 .../bpf/prog_tests/struct_ops_autocreate.c    | 58 +++++++++++++++++--
 .../bpf/progs/struct_ops_autocreate.c         | 10 ++++
 2 files changed, 62 insertions(+), 6 deletions(-)

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c +30 -3

index 35910cbb9ca4..a89ba036e2e2 100644
@@ -34,10 +34,24 @@ static void cant_load_full_object(void)
        struct_ops_autocreate__destroy(skel);
 }
 
+static int check_test_1_link(struct struct_ops_autocreate *skel, struct bpf_map *map)
+{
+       struct bpf_link *link;
+       int err;
+
+       link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
+       if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops"))
+               return -1;
+
+       /* test_1() would be called from bpf_dummy_reg2() in bpf_testmod.c */
+       err = ASSERT_EQ(skel->bss->test_1_result, 42, "test_1_result");
+       bpf_link__destroy(link);
+       return err;
+}
+
 static void can_load_partial_object(void)
 {
        struct struct_ops_autocreate *skel;
-       struct bpf_link *link = NULL;
        int err;
 
        skel = struct_ops_autocreate__open();

@@ -52,15 +66,45 @@ static void can_load_partial_object(void)
        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"))
+       check_test_1_link(skel, skel->maps.testmod_1);
+
+cleanup:
+       struct_ops_autocreate__destroy(skel);
+}
+
+static void optional_maps(void)
+{
+       struct struct_ops_autocreate *skel;
+       int err;
+
+       skel = struct_ops_autocreate__open();
+       if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open"))
+               return;
+

tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c +13 -3

-       /* test_1() would be called from bpf_dummy_reg2() in bpf_testmod.c */
-       ASSERT_EQ(skel->bss->test_1_result, 42, "test_1_result");
+       err  = bpf_map__set_autocreate(skel->maps.testmod_1, false);
+       err |= bpf_map__set_autocreate(skel->maps.testmod_2, false);
+       err |= bpf_map__set_autocreate(skel->maps.optional_map2, true);
+       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;
+
+       check_test_1_link(skel, skel->maps.optional_map2);
 
 cleanup:
-       bpf_link__destroy(link);
        struct_ops_autocreate__destroy(skel);
 }
 

@@ -108,4 +152,6 @@ void test_struct_ops_autocreate(void)
                can_load_partial_object();
        if (test__start_subtest("autoload_and_shadow_vars"))
                autoload_and_shadow_vars();
+       if (test__start_subtest("optional_maps"))
+               optional_maps();
 }

tools/testing/selftests/bpf/progs/struct_ops_autocreate.c +10 -1

index 9a951ee6f55c..ba10c3896213 100644
@@ -40,3 +40,13 @@ struct bpf_testmod_ops___v2 testmod_2 = {
        .test_1 = (void *)test_1,
        .does_not_exist = (void *)test_2
 };
+
+SEC("?.struct_ops")
+struct bpf_testmod_ops___v1 optional_map = {
+       .test_1 = (void *)test_1,
+};
+
+SEC("?.struct_ops.link")
+struct bpf_testmod_ops___v1 optional_map2 = {
+       .test_1 = (void *)test_1,
+};
--

2.43.0

Currently kernel does not allow question marks in BTF names. This commit makes an exception, allowing first character of the DATASEC name to be a question mark.

The intent is to allow libbpf to use SEC("?.struct_ops") to identify struct_ops maps that are optional, e.g. like in the following BPF code:

    SEC("?.struct_ops")
    struct test_ops optional_map = { ... };

Which yields the following BTF:

    ...
    [13] DATASEC '?.struct_ops' size=0 vlen=...
    ...

To load such BTF libbpf rewrites DATASEC name before load. After this patch the rewrite won't be necessary.

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

---
 kernel/bpf/btf.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

kernel/bpf/btf.c +0 -0

index 6ff0bd1a91d5..a25fb6bce808 100644
@@ -761,12 +761,13 @@ static bool btf_name_offset_valid(const struct btf *btf, u32 offset)
        return offset < btf->hdr.str_len;
 }

kernel/bpf/btf.c +6 -7

                return false;
        return true;
 }

@@ -783,20 +784,20 @@ static const char *btf_str_by_offset(const struct btf *btf, u32 offset)
        return NULL;
 }
 
-static bool __btf_name_valid(const struct btf *btf, u32 offset)
+static bool __btf_name_valid(const struct btf *btf, u32 offset, bool allow_qmark)
 {
        /* offset must be valid */
        const char *src = btf_str_by_offset(btf, offset);
        const char *src_limit;
 
-       if (!__btf_name_char_ok(*src, true))
+       if (!__btf_name_char_ok(*src, true, allow_qmark))
                return false;
 
        /* set a limit on identifier length */
        src_limit = src + KSYM_NAME_LEN;
        src++;
        while (*src && src < src_limit) {
-               if (!__btf_name_char_ok(*src, false))
+               if (!__btf_name_char_ok(*src, false, false))
                        return false;
                src++;
        }

@@ -806,12 +807,12 @@ static bool __btf_name_valid(const struct btf *btf, u32 offset)
 
 static bool btf_name_valid_identifier(const struct btf *btf, u32 offset)
 {
-       return __btf_name_valid(btf, offset);
+       return __btf_name_valid(btf, offset, false);
 }
 
 static bool btf_name_valid_section(const struct btf *btf, u32 offset)
 {
-       return __btf_name_valid(btf, offset);
+       return __btf_name_valid(btf, offset, true);
 }
 
 static const char *__btf_name_by_offset(const struct btf *btf, u32 offset)

@@ -4481,7 +4482,7 @@ static s32 btf_var_check_meta(struct btf_verifier_env *env,
        }
 
        if (!t->name_off ||
-           !__btf_name_valid(env->btf, t->name_off)) {
+           !btf_name_valid_identifier(env->btf, t->name_off)) {
                btf_verifier_log_type(env, t, "Invalid name");
                return -EINVAL;
        }
--

2.43.0

Quoted 4 lines

kernel/bpf/btf.c +0 -0

@@ -761,12 +761,13 @@ static bool btf_name_offset_valid(const struct btf *btf, u32 offset)
        return offset < btf->hdr.str_len;
 }
 

kernel/bpf/btf.c static bool btf_name_offset_valid(const struct btf *btf, u32 offset) · line 770

Quoted 9 lines

kernel/bpf/btf.c +3 -2

-static bool __btf_name_char_ok(char c, bool first)
+static bool __btf_name_char_ok(char c, bool first, bool allow_qmark)
 {
        if ((first ? !isalpha(c) :
                     !isalnum(c)) &&
            c != '_' &&
-           c != '.')
+           c != '.' &&
+           (allow_qmark && first ? c != '?' : true))

ELF data section can have pretty much arbitrary characters in the name. I don't think we should allow question mark only at the beginning. Let's just allow any printable character, anywhere (for DATASEC only, of course)? There is no danger in doing this, data section name is not a variable name or some C identifier, and so won't be used like that.