[PATCH bpf-next v3 00/15] libbpf: type suffixes and autocreate flag for struct_ops maps

[PATCH bpf-next v3 00/15] libbpf: type suffixes and autocreate flag for struct_ops maps

[Eduard Zingerman]

Tweak struct_ops related APIs to allow the following features:
- specify version suffixes for stuct_ops map types;
- share same BPF program between several map definitions with
  different local BTF types, assuming only maps with same
  kernel BTF type would be selected for load;
- toggle autocreate flag for struct_ops maps;
- automatically toggle autoload for struct_ops programs referenced
  from struct_ops maps, depending on autocreate status of the
  corresponding map;
- use SEC("?.struct_ops") and SEC("?.struct_ops.link")
  to define struct_ops maps with autocreate == false after object open.

This would allow loading programs like below:

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

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

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

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

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

Assuming the following tweaks are done before loading:

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

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

Patch #8 ties autocreate and autoload flags for struct_ops maps and
programs.

Changelog:
- v2 [2] -> v3:
  - moved patch #8 logic to be fully done on load
    (requested by Andrii in offlist discussion);
  - in patch #9 added test case for shadow vars and
    autocreate/autoload interaction.
- v1 [1] -> v2:
  - fixed memory leak in patch #1 (Kui-Feng);
  - improved error messages in patch #2 (Martin, Andrii);
  - in bad_struct_ops selftest from patch #6 added .test_2
    map member setup (David);
  - added utility functions to capture libbpf log from selftests (David)
  - in selftests replaced usage of ...__open_and_load by separate
    calls to ..._open() and ..._load() (Andrii);
  - removed serial_... in selftest definitions (Andrii);
  - improved comments in selftest struct_ops_autocreate
    from patch #7 (David);
  - removed autoload toggling logic incompatible with shadow variables
    from bpf_map__set_autocreate(), instead struct_ops programs
    autoload property is computed at struct_ops maps load phase,
    see patch #8 (Kui-Feng, Martin, Andrii);
  - added support for SEC("?.struct_ops") and SEC("?.struct_ops.link")
    (Andrii).

[1] https://lore.kernel.org/bpf/20240227204556.17524-1-eddyz87@gmail.com/
[2] https://lore.kernel.org/bpf/20240302011920.15302-1-eddyz87@gmail.com/

Eduard Zingerman (15):
  libbpf: allow version suffixes (___smth) for struct_ops types
  libbpf: tie struct_ops programs to kernel BTF ids, not to local ids
  libbpf: honor autocreate flag for struct_ops maps
  selftests/bpf: test struct_ops map definition with type suffix
  selftests/bpf: utility functions to capture libbpf log in test_progs
  selftests/bpf: bad_struct_ops test
  selftests/bpf: test autocreate behavior for struct_ops maps
  libbpf: sync progs autoload with maps autocreate for struct_ops maps
  selftests/bpf: verify struct_ops autoload/autocreate sync
  libbpf: replace elf_state->st_ops_* fields with SEC_ST_OPS sec_type
  libbpf: struct_ops in SEC("?.struct_ops") and SEC("?.struct_ops.link")
  libbpf: rewrite btf datasec names starting from '?'
  selftests/bpf: test case for SEC("?.struct_ops")
  bpf: allow '?' at the beginning of DATASEC names
  selftests/bpf: test cases for '?' in BTF names

 kernel/bpf/btf.c                              |  17 +-
 tools/lib/bpf/features.c                      |  22 ++
 tools/lib/bpf/libbpf.c                        | 214 +++++++++++++-----
 tools/lib/bpf/libbpf_internal.h               |   2 +
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  25 ++
 .../selftests/bpf/bpf_testmod/bpf_testmod.h   |   4 +
 .../selftests/bpf/prog_tests/bad_struct_ops.c |  67 ++++++
 tools/testing/selftests/bpf/prog_tests/btf.c  |  46 ++++
 .../bpf/prog_tests/struct_ops_autocreate.c    | 157 +++++++++++++
 .../bpf/prog_tests/test_struct_ops_module.c   |  33 ++-
 .../selftests/bpf/progs/bad_struct_ops.c      |  25 ++
 .../selftests/bpf/progs/bad_struct_ops2.c     |  14 ++
 .../bpf/progs/struct_ops_autocreate.c         |  52 +++++
 .../bpf/progs/struct_ops_autocreate2.c        |  32 +++
 .../selftests/bpf/progs/struct_ops_module.c   |  21 +-
 tools/testing/selftests/bpf/test_progs.c      |  57 +++++
 tools/testing/selftests/bpf/test_progs.h      |   3 +
 17 files changed, 717 insertions(+), 74 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
 create mode 100644 tools/testing/selftests/bpf/progs/bad_struct_ops.c
 create mode 100644 tools/testing/selftests/bpf/progs/bad_struct_ops2.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_autocreate2.c

-- 
2.43.0

[PATCH bpf-next v3 01/15] libbpf: allow version suffixes (___smth) for struct_ops types

[Eduard Zingerman]

E.g. allow the following struct_ops definitions:

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

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

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

Acked-by: David Vernet <void@manifault.com>
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/lib/bpf/libbpf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 6c2979f1b471..e2a4c409980b 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -948,7 +948,7 @@ static int find_btf_by_prefix_kind(const struct btf *btf, const char *prefix,
 				   const char *name, __u32 kind);
 
 static int
-find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
+find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
 			   struct module_btf **mod_btf,
 			   const struct btf_type **type, __u32 *type_id,
 			   const struct btf_type **vtype, __u32 *vtype_id,
@@ -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

[PATCH bpf-next v3 02/15] libbpf: tie struct_ops programs to kernel BTF ids, not to local ids

[Eduard Zingerman]

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

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

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

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

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

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

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

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index e2a4c409980b..2c0cb72bc7a4 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1146,8 +1146,32 @@ 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) {
+				pr_warn("struct_ops init_kern %s func ptr %s: invalid reuse of prog %s in sec %s with type %u: attach_btf_id %u != kern_type_id %u\n",
+					map->name, mname, prog->name, prog->sec_name, prog->type,
+					prog->attach_btf_id, kern_type_id);
+				return -EINVAL;
+			}
+			if (prog->expected_attach_type != kern_member_idx) {
+				pr_warn("struct_ops init_kern %s func ptr %s: invalid reuse of prog %s in sec %s with type %u: expected_attach_type %u != kern_member_idx %u\n",
+					map->name, mname, prog->name, prog->sec_name, prog->type,
+					prog->expected_attach_type, kern_member_idx);
+				return -EINVAL;
+			}
 
 			st_ops->kern_func_off[i] = kern_data_off + kern_moff;
 
@@ -9428,27 +9452,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;
 
 		/* st_ops->data will be exposed to users, being returned by
-- 
2.43.0

[PATCH bpf-next v3 03/15] libbpf: honor autocreate flag for struct_ops maps

[Eduard Zingerman]

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

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

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

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

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

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

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

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

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

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 2c0cb72bc7a4..25c452c20d7d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -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)
 			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)
 			bpf_map_prepare_vdata(&obj->maps[i]);
 
 	return 0;
-- 
2.43.0

[PATCH bpf-next v3 04/15] selftests/bpf: test struct_ops map definition with type suffix

[Eduard Zingerman]

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

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

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 098ddd067224..b9ff88e3d463 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -564,6 +564,7 @@ static int bpf_dummy_reg(void *kdata)
 {
 	struct bpf_testmod_ops *ops = kdata;
 
+	ops->test_1();
 	/* Some test cases (ex. struct_ops_maybe_null) may not have test_2
 	 * initialized, so we need to check for NULL.
 	 */
diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
index 7d6facf46ebb..ee5372c7f2c7 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
@@ -30,11 +30,29 @@ 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)
 {
 	struct struct_ops_module *skel;
 	struct bpf_map_info info = {};
-	struct bpf_link *link;
 	int err;
 	u32 len;
 
@@ -59,20 +77,17 @@ 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");
-
+	check_map_info(&info);
 	/* test_3() will be called from bpf_dummy_reg() in bpf_testmod.c
 	 *
 	 * In bpf_testmod.c it will pass 4 and 13 (the value of data) to
 	 * .test_2.  So, the value of test_2_result should be 20 (4 + 13 +
 	 * 3).
 	 */
-	ASSERT_EQ(skel->bss->test_2_result, 20, "check_shadow_variables");
-
-	bpf_link__destroy(link);
-
-	check_map_info(&info);
+	if (!attach_ops_and_check(skel, skel->maps.testmod_1, 20))
+		goto cleanup;
+	if (!attach_ops_and_check(skel, skel->maps.testmod_2, 12))
+		goto cleanup;
 
 cleanup:
 	struct_ops_module__destroy(skel);
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_module.c b/tools/testing/selftests/bpf/progs/struct_ops_module.c
index 25952fa09348..026cabfa7f1f 100644
--- a/tools/testing/selftests/bpf/progs/struct_ops_module.c
+++ b/tools/testing/selftests/bpf/progs/struct_ops_module.c
@@ -7,12 +7,14 @@
 
 char _license[] SEC("license") = "GPL";
 
+int test_1_result = 0;
 int test_2_result = 0;
 
 SEC("struct_ops/test_1")
 int BPF_PROG(test_1)
 {
-	return 0xdeadbeef;
+	test_1_result = 0xdeadbeef;
+	return 0;
 }
 
 SEC("struct_ops/test_2")
@@ -35,3 +37,20 @@ struct bpf_testmod_ops testmod_1 = {
 	.data = 0x1,
 };
 
+SEC("struct_ops/test_2")
+void BPF_PROG(test_2_v2, int a, int b)
+{
+	test_2_result = a * b;
+}
+
+struct bpf_testmod_ops___v2 {
+	int (*test_1)(void);
+	void (*test_2)(int a, int b);
+	int (*test_maybe_null)(int dummy, struct task_struct *task);
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops___v2 testmod_2 = {
+	.test_1 = (void *)test_1,
+	.test_2 = (void *)test_2_v2,
+};
-- 
2.43.0

[PATCH bpf-next v3 05/15] selftests/bpf: utility functions to capture libbpf log in test_progs

[Eduard Zingerman]

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 <eddyz87@gmail.com>
---
 tools/testing/selftests/bpf/test_progs.c | 57 ++++++++++++++++++++++++
 tools/testing/selftests/bpf/test_progs.h |  3 ++
 2 files changed, 60 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 808550986f30..698c7387b310 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -683,9 +683,65 @@ static const struct argp_option opts[] = {
 	{},
 };
 
+static FILE *libbpf_capture_stream;
+
+static struct {
+	char *buf;
+	size_t buf_sz;
+} libbpf_output_capture;
+
+/* 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)
+{
+	if (libbpf_capture_stream) {
+		PRINT_FAIL("%s: libbpf_capture_stream != NULL\n", __func__);
+		return -EINVAL;
+	}
+
+	libbpf_capture_stream = open_memstream(&libbpf_output_capture.buf,
+					       &libbpf_output_capture.buf_sz);
+	if (!libbpf_capture_stream) {
+		PRINT_FAIL("%s: open_memstream failed errno=%d\n", __func__, errno);
+		return -EINVAL;
+	}
+
+	return 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;
+
+	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));
+	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;
+
+		va_copy(args2, args);
+		vfprintf(libbpf_capture_stream, format, args2);
+	}
+
 	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);
 }
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 80df51244886..0ba5a20b19ba 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -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

[PATCH bpf-next v3 06/15] selftests/bpf: bad_struct_ops test

[Eduard Zingerman]

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

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 24 +++++++++++++
 .../selftests/bpf/bpf_testmod/bpf_testmod.h   |  4 +++
 .../selftests/bpf/prog_tests/bad_struct_ops.c | 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

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index b9ff88e3d463..2de7e80dbb4b 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -610,6 +610,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)
@@ -621,6 +644,7 @@ static int bpf_testmod_init(void)
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_testmod_kfunc_set);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_testmod_kfunc_set);
 	ret = ret ?: register_bpf_struct_ops(&bpf_bpf_testmod_ops, bpf_testmod_ops);
+	ret = ret ?: register_bpf_struct_ops(&bpf_testmod_ops2, bpf_testmod_ops2);
 	if (ret < 0)
 		return ret;
 	if (bpf_fentry_test1(0) < 0)
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
index 971458acfac3..c51c4eae9ed5 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
@@ -45,4 +45,8 @@ struct bpf_testmod_ops {
 	int data;
 };
 
+struct bpf_testmod_ops2 {
+	int (*test_1)(void);
+};
+
 #endif /* _BPF_TESTMOD_H */
diff --git a/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c b/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
new file mode 100644
index 000000000000..9f5dbefa0dd9
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
@@ -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;
+
+	skel = bad_struct_ops__open();
+	if (!ASSERT_OK_PTR(skel, "bad_struct_ops__open"))
+		return;
+
+	if (start_libbpf_log_capture())
+		goto cleanup;
+
+	err = bad_struct_ops__load(skel);
+	log = stop_libbpf_log_capture();
+	ASSERT_ERR(err, "bad_struct_ops__load should fail");
+	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();
+}
diff --git a/tools/testing/selftests/bpf/progs/bad_struct_ops.c b/tools/testing/selftests/bpf/progs/bad_struct_ops.c
new file mode 100644
index 000000000000..b7e175cd0af0
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bad_struct_ops.c
@@ -0,0 +1,25 @@
+// 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/test_2")
+int BPF_PROG(test_2) { return 0; }
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops testmod_1 = {
+	.test_1 = (void *)test_1,
+	.test_2 = (void *)test_2
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops2 testmod_2 = {
+	.test_1 = (void *)test_1
+};
-- 
2.43.0

[PATCH bpf-next v3 07/15] selftests/bpf: test autocreate behavior for struct_ops maps

[Eduard Zingerman]

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

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 .../bpf/prog_tests/struct_ops_autocreate.c    | 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

diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
new file mode 100644
index 000000000000..883f938d518c
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
@@ -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;
+
+	skel = struct_ops_autocreate__open();
+	if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open"))
+		return;
+
+	if (start_libbpf_log_capture())
+		goto cleanup;
+	/* 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);
+}
+
+static void can_load_partial_object(void)
+{
+	struct struct_ops_autocreate *skel;
+	struct bpf_link *link = NULL;
+	int err;
+
+	skel = struct_ops_autocreate__open();
+	if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open_opts"))
+		return;
+
+	err = bpf_program__set_autoload(skel->progs.test_2, false);
+	if (!ASSERT_OK(err, "bpf_program__set_autoload"))
+		goto cleanup;
+
+	err = bpf_map__set_autocreate(skel->maps.testmod_2, false);
+	if (!ASSERT_OK(err, "bpf_map__set_autocreate"))
+		goto cleanup;
+
+	err = struct_ops_autocreate__load(skel);
+	if (ASSERT_OK(err, "struct_ops_autocreate__load"))
+		goto cleanup;
+
+	link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
+	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops"))
+		goto cleanup;
+
+	/* test_1() would be called from bpf_dummy_reg2() in bpf_testmod.c */
+	ASSERT_EQ(skel->bss->test_1_result, 42, "test_1_result");
+
+cleanup:
+	bpf_link__destroy(link);
+	struct_ops_autocreate__destroy(skel);
+}
+
+void test_struct_ops_autocreate(void)
+{
+	if (test__start_subtest("cant_load_full_object"))
+		cant_load_full_object();
+	if (test__start_subtest("can_load_partial_object"))
+		can_load_partial_object();
+}
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
new file mode 100644
index 000000000000..9a951ee6f55c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
@@ -0,0 +1,42 @@
+// 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(test_1)
+{
+	test_1_result = 42;
+	return 0;
+}
+
+SEC("struct_ops/test_1")
+int BPF_PROG(test_2)
+{
+	return 0;
+}
+
+struct bpf_testmod_ops___v1 {
+	int (*test_1)(void);
+};
+
+struct bpf_testmod_ops___v2 {
+	int (*test_1)(void);
+	int (*does_not_exist)(void);
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops___v1 testmod_1 = {
+	.test_1 = (void *)test_1
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops___v2 testmod_2 = {
+	.test_1 = (void *)test_1,
+	.does_not_exist = (void *)test_2
+};
-- 
2.43.0

[PATCH bpf-next v3 08/15] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Eduard Zingerman]

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 <eddyz87@gmail.com>
---
 tools/lib/bpf/libbpf.c | 48 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 25c452c20d7d..ce39ae34fec0 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1031,6 +1031,53 @@ static bool is_valid_st_ops_program(struct bpf_object *obj,
 	return false;
 }
 
+/* For each struct_ops program P, referenced from some struct_ops map M,
+ * enable P.autoload if there are Ms for which M.autocreate is true,
+ * disable P.autoload if for all Ms M.autocreate is false.
+ * Don't change P.autoload for programs that are not referenced from any maps.
+ */
+static int bpf_object__adjust_struct_ops_autoload(struct bpf_object *obj)
+{
+	struct bpf_program *prog;
+	struct bpf_map *map;
+	int i, j, k, vlen;
+	struct {
+		__u8 any_map_autocreate:1;
+		__u8 used_in_struct_ops_map:1;
+	} *refs;
+
+	refs = calloc(obj->nr_programs, sizeof(refs[0]));
+	if (!refs)
+		return -ENOMEM;
+
+	for (i = 0; i < obj->nr_maps; i++) {
+		map = &obj->maps[i];
+		if (!bpf_map__is_struct_ops(map))
+			continue;
+
+		vlen = btf_vlen(map->st_ops->type);
+		for (j = 0; j < vlen; ++j) {
+			prog = map->st_ops->progs[j];
+			if (!prog)
+				continue;
+
+			k = prog - obj->programs;
+			if (k < 0 || k > obj->nr_programs)
+				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

[PATCH bpf-next v3 09/15] selftests/bpf: verify struct_ops autoload/autocreate sync

[Eduard Zingerman]

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 <eddyz87@gmail.com>
---
 .../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

diff --git a/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c b/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
index 9f5dbefa0dd9..6a707213e46b 100644
--- a/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
+++ b/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
@@ -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();
 }
diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
index 883f938d518c..35910cbb9ca4 100644
--- a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
+++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
@@ -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");
+	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;
+
+	err = struct_ops_autocreate2__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 */
+	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();
 }
diff --git a/tools/testing/selftests/bpf/progs/bad_struct_ops2.c b/tools/testing/selftests/bpf/progs/bad_struct_ops2.c
new file mode 100644
index 000000000000..64a95f6be86d
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bad_struct_ops2.c
@@ -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) {}
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_autocreate2.c b/tools/testing/selftests/bpf/progs/struct_ops_autocreate2.c
new file mode 100644
index 000000000000..6049d9c902d3
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_autocreate2.c
@@ -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

[PATCH bpf-next v3 10/15] libbpf: replace elf_state->st_ops_* fields with SEC_ST_OPS sec_type

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

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

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ce39ae34fec0..4ef3902e65db 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -612,6 +612,7 @@ enum sec_type {
 	SEC_BSS,
 	SEC_DATA,
 	SEC_RODATA,
+	SEC_ST_OPS,
 };
 
 struct elf_sec_desc {
@@ -627,8 +628,6 @@ struct elf_state {
 	Elf *elf;
 	Elf64_Ehdr *ehdr;
 	Elf_Data *symbols;
-	Elf_Data *st_ops_data;
-	Elf_Data *st_ops_link_data;
 	size_t shstrndx; /* section index for section name strings */
 	size_t strtabidx;
 	struct elf_sec_desc *secs;
@@ -637,8 +636,7 @@ struct elf_state {
 	__u32 btf_maps_sec_btf_id;
 	int text_shndx;
 	int symbols_shndx;
-	int st_ops_shndx;
-	int st_ops_link_shndx;
+	bool has_st_ops;
 };
 
 struct usdt_manager;
@@ -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;
@@ -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;
 
 		map->st_ops = calloc(1, sizeof(*map->st_ops));
 		if (!map->st_ops)
@@ -1365,15 +1364,25 @@ static int init_struct_ops_maps(struct bpf_object *obj, const char *sec_name,
 
 static int bpf_object_init_struct_ops(struct bpf_object *obj)
 {
-	int err;
+	const char *sec_name;
+	int sec_idx, err;
 
-	err = init_struct_ops_maps(obj, STRUCT_OPS_SEC, obj->efile.st_ops_shndx,
-				   obj->efile.st_ops_data, 0);
-	err = err ?: init_struct_ops_maps(obj, STRUCT_OPS_LINK_SEC,
-					  obj->efile.st_ops_link_shndx,
-					  obj->efile.st_ops_link_data,
-					  BPF_F_LINK);
-	return err;
+	for (sec_idx = 0; sec_idx < obj->efile.sec_cnt; ++sec_idx) {
+		struct elf_sec_desc *desc = &obj->efile.secs[sec_idx];
+
+		if (desc->sec_type != SEC_ST_OPS)
+			continue;
+
+		sec_name = elf_sec_name(obj, elf_sec_by_idx(obj, sec_idx));
+		if (!sec_name)
+			return -LIBBPF_ERRNO__FORMAT;
+
+		err = init_struct_ops_maps(obj, sec_name, sec_idx, desc->data);
+		if (err)
+			return err;
+	}
+
+	return 0;
 }
 
 static struct bpf_object *bpf_object__new(const char *path,
@@ -1411,8 +1420,6 @@ static struct bpf_object *bpf_object__new(const char *path,
 	obj->efile.obj_buf = obj_buf;
 	obj->efile.obj_buf_sz = obj_buf_sz;
 	obj->efile.btf_maps_shndx = -1;
-	obj->efile.st_ops_shndx = -1;
-	obj->efile.st_ops_link_shndx = -1;
 	obj->kconfig_map_idx = -1;
 
 	obj->kern_version = get_kernel_version();
@@ -1429,8 +1436,6 @@ static void bpf_object__elf_finish(struct bpf_object *obj)
 	elf_end(obj->efile.elf);
 	obj->efile.elf = NULL;
 	obj->efile.symbols = NULL;
-	obj->efile.st_ops_data = NULL;
-	obj->efile.st_ops_link_data = NULL;
 
 	zfree(&obj->efile.secs);
 	obj->efile.sec_cnt = 0;
@@ -2975,14 +2980,13 @@ static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
 static bool libbpf_needs_btf(const struct bpf_object *obj)
 {
 	return obj->efile.btf_maps_shndx >= 0 ||
-	       obj->efile.st_ops_shndx >= 0 ||
-	       obj->efile.st_ops_link_shndx >= 0 ||
+	       obj->efile.has_st_ops ||
 	       obj->nr_extern > 0;
 }
 
 static bool kernel_needs_btf(const struct bpf_object *obj)
 {
-	return obj->efile.st_ops_shndx >= 0 || obj->efile.st_ops_link_shndx >= 0;
+	return obj->efile.has_st_ops;
 }
 
 static int bpf_object__init_btf(struct bpf_object *obj,
@@ -3683,12 +3687,12 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
 				sec_desc->sec_type = SEC_RODATA;
 				sec_desc->shdr = sh;
 				sec_desc->data = data;
-			} else if (strcmp(name, STRUCT_OPS_SEC) == 0) {
-				obj->efile.st_ops_data = data;
-				obj->efile.st_ops_shndx = idx;
-			} else if (strcmp(name, STRUCT_OPS_LINK_SEC) == 0) {
-				obj->efile.st_ops_link_data = data;
-				obj->efile.st_ops_link_shndx = idx;
+			} else if (strcmp(name, STRUCT_OPS_SEC) == 0 ||
+				   strcmp(name, STRUCT_OPS_LINK_SEC) == 0) {
+				sec_desc->sec_type = SEC_ST_OPS;
+				sec_desc->shdr = sh;
+				sec_desc->data = data;
+				obj->efile.has_st_ops = true;
 			} else {
 				pr_info("elf: skipping unrecognized data section(%d) %s\n",
 					idx, name);
@@ -7001,12 +7005,12 @@ static int bpf_object__collect_relos(struct bpf_object *obj)
 		data = sec_desc->data;
 		idx = shdr->sh_info;
 
-		if (shdr->sh_type != SHT_REL) {
+		if (shdr->sh_type != SHT_REL || idx < 0 || idx >= obj->efile.sec_cnt) {
 			pr_warn("internal error at %d\n", __LINE__);
 			return -LIBBPF_ERRNO__INTERNAL;
 		}
 
-		if (idx == obj->efile.st_ops_shndx || idx == obj->efile.st_ops_link_shndx)
+		if (obj->efile.secs[idx].sec_type == SEC_ST_OPS)
 			err = bpf_object__collect_st_ops_relos(obj, shdr, data);
 		else if (idx == obj->efile.btf_maps_shndx)
 			err = bpf_object__collect_map_relos(obj, shdr, data);
-- 
2.43.0

[PATCH bpf-next v3 11/15] libbpf: struct_ops in SEC("?.struct_ops") and SEC("?.struct_ops.link")

[Eduard Zingerman]

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 <eddyz87@gmail.com>
---
 tools/lib/bpf/libbpf.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 4ef3902e65db..c0212244bdf7 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -497,6 +497,8 @@ struct bpf_struct_ops {
 #define KSYMS_SEC ".ksyms"
 #define STRUCT_OPS_SEC ".struct_ops"
 #define STRUCT_OPS_LINK_SEC ".struct_ops.link"
+#define OPT_STRUCT_OPS_SEC "?.struct_ops"
+#define OPT_STRUCT_OPS_LINK_SEC "?.struct_ops.link"
 
 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;
@@ -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) {
 				sec_desc->sec_type = SEC_ST_OPS;
 				sec_desc->shdr = sh;
 				sec_desc->data = data;
@@ -3708,6 +3721,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
 			if (!section_have_execinstr(obj, targ_sec_idx) &&
 			    strcmp(name, ".rel" STRUCT_OPS_SEC) &&
 			    strcmp(name, ".rel" STRUCT_OPS_LINK_SEC) &&
+			    strcmp(name, ".rel" OPT_STRUCT_OPS_SEC) &&
+			    strcmp(name, ".rel" OPT_STRUCT_OPS_LINK_SEC) &&
 			    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

[PATCH bpf-next v3 12/15] libbpf: rewrite btf datasec names starting from '?'

[Eduard Zingerman]

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 <eddyz87@gmail.com>
---
 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(-)

diff --git a/tools/lib/bpf/features.c b/tools/lib/bpf/features.c
index 6b0738ad7063..4e783cc7fc4b 100644
--- a/tools/lib/bpf/features.c
+++ b/tools/lib/bpf/features.c
@@ -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)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index c0212244bdf7..cf60291db8fd 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -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;
@@ -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 == '.')
 					*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];
+			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;
+
+			t = (struct btf_type *)btf__type_by_id(btf, i);
+			t->name_off = str;
 		} else if (!has_func && btf_is_func_proto(t)) {
 			/* replace FUNC_PROTO with ENUM */
 			vlen = btf_vlen(t);
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index ad936ac5e639..864b36177424 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -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

[PATCH bpf-next v3 13/15] selftests/bpf: test case for SEC("?.struct_ops")

[Eduard Zingerman]

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

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 .../bpf/prog_tests/struct_ops_autocreate.c    | 58 +++++++++++++++++--
 .../bpf/progs/struct_ops_autocreate.c         | 10 ++++
 2 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
index 35910cbb9ca4..a89ba036e2e2 100644
--- a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
+++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
@@ -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;
+
+	err  = !ASSERT_TRUE(bpf_map__autocreate(skel->maps.testmod_1),
+			    "default autocreate for testmod_1");
+	err |= !ASSERT_TRUE(bpf_map__autocreate(skel->maps.testmod_2),
+			    "default autocreate for testmod_2");
+	err |= !ASSERT_FALSE(bpf_map__autocreate(skel->maps.optional_map),
+			     "default autocreate for optional_map");
+	err |= !ASSERT_FALSE(bpf_map__autocreate(skel->maps.optional_map2),
+			    "default autocreate for optional_map2");
+	if (err)
 		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");
+	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();
 }
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
index 9a951ee6f55c..ba10c3896213 100644
--- a/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
+++ b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
@@ -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

[PATCH bpf-next v3 14/15] bpf: allow '?' at the beginning of DATASEC names

[Eduard Zingerman]

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 <eddyz87@gmail.com>
---
 kernel/bpf/btf.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 6ff0bd1a91d5..a25fb6bce808 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -761,12 +761,13 @@ static bool btf_name_offset_valid(const struct btf *btf, u32 offset)
 	return offset < btf->hdr.str_len;
 }
 
-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))
 		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

[PATCH bpf-next v3 15/15] selftests/bpf: test cases for '?' in BTF names

[Eduard Zingerman]

Three test cases to verify when '?' is allowed in BTF names:
- allowed as first character in DATASEC name;
- not allowed as non-first character in DATASEC name;
- not allowed in any position in non-DATASEC names.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/btf.c | 46 ++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c
index 816145bcb647..88c71e3924b9 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf.c
@@ -3535,6 +3535,49 @@ static struct btf_raw_test raw_tests[] = {
 	.value_type_id = 1,
 	.max_entries = 1,
 },
+{
+	.descr = "datasec: name '?.foo' is ok",
+	.raw_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),
+		BTF_END_RAW,
+	},
+	BTF_STR_SEC("\0x\0?.foo"),
+},
+{
+	.descr = "datasec: name '.?foo' is not ok",
+	.raw_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),
+		BTF_END_RAW,
+	},
+	BTF_STR_SEC("\0x\0.?foo"),
+	.err_str = "Invalid name",
+	.btf_load_err = true,
+},
+{
+	.descr = "type name '?foo' is not ok",
+	.raw_types = {
+		/* union ?foo; */
+		BTF_TYPE_ENC(1, BTF_INFO_ENC(BTF_KIND_FWD, 1, 0), 0), /* [1] */
+		BTF_END_RAW,
+	},
+	BTF_STR_SEC("\0?foo"),
+	.err_str = "Invalid name",
+	.btf_load_err = true,
+},
 
 {
 	.descr = "float test #1, well-formed",
@@ -4363,6 +4406,9 @@ static void do_test_raw(unsigned int test_num)
 	if (err || btf_fd < 0)
 		goto done;
 
+	if (!test->map_type)
+		goto done;
+
 	opts.btf_fd = btf_fd;
 	opts.btf_key_type_id = test->key_type_id;
 	opts.btf_value_type_id = test->value_type_id;
-- 
2.43.0

Re: [PATCH bpf-next v3 07/15] selftests/bpf: test autocreate behavior for struct_ops maps

[Daniel Borkmann]

On 3/4/24 11:51 PM, Eduard Zingerman wrote:
> Check that bpf_map__set_autocreate() can be used to disable automatic
> creation for struct_ops maps.
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>   .../bpf/prog_tests/struct_ops_autocreate.c    | 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
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> new file mode 100644
> index 000000000000..883f938d518c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> @@ -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;
> +
> +	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.

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

Re: [PATCH bpf-next v3 07/15] selftests/bpf: test autocreate behavior for struct_ops maps

[Eduard Zingerman]

On Tue, 2024-03-05 at 10:51 +0100, Daniel Borkmann wrote:
[...]

> > diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> > new file mode 100644
> > index 000000000000..883f938d518c
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> > @@ -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;
> > +
> > +	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.

Thank you for noticing, I will double check all tests and post v4
after collecting some additional feedback.

[...]

Re: [PATCH bpf-next v3 01/15] libbpf: allow version suffixes (___smth) for struct_ops types

[Andrii Nakryiko]

On Mon, Mar 4, 2024 at 2:52 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> E.g. allow the following struct_ops definitions:
>
>     struct bpf_testmod_ops___v1 { int (*test)(void); };
>     struct bpf_testmod_ops___v2 { int (*test)(void); };
>
>     SEC(".struct_ops.link")
>     struct bpf_testmod_ops___v1 a = { .test = ... }
>     SEC(".struct_ops.link")
>     struct bpf_testmod_ops___v2 b = { .test = ... }
>
> Where both bpf_testmod_ops__v1 and bpf_testmod_ops__v2 would be
> resolved as 'struct bpf_testmod_ops' from kernel BTF.
>
> Acked-by: David Vernet <void@manifault.com>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  tools/lib/bpf/libbpf.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 6c2979f1b471..e2a4c409980b 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -948,7 +948,7 @@ static int find_btf_by_prefix_kind(const struct btf *btf, const char *prefix,
>                                    const char *name, __u32 kind);
>
>  static int
> -find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
> +find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
>                            struct module_btf **mod_btf,
>                            const struct btf_type **type, __u32 *type_id,
>                            const struct btf_type **vtype, __u32 *vtype_id,
> @@ -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
>

Re: [PATCH bpf-next v3 02/15] libbpf: tie struct_ops programs to kernel BTF ids, not to local ids

[Andrii Nakryiko]

On Mon, Mar 4, 2024 at 2:52 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Enforce the following existing limitation on struct_ops programs based
> on kernel BTF id instead of program-local BTF id:
>
>     struct_ops BPF prog can be re-used between multiple .struct_ops &
>     .struct_ops.link as long as it's the same struct_ops struct
>     definition and the same function pointer field
>
> This allows reusing same BPF program for versioned struct_ops map
> definitions, e.g.:
>
>     SEC("struct_ops/test")
>     int BPF_PROG(foo) { ... }
>
>     struct some_ops___v1 { int (*test)(void); };
>     struct some_ops___v2 { int (*test)(void); };
>
>     SEC(".struct_ops.link") struct some_ops___v1 a = { .test = foo }
>     SEC(".struct_ops.link") struct some_ops___v2 b = { .test = foo }
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  tools/lib/bpf/libbpf.c | 49 ++++++++++++++++++++++--------------------
>  1 file changed, 26 insertions(+), 23 deletions(-)
>

Acked-by: Andrii Nakryiko <andrii@kernel.org>

[...]

Re: [PATCH bpf-next v3 03/15] libbpf: honor autocreate flag for struct_ops maps

[Andrii Nakryiko]

On Mon, Mar 4, 2024 at 2:52 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Skip load steps for struct_ops maps not marked for automatic creation.
> This should allow to load bpf object in situations like below:
>
>     SEC("struct_ops/foo") int BPF_PROG(foo) { ... }
>     SEC("struct_ops/bar") int BPF_PROG(bar) { ... }
>
>     struct test_ops___v1 {
>         int (*foo)(void);
>     };
>
>     struct test_ops___v2 {
>         int (*foo)(void);
>         int (*does_not_exist)(void);
>     };
>
>     SEC(".struct_ops.link")
>     struct test_ops___v1 map_for_old = {
>         .test_1 = (void *)foo
>     };
>
>     SEC(".struct_ops.link")
>     struct test_ops___v2 map_for_new = {
>         .test_1 = (void *)foo,
>         .does_not_exist = (void *)bar
>     };
>
> Suppose program is loaded on old kernel that does not have definition
> for 'does_not_exist' struct_ops member. After this commit it would be
> possible to load such object file after the following tweaks:
>
>     bpf_program__set_autoload(skel->progs.bar, false);
>     bpf_map__set_autocreate(skel->maps.map_for_new, false);
>
> Acked-by: David Vernet <void@manifault.com>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  tools/lib/bpf/libbpf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 2c0cb72bc7a4..25c452c20d7d 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -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)

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


It will also mirror the structure in bpf_object__init_kern_struct_ops_maps()

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

Re: [PATCH bpf-next v3 05/15] selftests/bpf: utility functions to capture libbpf log in test_progs

[Andrii Nakryiko]

On Mon, Mar 4, 2024 at 2:52 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> 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 <eddyz87@gmail.com>
> ---
>  tools/testing/selftests/bpf/test_progs.c | 57 ++++++++++++++++++++++++
>  tools/testing/selftests/bpf/test_progs.h |  3 ++
>  2 files changed, 60 insertions(+)
>

[...]

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


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

> +
>         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);
>  }
> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> index 80df51244886..0ba5a20b19ba 100644
> --- a/tools/testing/selftests/bpf/test_progs.h
> +++ b/tools/testing/selftests/bpf/test_progs.h
> @@ -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
>

Re: [PATCH bpf-next v3 06/15] selftests/bpf: bad_struct_ops test

[Andrii Nakryiko]

On Mon, Mar 4, 2024 at 2:52 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> When loading struct_ops programs kernel requires BTF id of the
> struct_ops type and member index for attachment point inside that
> type. This makes it not possible to have same BPF program used in
> struct_ops maps that have different struct_ops type.
> Check if libbpf rejects such BPF objects files.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 24 +++++++++++++
>  .../selftests/bpf/bpf_testmod/bpf_testmod.h   |  4 +++
>  .../selftests/bpf/prog_tests/bad_struct_ops.c | 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 <andrii@kernel.org>

> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index b9ff88e3d463..2de7e80dbb4b 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -610,6 +610,29 @@ struct bpf_struct_ops bpf_bpf_testmod_ops = {
>         .owner = THIS_MODULE,
>  };
>

[...]

> diff --git a/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c b/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
> new file mode 100644
> index 000000000000..9f5dbefa0dd9
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
> @@ -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;
> +
> +       skel = bad_struct_ops__open();
> +       if (!ASSERT_OK_PTR(skel, "bad_struct_ops__open"))
> +               return;
> +
> +       if (start_libbpf_log_capture())
> +               goto cleanup;
> +
> +       err = bad_struct_ops__load(skel);
> +       log = stop_libbpf_log_capture();
> +       ASSERT_ERR(err, "bad_struct_ops__load should fail");

second argument isn't actually a test description, it's meant to be
used as a (relatively) short identifier of *what* is being checked
(see how ASSERT_xxx() macro are using them)

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

[...]

Re: [PATCH bpf-next v3 08/15] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Andrii Nakryiko]

On Mon, Mar 4, 2024 at 2:52 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> 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 <eddyz87@gmail.com>
> ---
>  tools/lib/bpf/libbpf.c | 48 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 25c452c20d7d..ce39ae34fec0 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1031,6 +1031,53 @@ static bool is_valid_st_ops_program(struct bpf_object *obj,
>         return false;
>  }
>
> +/* For each struct_ops program P, referenced from some struct_ops map M,
> + * enable P.autoload if there are Ms for which M.autocreate is true,
> + * disable P.autoload if for all Ms M.autocreate is false.
> + * Don't change P.autoload for programs that are not referenced from any maps.
> + */
> +static int bpf_object__adjust_struct_ops_autoload(struct bpf_object *obj)

nit: we try not to use double underscore separate for non-public
internal helper functions (see bpf_object_init_prog_arrays and
bpf_object_prepare_struct_ops)

> +{
> +       struct bpf_program *prog;
> +       struct bpf_map *map;
> +       int i, j, k, vlen;
> +       struct {
> +               __u8 any_map_autocreate:1;
> +               __u8 used_in_struct_ops_map:1;
> +       } *refs;

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?

> +
> +       refs = calloc(obj->nr_programs, sizeof(refs[0]));
> +       if (!refs)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < obj->nr_maps; i++) {
> +               map = &obj->maps[i];
> +               if (!bpf_map__is_struct_ops(map))
> +                       continue;
> +
> +               vlen = btf_vlen(map->st_ops->type);
> +               for (j = 0; j < vlen; ++j) {

nit: stay consistent with ++j vs j++?

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

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

Re: [PATCH bpf-next v3 09/15] selftests/bpf: verify struct_ops autoload/autocreate sync

[Andrii Nakryiko]

On Mon, Mar 4, 2024 at 2:52 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> 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 <eddyz87@gmail.com>
> ---
>  .../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
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c b/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
> index 9f5dbefa0dd9..6a707213e46b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bad_struct_ops.c
> @@ -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();
>  }
> diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> index 883f938d518c..35910cbb9ca4 100644
> --- a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> +++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> @@ -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");
> +       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;
> +
> +       err = struct_ops_autocreate2__load(skel);
> +       if (ASSERT_OK(err, "struct_ops_autocreate__load"))
> +               goto cleanup;
> +
> +

let's assert autoload state here as well?

> +       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();
>  }
> diff --git a/tools/testing/selftests/bpf/progs/bad_struct_ops2.c b/tools/testing/selftests/bpf/progs/bad_struct_ops2.c
> new file mode 100644
> index 000000000000..64a95f6be86d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/bad_struct_ops2.c
> @@ -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) {}
> diff --git a/tools/testing/selftests/bpf/progs/struct_ops_autocreate2.c b/tools/testing/selftests/bpf/progs/struct_ops_autocreate2.c
> new file mode 100644
> index 000000000000..6049d9c902d3
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/struct_ops_autocreate2.c
> @@ -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
>

Re: [PATCH bpf-next v3 10/15] libbpf: replace elf_state->st_ops_* fields with SEC_ST_OPS sec_type

[Andrii Nakryiko]

On Mon, Mar 4, 2024 at 2:52 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> 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 <eddyz87@gmail.com>
> ---
>  tools/lib/bpf/libbpf.c | 62 ++++++++++++++++++++++--------------------
>  1 file changed, 33 insertions(+), 29 deletions(-)
>

Makes sense and a good generalization!

Acked-by: Andrii Nakryiko <andrii@kernel.org>

[...]

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

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

[...]

Re: [PATCH bpf-next v3 11/15] libbpf: struct_ops in SEC("?.struct_ops") and SEC("?.struct_ops.link")

[Andrii Nakryiko]

On Mon, Mar 4, 2024 at 2:52 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> 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 <eddyz87@gmail.com>
> ---
>  tools/lib/bpf/libbpf.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>

Please check that patch subject is <80, and perhaps shorten it a bit?

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 4ef3902e65db..c0212244bdf7 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -497,6 +497,8 @@ struct bpf_struct_ops {
>  #define KSYMS_SEC ".ksyms"
>  #define STRUCT_OPS_SEC ".struct_ops"
>  #define STRUCT_OPS_LINK_SEC ".struct_ops.link"
> +#define OPT_STRUCT_OPS_SEC "?.struct_ops"
> +#define OPT_STRUCT_OPS_LINK_SEC "?.struct_ops.link"
>

I wouldn't do this, see below


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


>                                 sec_desc->sec_type = SEC_ST_OPS;
>                                 sec_desc->shdr = sh;
>                                 sec_desc->data = data;
> @@ -3708,6 +3721,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
>                         if (!section_have_execinstr(obj, targ_sec_idx) &&
>                             strcmp(name, ".rel" STRUCT_OPS_SEC) &&
>                             strcmp(name, ".rel" STRUCT_OPS_LINK_SEC) &&
> +                           strcmp(name, ".rel" OPT_STRUCT_OPS_SEC) &&
> +                           strcmp(name, ".rel" OPT_STRUCT_OPS_LINK_SEC) &&

and similarly here, just use ".rel?" ?

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

Re: [PATCH bpf-next v3 12/15] libbpf: rewrite btf datasec names starting from '?'

[Andrii Nakryiko]

On Mon, Mar 4, 2024 at 2:52 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> 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 <eddyz87@gmail.com>
> ---
>  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(-)
>
> diff --git a/tools/lib/bpf/features.c b/tools/lib/bpf/features.c
> index 6b0738ad7063..4e783cc7fc4b 100644
> --- a/tools/lib/bpf/features.c
> +++ b/tools/lib/bpf/features.c
> @@ -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)
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index c0212244bdf7..cf60291db8fd 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -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;
> @@ -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 == '?')` ?

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

>                 } else if (!has_func && btf_is_func_proto(t)) {
>                         /* replace FUNC_PROTO with ENUM */
>                         vlen = btf_vlen(t);
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index ad936ac5e639..864b36177424 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -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
>

Re: [PATCH bpf-next v3 13/15] selftests/bpf: test case for SEC("?.struct_ops")

[Andrii Nakryiko]

On Mon, Mar 4, 2024 at 2:52 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Check that "?.struct_ops" and "?.struct_ops.link" section names define
> struct_ops maps with autocreate == false after open.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  .../bpf/prog_tests/struct_ops_autocreate.c    | 58 +++++++++++++++++--
>  .../bpf/progs/struct_ops_autocreate.c         | 10 ++++
>  2 files changed, 62 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> index 35910cbb9ca4..a89ba036e2e2 100644
> --- a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> +++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> @@ -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;
> +
> +       err  = !ASSERT_TRUE(bpf_map__autocreate(skel->maps.testmod_1),
> +                           "default autocreate for testmod_1");
> +       err |= !ASSERT_TRUE(bpf_map__autocreate(skel->maps.testmod_2),
> +                           "default autocreate for testmod_2");
> +       err |= !ASSERT_FALSE(bpf_map__autocreate(skel->maps.optional_map),
> +                            "default autocreate for optional_map");
> +       err |= !ASSERT_FALSE(bpf_map__autocreate(skel->maps.optional_map2),
> +                           "default autocreate for optional_map2");
> +       if (err)
>                 goto cleanup;

The above is a bit ugly. I think we can keep it simple just by not
exiting early, just do

ASSERT_TRUE(...);
ASSERT_TRUE(...);
ASSERT_FALSE(...);
ASSERT_FALSE(...);

.. then continue to bpf_map__set_autocreate() tests, even if previous
assertions already failed.

Nothing will crash, you'll just get more errors, potentially (because
some preconditions weren't set correctly or something).

>
> -       /* 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();
>  }
> diff --git a/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
> index 9a951ee6f55c..ba10c3896213 100644
> --- a/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
> +++ b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
> @@ -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
>

Re: [PATCH bpf-next v3 14/15] bpf: allow '?' at the beginning of DATASEC names

[Andrii Nakryiko]

On Mon, Mar 4, 2024 at 2:52 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> 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 <eddyz87@gmail.com>
> ---
>  kernel/bpf/btf.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 6ff0bd1a91d5..a25fb6bce808 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -761,12 +761,13 @@ static bool btf_name_offset_valid(const struct btf *btf, u32 offset)
>         return offset < btf->hdr.str_len;
>  }
>
> -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.

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

Re: [PATCH bpf-next v3 03/15] libbpf: honor autocreate flag for struct_ops maps

[Eduard Zingerman]

On Tue, 2024-03-05 at 11:19 -0800, Andrii Nakryiko wrote:
[...]

> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 2c0cb72bc7a4..25c452c20d7d 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -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)
> 
> >                         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(...)

Ok, will do

[...]

Re: [PATCH bpf-next v3 05/15] selftests/bpf: utility functions to capture libbpf log in test_progs

[Eduard Zingerman]

On Tue, 2024-03-05 at 11:24 -0800, Andrii Nakryiko wrote:
[...]

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

Ok, will do

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

LIBBPF_DEBUG messages are not necessary for my tests,
so I'll move this down after verbosity check,
could be changed later if need be.

Re: [PATCH bpf-next v3 06/15] selftests/bpf: bad_struct_ops test

[Eduard Zingerman]

On Tue, 2024-03-05 at 11:29 -0800, Andrii Nakryiko wrote:
> On Mon, Mar 4, 2024 at 2:52 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > When loading struct_ops programs kernel requires BTF id of the
> > struct_ops type and member index for attachment point inside that
> > type. This makes it not possible to have same BPF program used in
> > struct_ops maps that have different struct_ops type.
> > Check if libbpf rejects such BPF objects files.
> > 
> > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > ---
> >  .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 24 +++++++++++++
> >  .../selftests/bpf/bpf_testmod/bpf_testmod.h   |  4 +++
> >  .../selftests/bpf/prog_tests/bad_struct_ops.c | 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 <andrii@kernel.org>

Newly added test case struct_ops_multi_pages does not define
bpf_testmod_ops->test_1 field for struct_ops map,
so the following code from my patch caused null pointer dereference:

       563  static int bpf_dummy_reg(void *kdata)
       564  {
       565          struct bpf_testmod_ops *ops = kdata;
       566
       567          ops->test_1();

Fixed by adding "if (ops)" check.

[...]

Re: [PATCH bpf-next v3 08/15] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Eduard Zingerman]

On Tue, 2024-03-05 at 11:46 -0800, Andrii Nakryiko wrote:
[...]

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

I like the version in the patch more but ok,
will switch to suggested scheme as it is a bit simpler.

[...]

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

Yeah, should have been >=, my bad.

Re: [PATCH bpf-next v3 09/15] selftests/bpf: verify struct_ops autoload/autocreate sync

[Eduard Zingerman]

On Tue, 2024-03-05 at 11:48 -0800, Andrii Nakryiko wrote:
[...]

> > +/* 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;
> > +
> > +       err = struct_ops_autocreate2__load(skel);
> > +       if (ASSERT_OK(err, "struct_ops_autocreate__load"))
> > +               goto cleanup;
> > +
> > +
> 
> let's assert autoload state here as well?

Good idea!
Did not occur to me to check this after load for some reason.

[...]

Re: [PATCH bpf-next v3 10/15] libbpf: replace elf_state->st_ops_* fields with SEC_ST_OPS sec_type

[Eduard Zingerman]

On Tue, 2024-03-05 at 11:53 -0800, Andrii Nakryiko wrote:
> On Mon, Mar 4, 2024 at 2:52 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > 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 <eddyz87@gmail.com>
> > ---
> >  tools/lib/bpf/libbpf.c | 62 ++++++++++++++++++++++--------------------
> >  1 file changed, 33 insertions(+), 29 deletions(-)
> > 
> 
> Makes sense and a good generalization!

Thank you.

[...]

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

Done

[...]

Re: [PATCH bpf-next v3 11/15] libbpf: struct_ops in SEC("?.struct_ops") and SEC("?.struct_ops.link")

[Eduard Zingerman]

On Tue, 2024-03-05 at 11:55 -0800, Andrii Nakryiko wrote:
[...]

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

This is better, thank you.

Re: [PATCH bpf-next v3 12/15] libbpf: rewrite btf datasec names starting from '?'

[Eduard Zingerman]

On Tue, 2024-03-05 at 12:03 -0800, Andrii Nakryiko wrote:
[...]

> > @@ -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 == '?')` ?

Ok.

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

Well, in theory this string is shared between several locations,
though this is probably highly unlikely.
Anyways, I made requested change.

[...]

Re: [PATCH bpf-next v3 14/15] bpf: allow '?' at the beginning of DATASEC names

[Eduard Zingerman]

On Tue, 2024-03-05 at 13:43 -0800, Andrii Nakryiko wrote:
[...]

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

Makes sense to me.

Thank you for reviewing this patch-set, I've applied the changes,
will share v4 tomorrow.