From patchwork Fri Nov 3 05:52:18 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yonghong Song X-Patchwork-Id: 13444331 X-Patchwork-Delegate: bpf@iogearbox.net Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 361BD7461 for ; Fri, 3 Nov 2023 06:11:02 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=none X-Greylist: delayed 1103 seconds by postgrey-1.37 at lindbergh.monkeyblade.net; Thu, 02 Nov 2023 23:10:56 PDT Received: from 66-220-155-178.mail-mxout.facebook.com (66-220-155-178.mail-mxout.facebook.com [66.220.155.178]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 92D641B4 for ; Thu, 2 Nov 2023 23:10:56 -0700 (PDT) Received: by devbig309.ftw3.facebook.com (Postfix, from userid 128203) id 051132942BA2C; Thu, 2 Nov 2023 22:52:19 -0700 (PDT) From: Yonghong Song To: bpf@vger.kernel.org Cc: Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , kernel-team@fb.com, Martin KaFai Lau , Vadim Fedorenko , Martin KaFai Lau Subject: [RFC PATCH bpf-next] libbpf: bpftool : Emit aligned(8) attr for empty struct in btf source dump Date: Thu, 2 Nov 2023 22:52:18 -0700 Message-Id: <20231103055218.2395034-1-yonghong.song@linux.dev> X-Mailer: git-send-email 2.34.1 Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Patchwork-Delegate: bpf@iogearbox.net X-Patchwork-State: RFC Martin and Vadim reported a verifier failure with bpf_dynptr usage. The issue is mentioned but Vadim workarounded the issue with source change ([1]). The below describes what is the issue and why there is a verification failure. int BPF_PROG(skb_crypto_setup) { struct bpf_dynptr algo, key; ... bpf_dynptr_from_mem(..., ..., 0, &algo); ... } The bpf program is using vmlinux.h, so we have the following definition in vmlinux.h: struct bpf_dynptr { long: 64; long: 64; }; Note that in uapi header bpf.h, we have struct bpf_dynptr { long: 64; long: 64; } __attribute__((aligned(8))); So we lost alignment information for struct bpf_dynptr by using vmlinux.h. Let us take a look at a simple program below: $ cat align.c typedef unsigned long long __u64; struct bpf_dynptr_no_align { __u64 :64; __u64 :64; }; struct bpf_dynptr_yes_align { __u64 :64; __u64 :64; } __attribute__((aligned(8))); void bar(void *, void *); int foo() { struct bpf_dynptr_no_align a; struct bpf_dynptr_yes_align b; bar(&a, &b); return 0; } $ clang --target=bpf -O2 -S -emit-llvm align.c Look at the generated IR file align.ll: ... %a = alloca %struct.bpf_dynptr_no_align, align 1 %b = alloca %struct.bpf_dynptr_yes_align, align 8 ... The compiler dictates the alignment for struct bpf_dynptr_no_align is 1 and the alignment for struct bpf_dynptr_yes_align is 8. So theoretically compiler could allocate variable %a with alignment 1 although in reallity the compiler may choose a different alignment by considering other variables. In [1], the verification failure happens because variable 'algo' is allocated on the stack with alignment 4 (fp-28). But the verifer wants its alignment to be 8. To fix the issue, the aligned(8) attribute should be emitted for those special uapi structs (bpf_dynptr etc.) whose values will be used by kernel helpers or kfuncs. For example, the following bpf_dynptr type will be generated in vmlinux.h: struct bpf_dynptr { long: 64; long: 64; } __attribute__((aligned(8))); There are a few ways to do this: (1). this patch added an option 'empty_struct_align8' in 'btf_dump_opts', and bpftool will enable this option so libbpf will emit aligned(8) for empty structs. The only drawback is that some other non-bpf-uapi empty structs may be marked as well but this does not have any real impact. (2). Only add aligned(8) if the struct having 'bpf_' prefix. Similar to (1), the action is controlled with an option in 'btf_dump_opts'. Also, not sure whether adding an option in 'btf_dump_opts' is the best solution or not. Another possibility is to add an option to btf_dump__dump_type() with a different function name, e.g., btf_dump__dump_type_opts() but it makes the function is not consistent with btf_dump__emit_type_decl(). So send this patch as RFC due to above different implementation choices. [1] https://lore.kernel.org/bpf/1b100f73-7625-4c1f-3ae5-50ecf84d3ff0@linux.dev/ Cc: Vadim Fedorenko Cc: Martin KaFai Lau Signed-off-by: Yonghong Song --- tools/bpf/bpftool/btf.c | 5 ++++- tools/lib/bpf/btf.h | 7 ++++++- tools/lib/bpf/btf_dump.c | 7 ++++++- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c index 91fcb75babe3..c9061d476f7d 100644 --- a/tools/bpf/bpftool/btf.c +++ b/tools/bpf/bpftool/btf.c @@ -463,10 +463,13 @@ static void __printf(2, 0) btf_dump_printf(void *ctx, static int dump_btf_c(const struct btf *btf, __u32 *root_type_ids, int root_type_cnt) { + LIBBPF_OPTS(btf_dump_opts, opts, + .empty_struct_align8 = true, + ); struct btf_dump *d; int err = 0, i; - d = btf_dump__new(btf, btf_dump_printf, NULL, NULL); + d = btf_dump__new(btf, btf_dump_printf, NULL, &opts); if (!d) return -errno; diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h index 8e6880d91c84..af88563fe0ff 100644 --- a/tools/lib/bpf/btf.h +++ b/tools/lib/bpf/btf.h @@ -235,8 +235,13 @@ struct btf_dump; struct btf_dump_opts { size_t sz; + /* emit '__attribute__((aligned(8)))' for empty struct, i.e., + * the struct has no named member. + */ + bool empty_struct_align8; + size_t :0; }; -#define btf_dump_opts__last_field sz +#define btf_dump_opts__last_field empty_struct_align8 typedef void (*btf_dump_printf_fn_t)(void *ctx, const char *fmt, va_list args); diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c index 4d9f30bf7f01..fe386d20a43a 100644 --- a/tools/lib/bpf/btf_dump.c +++ b/tools/lib/bpf/btf_dump.c @@ -83,6 +83,7 @@ struct btf_dump { int ptr_sz; bool strip_mods; bool skip_anon_defs; + bool empty_struct_align8; int last_id; /* per-type auxiliary state */ @@ -167,6 +168,7 @@ struct btf_dump *btf_dump__new(const struct btf *btf, d->printf_fn = printf_fn; d->cb_ctx = ctx; d->ptr_sz = btf__pointer_size(btf) ? : sizeof(void *); + d->empty_struct_align8 = OPTS_GET(opts, empty_struct_align8, false); d->type_names = hashmap__new(str_hash_fn, str_equal_fn, NULL); if (IS_ERR(d->type_names)) { @@ -808,7 +810,10 @@ static void btf_dump_emit_type(struct btf_dump *d, __u32 id, __u32 cont_id) if (top_level_def) { btf_dump_emit_struct_def(d, id, t, 0); - btf_dump_printf(d, ";\n\n"); + if (kind == BTF_KIND_UNION || btf_vlen(t) || !d->empty_struct_align8) + btf_dump_printf(d, ";\n\n"); + else + btf_dump_printf(d, " __attribute__((aligned(8)));\n\n"); tstate->emit_state = EMITTED; } else { tstate->emit_state = NOT_EMITTED;