[bpf-next] bpf: Use named fields for certain bpf uapi structs

Commit Message

Yonghong Song Nov. 4, 2023, 2:49 a.m. UTC

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/[e-mail]/
  [2] https://lore.kernel.org/bpf/[e-mail]/

Cc: Vadim Fedorenko <[e-mail]>
Cc: Martin KaFai Lau <[e-mail]>
Suggested-by: Andrii Nakryiko <[e-mail]>
Signed-off-by: Yonghong Song <[e-mail]>
---
 include/uapi/linux/bpf.h       | 23 +++++++----------------
 tools/include/uapi/linux/bpf.h | 23 +++++++----------------
 2 files changed, 14 insertions(+), 32 deletions(-)

Comments

Andrii Nakryiko Nov. 4, 2023, 3:21 a.m. UTC | #1

On Fri, Nov 3, 2023 at 7:49 PM Yonghong Song <[e-mail]> 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/[e-mail]/
>   [2] https://lore.kernel.org/bpf/[e-mail]/
>
> Cc: Vadim Fedorenko <[e-mail]>
> Cc: Martin KaFai Lau <[e-mail]>
> Suggested-by: Andrii Nakryiko <[e-mail]>
> Signed-off-by: Yonghong Song <[e-mail]>
> ---
>  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 <[e-mail]>

[...]

[e-mail] Nov. 9, 2023, 7:20 p.m. UTC | #2

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <[e-mail]>:

On Fri,  3 Nov 2023 19:49:00 -0700 you 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;
>     ...
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpf: Use named fields for certain bpf uapi structs
    https://git.kernel.org/bpf/bpf-next/c/e80742d91749

You are awesome, thank you!

Patch

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 0f6cdf52b1da..095ca7238ac2 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -7151,40 +7151,31 @@  struct bpf_spin_lock {
 };
 
 struct bpf_timer {
-	__u64 :64;
-	__u64 :64;
+	__u64 __opaque[2];
 } __attribute__((aligned(8)));
 
 struct bpf_dynptr {
-	__u64 :64;
-	__u64 :64;
+	__u64 __opaque[2];
 } __attribute__((aligned(8)));
 
 struct bpf_list_head {
-	__u64 :64;
-	__u64 :64;
+	__u64 __opaque[2];
 } __attribute__((aligned(8)));
 
 struct bpf_list_node {
-	__u64 :64;
-	__u64 :64;
-	__u64 :64;
+	__u64 __opaque[3];
 } __attribute__((aligned(8)));
 
 struct bpf_rb_root {
-	__u64 :64;
-	__u64 :64;
+	__u64 __opaque[2];
 } __attribute__((aligned(8)));
 
 struct bpf_rb_node {
-	__u64 :64;
-	__u64 :64;
-	__u64 :64;
-	__u64 :64;
+	__u64 __opaque[4];
 } __attribute__((aligned(8)));
 
 struct bpf_refcount {
-	__u32 :32;
+	__u32 __opaque[1];
 } __attribute__((aligned(4)));
 
 struct bpf_sysctl {
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 0f6cdf52b1da..095ca7238ac2 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -7151,40 +7151,31 @@  struct bpf_spin_lock {
 };
 
 struct bpf_timer {
-	__u64 :64;
-	__u64 :64;
+	__u64 __opaque[2];
 } __attribute__((aligned(8)));
 
 struct bpf_dynptr {
-	__u64 :64;
-	__u64 :64;
+	__u64 __opaque[2];
 } __attribute__((aligned(8)));
 
 struct bpf_list_head {
-	__u64 :64;
-	__u64 :64;
+	__u64 __opaque[2];
 } __attribute__((aligned(8)));
 
 struct bpf_list_node {
-	__u64 :64;
-	__u64 :64;
-	__u64 :64;
+	__u64 __opaque[3];
 } __attribute__((aligned(8)));
 
 struct bpf_rb_root {
-	__u64 :64;
-	__u64 :64;
+	__u64 __opaque[2];
 } __attribute__((aligned(8)));
 
 struct bpf_rb_node {
-	__u64 :64;
-	__u64 :64;
-	__u64 :64;
-	__u64 :64;
+	__u64 __opaque[4];
 } __attribute__((aligned(8)));
 
 struct bpf_refcount {
-	__u32 :32;
+	__u32 __opaque[1];
 } __attribute__((aligned(4)));
 
 struct bpf_sysctl {