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