On Fri, Nov 3, 2023 at 7:49 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 local 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 RFC patch ([1]) tried to add '__attribute__((aligned(8)))'
> to struct bpf_dynptr plus other similar structs. Andrii suggested that
> we could directly modify uapi struct with named fields like struct 'bpf_iter_num':
> 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)));
>
> Indeed, adding named fields for those affected structs in this patch can preserve
> alignment when bpf program references them in vmlinux.h. With this patch,
> the verification failure in [1] can also be resolved.
>
> [1] https://lore.kernel.org/bpf/1b100f73-7625-4c1f-3ae5-50ecf84d3ff0@xxxxxxxxx/
> [2] https://lore.kernel.org/bpf/20231103055218.2395034-1-yonghong.song@xxxxxxxxx/
>
> Cc: Vadim Fedorenko <vadfed@xxxxxxxx>
> Cc: Martin KaFai Lau <martin.lau@xxxxxxxxx>
> Suggested-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> Signed-off-by: Yonghong Song <yonghong.song@xxxxxxxxx>
> ---
> include/uapi/linux/bpf.h | 23 +++++++----------------
> tools/include/uapi/linux/bpf.h | 23 +++++++----------------
> 2 files changed, 14 insertions(+), 32 deletions(-)
>
I think that's the best solution, thanks!
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
[...]
