On Fri, Nov 3, 2023 at 9:53 AM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
>
>
> On 11/3/23 9:21 AM, Andrii Nakryiko wrote:
> > On Thu, Nov 2, 2023 at 10:52 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
> >> 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.
> >>
> > Let's do what we do for open-coded iterators, add opaque u64s:
> >
> > /* BPF numbers iterator state */
> > struct bpf_iter_num {
> > /* opaque iterator state; having __u64 here allows to preserve correct
> > * alignment requirements in vmlinux.h, generated from BTF
> > */
> > __u64 __opaque[1];
> > } __attribute__((aligned(8)));
>
> Good point. Will do. This will need change uapi struct, I think it is okay.
> with __u64, we should not need aligned(8) attribute, but since uapi header
> already has it like in the above, I can keep it as well.
>
Yes, let's keep aligned(8) as well, it is needed on 32-bit architectures
>
> >
> >
> > I think it's much better than random extra options or having to do
> > what we do with private() macro everywhere:
> >
> > #define private(name) SEC(".bss." #name) __hidden __attribute__((aligned(8)))
> >
> >
> >> [1] https://lore.kernel.org/bpf/1b100f73-7625-4c1f-3ae5-50ecf84d3ff0@xxxxxxxxx/
> >>
> >> Cc: Vadim Fedorenko <vadfed@xxxxxxxx>
> >> Cc: Martin KaFai Lau <martin.lau@xxxxxxxxx>
> >> Signed-off-by: Yonghong Song <yonghong.song@xxxxxxxxx>
> >> ---
> >> 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;
> >> --
> >> 2.34.1
> >>
