diff mbox series

[RFC,v2,2/3] bpftool: add attribute preserve_static_offset for context types

Message ID 20231212023136.7021-3-eddyz87@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series use preserve_static_offset in bpf uapi headers | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch, async
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-9 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-17 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-12 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-15 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-14 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-3 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-10 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-18 / veristat

Commit Message

Eduard Zingerman Dec. 12, 2023, 2:31 a.m. UTC 
When printing vmlinux.h emit attribute preserve_static_offset [0] for
types that are used as context parameters for BPF programs. To avoid
hacking libbpf dump logic emit forward declarations annotated with
attribute. Such forward declarations have to come before structure
definitions.

Only emit such forward declarations when context types are present in
target BTF (identified by name).

C language standard wording in section "6.7.2.1 Structure and union
specifiers" [1] is vague, but example in 6.7.2.1.21 explicitly allows
such notation, and it matches clang behavior.

Here is how 'bpftool btf gen ... format c' looks after this change:

    #ifndef __VMLINUX_H__
    #define __VMLINUX_H__

    #if !defined(BPF_NO_PRESERVE_STATIC_OFFSET) && \
        __has_attribute(preserve_static_offset)
    #pragma clang attribute push \
              (__attribute__((preserve_static_offset)), apply_to = record)

    struct bpf_cgroup_dev_ctx;
    ...

    #pragma clang attribute pop
    #endif

    ... rest of the output unchanged ...

This is a follow up for discussion in thread [2].

[0] https://clang.llvm.org/docs/AttributeReference.html#preserve-static-offset
[1] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3088.pdf
[2] https://lore.kernel.org/bpf/20231208000531.19179-1-eddyz87@gmail.com/

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/bpf/bpftool/btf.c | 131 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 116 insertions(+), 15 deletions(-)

Comments

Quentin Monnet Dec. 12, 2023, 11:39 a.m. UTC | #1 
2023-12-12 02:32 UTC+0000 ~ Eduard Zingerman <eddyz87@gmail.com>
> When printing vmlinux.h emit attribute preserve_static_offset [0] for
> types that are used as context parameters for BPF programs. To avoid
> hacking libbpf dump logic emit forward declarations annotated with
> attribute. Such forward declarations have to come before structure
> definitions.
> 
> Only emit such forward declarations when context types are present in
> target BTF (identified by name).
> 
> C language standard wording in section "6.7.2.1 Structure and union
> specifiers" [1] is vague, but example in 6.7.2.1.21 explicitly allows
> such notation, and it matches clang behavior.
> 
> Here is how 'bpftool btf gen ... format c' looks after this change:
> 
>     #ifndef __VMLINUX_H__
>     #define __VMLINUX_H__
> 
>     #if !defined(BPF_NO_PRESERVE_STATIC_OFFSET) && \
>         __has_attribute(preserve_static_offset)
>     #pragma clang attribute push \
>               (__attribute__((preserve_static_offset)), apply_to = record)
> 
>     struct bpf_cgroup_dev_ctx;
>     ...
> 
>     #pragma clang attribute pop
>     #endif
> 
>     ... rest of the output unchanged ...
> 
> This is a follow up for discussion in thread [2].
> 
> [0] https://clang.llvm.org/docs/AttributeReference.html#preserve-static-offset
> [1] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3088.pdf
> [2] https://lore.kernel.org/bpf/20231208000531.19179-1-eddyz87@gmail.com/
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  tools/bpf/bpftool/btf.c | 131 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 116 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 91fcb75babe3..2abe71194afb 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -460,11 +460,118 @@ static void __printf(2, 0) btf_dump_printf(void *ctx,
>  	vfprintf(stdout, fmt, args);
>  }
>  
> +static const char * const context_types[] = {
> +	"bpf_cgroup_dev_ctx",
> +	"bpf_nf_ctx",
> +	"bpf_perf_event_data",
> +	"bpf_raw_tracepoint_args",
> +	"bpf_sk_lookup",
> +	"bpf_sock",
> +	"bpf_sock_addr",
> +	"bpf_sock_ops",
> +	"bpf_sockopt",
> +	"bpf_sysctl",
> +	"__sk_buff",
> +	"sk_msg_md",
> +	"sk_reuseport_md",
> +	"xdp_md",
> +	"pt_regs",
> +};

Hi, and thanks for this!

Apologies for missing the discussion on v1. Reading through the previous
thread I see that they were votes in favour of the hard-coded approach,
but I would ask folks to please reconsider.

I'm not keen on taking this list in bpftool, it doesn't seem to be the
right place for that. I understand there's no plan to add new mirror
context structs, but if we change policy for whatever reason, we're sure
to miss the update in this list and that's a bug in bpftool. If bpftool
ever gets ported to Windows and Windows needs support for new structs,
that's more juggling to do to support multiple platforms. And if any
tool other than bpftool needs to generate vmlinux.h headers in the
future, it's back to square one - although by then there might be extra
pushback for changing the BTF, if bpftool already does the work.

Like Alan, I rather share your own inclination towards the more generic
declaration tags approach, if you don't mind the additional work.

Quentin
Eduard Zingerman Dec. 12, 2023, 3:58 p.m. UTC | #2 
On Tue, 2023-12-12 at 11:39 +0000, Quentin Monnet wrote:
[...]
> Hi, and thanks for this!
> 
> Apologies for missing the discussion on v1. Reading through the previous
> thread I see that they were votes in favour of the hard-coded approach,
> but I would ask folks to please reconsider.
> 
> I'm not keen on taking this list in bpftool, it doesn't seem to be the
> right place for that. I understand there's no plan to add new mirror
> context structs, but if we change policy for whatever reason, we're sure
> to miss the update in this list and that's a bug in bpftool. If bpftool
> ever gets ported to Windows and Windows needs support for new structs,
> that's more juggling to do to support multiple platforms. And if any
> tool other than bpftool needs to generate vmlinux.h headers in the
> future, it's back to square one - although by then there might be extra
> pushback for changing the BTF, if bpftool already does the work.
> 
> Like Alan, I rather share your own inclination towards the more generic
> declaration tags approach, if you don't mind the additional work.

Understood, thank you for feedback.
The second option is to:

1. Define __bpf_ctx macro in linux headers as follows:

    #if __has_attribute(preserve_static_offset) && defined(__bpf__)
    #define __bpf_ctx __attribute__((preserve_static_offset)) \
                      __attribute__((btf_decl_tag(preserve_static_offset)))
    #else
    #define __bpf_ctx
    #endif

2a. Update libbpf to emit __attribute__((preserve_static_offset)) when
    corresponding decl tag is present in the BTF.

2b. Update bpftool to emit __attribute__((preserve_static_offset)) for
    types with corresponding decl tag. (Like in this patch but check
    for decl tag instead of name).

I think that 2b is better, because emitting
BPF_NO_PRESERVE_STATIC_OFFSET from the same place where
BPF_NO_PRESERVE_ACCESS_INDEX makes more sense,
libbpf does not emit any macro definitions at the moment.

wdyt?
Quentin Monnet Dec. 12, 2023, 4:07 p.m. UTC | #3 
2023-12-12 15:58 UTC+0000 ~ Eduard Zingerman <eddyz87@gmail.com>
> On Tue, 2023-12-12 at 11:39 +0000, Quentin Monnet wrote:
> [...]
>> Hi, and thanks for this!
>>
>> Apologies for missing the discussion on v1. Reading through the previous
>> thread I see that they were votes in favour of the hard-coded approach,
>> but I would ask folks to please reconsider.
>>
>> I'm not keen on taking this list in bpftool, it doesn't seem to be the
>> right place for that. I understand there's no plan to add new mirror
>> context structs, but if we change policy for whatever reason, we're sure
>> to miss the update in this list and that's a bug in bpftool. If bpftool
>> ever gets ported to Windows and Windows needs support for new structs,
>> that's more juggling to do to support multiple platforms. And if any
>> tool other than bpftool needs to generate vmlinux.h headers in the
>> future, it's back to square one - although by then there might be extra
>> pushback for changing the BTF, if bpftool already does the work.
>>
>> Like Alan, I rather share your own inclination towards the more generic
>> declaration tags approach, if you don't mind the additional work.
> 
> Understood, thank you for feedback.
> The second option is to:
> 
> 1. Define __bpf_ctx macro in linux headers as follows:
> 
>     #if __has_attribute(preserve_static_offset) && defined(__bpf__)
>     #define __bpf_ctx __attribute__((preserve_static_offset)) \
>                       __attribute__((btf_decl_tag(preserve_static_offset)))
>     #else
>     #define __bpf_ctx
>     #endif
> 
> 2a. Update libbpf to emit __attribute__((preserve_static_offset)) when
>     corresponding decl tag is present in the BTF.
> 
> 2b. Update bpftool to emit __attribute__((preserve_static_offset)) for
>     types with corresponding decl tag. (Like in this patch but check
>     for decl tag instead of name).

I don't have a strong opinion on that part, so...

> I think that 2b is better, because emitting
> BPF_NO_PRESERVE_STATIC_OFFSET from the same place where
> BPF_NO_PRESERVE_ACCESS_INDEX makes more sense,
> libbpf does not emit any macro definitions at the moment.

... the above makes sense, I'd say let's go for this if nobody else
objects (or wants it in libbpf instead - but bpftool is fine as far as
I'm concerned).

Thanks,
Quentin
Yonghong Song Dec. 13, 2023, 4:53 a.m. UTC | #4 
On 12/12/23 8:07 AM, Quentin Monnet wrote:
> 2023-12-12 15:58 UTC+0000 ~ Eduard Zingerman <eddyz87@gmail.com>
>> On Tue, 2023-12-12 at 11:39 +0000, Quentin Monnet wrote:
>> [...]
>>> Hi, and thanks for this!
>>>
>>> Apologies for missing the discussion on v1. Reading through the previous
>>> thread I see that they were votes in favour of the hard-coded approach,
>>> but I would ask folks to please reconsider.
>>>
>>> I'm not keen on taking this list in bpftool, it doesn't seem to be the
>>> right place for that. I understand there's no plan to add new mirror
>>> context structs, but if we change policy for whatever reason, we're sure
>>> to miss the update in this list and that's a bug in bpftool. If bpftool
>>> ever gets ported to Windows and Windows needs support for new structs,
>>> that's more juggling to do to support multiple platforms. And if any
>>> tool other than bpftool needs to generate vmlinux.h headers in the
>>> future, it's back to square one - although by then there might be extra
>>> pushback for changing the BTF, if bpftool already does the work.
>>>
>>> Like Alan, I rather share your own inclination towards the more generic
>>> declaration tags approach, if you don't mind the additional work.
>> Understood, thank you for feedback.
>> The second option is to:
>>
>> 1. Define __bpf_ctx macro in linux headers as follows:
>>
>>      #if __has_attribute(preserve_static_offset) && defined(__bpf__)
>>      #define __bpf_ctx __attribute__((preserve_static_offset)) \
>>                        __attribute__((btf_decl_tag(preserve_static_offset)))
>>      #else
>>      #define __bpf_ctx
>>      #endif
>>
>> 2a. Update libbpf to emit __attribute__((preserve_static_offset)) when
>>      corresponding decl tag is present in the BTF.
>>
>> 2b. Update bpftool to emit __attribute__((preserve_static_offset)) for
>>      types with corresponding decl tag. (Like in this patch but check
>>      for decl tag instead of name).
> I don't have a strong opinion on that part, so...
>
>> I think that 2b is better, because emitting
>> BPF_NO_PRESERVE_STATIC_OFFSET from the same place where
>> BPF_NO_PRESERVE_ACCESS_INDEX makes more sense,
>> libbpf does not emit any macro definitions at the moment.
> ... the above makes sense, I'd say let's go for this if nobody else
> objects (or wants it in libbpf instead - but bpftool is fine as far as
> I'm concerned).

Thanks, Eduard and Quentin, my original proposal for hardcoded list is
for simplicity. Eduard, could you help also do an implementation like
you proposed in the above so we can then compare each other?

[...]
diff mbox series

Patch

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 91fcb75babe3..2abe71194afb 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -460,11 +460,118 @@  static void __printf(2, 0) btf_dump_printf(void *ctx,
 	vfprintf(stdout, fmt, args);
 }
 
+static const char * const context_types[] = {
+	"bpf_cgroup_dev_ctx",
+	"bpf_nf_ctx",
+	"bpf_perf_event_data",
+	"bpf_raw_tracepoint_args",
+	"bpf_sk_lookup",
+	"bpf_sock",
+	"bpf_sock_addr",
+	"bpf_sock_ops",
+	"bpf_sockopt",
+	"bpf_sysctl",
+	"__sk_buff",
+	"sk_msg_md",
+	"sk_reuseport_md",
+	"xdp_md",
+	"pt_regs",
+};
+
+static bool is_context_type_name(const struct btf *btf, const char *name)
+{
+	__u32 i;
+
+	for (i = 0; i < ARRAY_SIZE(context_types); ++i)
+		if (strcmp(name, context_types[i]) == 0)
+			return true;
+
+	return false;
+}
+
+/* When root_type_ids == NULL represents an iterator
+ * over all type ids in BTF: [1 .. btf__type_cnt(btf)].
+ *
+ * When root_type_ids != NULL represents an iterator
+ * over all type ids in root_type_ids array.
+ */
+struct root_type_iter {
+	__u32 *root_type_ids;
+	__u32 cnt;
+	__u32 pos;
+};
+
+static struct root_type_iter make_root_type_iter(const struct btf *btf,
+						 __u32 *root_type_ids, int root_type_cnt)
+{
+	if (root_type_cnt)
+		return (struct root_type_iter) { root_type_ids, root_type_cnt, 0 };
+
+	return (struct root_type_iter) { NULL, btf__type_cnt(btf), 1 };
+}
+
+static __u32 root_type_iter_next(struct root_type_iter *iter)
+{
+	if (iter->pos >= iter->cnt)
+		return 0;
+
+	if (iter->root_type_ids)
+		return iter->root_type_ids[iter->pos++];
+
+	return iter->pos++;
+}
+
+/* Iterate all types in 'btf', if there are types with name matching
+ * name of a BPF program context parameter type - emit a forward
+ * declaration for this type annotated with preserve_static_offset
+ * attribute [0].
+ *
+ * [0] https://clang.llvm.org/docs/AttributeReference.html#preserve-static-offset
+ */
+static void emit_static_offset_protos(const struct btf *btf, struct root_type_iter iter)
+{
+	bool first = true;
+	__u32 id;
+
+	while ((id = root_type_iter_next(&iter))) {
+		const struct btf_type *t;
+		const char *name;
+
+		t = btf__type_by_id(btf, id);
+		if (!t)
+			continue;
+
+		name = btf__name_by_offset(btf, t->name_off);
+		if (!name)
+			continue;
+
+		if (!btf_is_struct(t) || !is_context_type_name(btf, name))
+			continue;
+
+		if (first) {
+			first = false;
+			printf("#if !defined(BPF_NO_PRESERVE_STATIC_OFFSET) && __has_attribute(preserve_static_offset)\n");
+			printf("#pragma clang attribute push (__attribute__((preserve_static_offset)), apply_to = record)\n");
+			printf("\n");
+		}
+
+		printf("struct %s;\n", name);
+	}
+
+	if (!first) {
+		printf("\n");
+		printf("#pragma clang attribute pop\n");
+		printf("#endif /* BPF_NO_PRESERVE_STATIC_OFFSET */\n\n");
+	}
+}
+
 static int dump_btf_c(const struct btf *btf,
 		      __u32 *root_type_ids, int root_type_cnt)
 {
+	struct root_type_iter iter;
 	struct btf_dump *d;
-	int err = 0, i;
+	int err = 0;
+	__u32 id;
 
 	d = btf_dump__new(btf, btf_dump_printf, NULL, NULL);
 	if (!d)
@@ -473,24 +580,18 @@  static int dump_btf_c(const struct btf *btf,
 	printf("#ifndef __VMLINUX_H__\n");
 	printf("#define __VMLINUX_H__\n");
 	printf("\n");
+
+	emit_static_offset_protos(btf, make_root_type_iter(btf, root_type_ids, root_type_cnt));
+
 	printf("#ifndef BPF_NO_PRESERVE_ACCESS_INDEX\n");
 	printf("#pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record)\n");
 	printf("#endif\n\n");
 
-	if (root_type_cnt) {
-		for (i = 0; i < root_type_cnt; i++) {
-			err = btf_dump__dump_type(d, root_type_ids[i]);
-			if (err)
-				goto done;
-		}
-	} else {
-		int cnt = btf__type_cnt(btf);
-
-		for (i = 1; i < cnt; i++) {
-			err = btf_dump__dump_type(d, i);
-			if (err)
-				goto done;
-		}
+	iter = make_root_type_iter(btf, root_type_ids, root_type_cnt);
+	while ((id = root_type_iter_next(&iter))) {
+		err = btf_dump__dump_type(d, id);
+		if (err)
+			goto done;
 	}
 
 	printf("#ifndef BPF_NO_PRESERVE_ACCESS_INDEX\n");