| Message ID | 20230302231924.344383-1-davemarchevsky@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] selftests/bpf: Add -Wuninitialized flag to bpf prog flags | expand |
On Thu, Mar 2, 2023 at 3:19 PM Dave Marchevsky <davemarchevsky@fb.com> wrote: > > --- a/tools/testing/selftests/bpf/progs/rbtree_fail.c > +++ b/tools/testing/selftests/bpf/progs/rbtree_fail.c > @@ -232,8 +232,9 @@ long rbtree_api_first_release_unlock_escape(void *ctx) > > bpf_spin_lock(&glock); > res = bpf_rbtree_first(&groot); > - if (res) > - n = container_of(res, struct node_data, node); > + if (!res) > + return -1; The verifier cannot be ok with this return... I hope... > + n = container_of(res, struct node_data, node); > bpf_spin_unlock(&glock);
On Thu, Mar 02, 2023 at 03:19:24PM -0800, Dave Marchevsky wrote: > Per C99 standard [0], Section 6.7.8, Paragraph 10: > > If an object that has automatic storage duration is not initialized > explicitly, its value is indeterminate. > > And in the same document, in appendix "J.2 Undefined behavior": > > The behavior is undefined in the following circumstances: > [...] > The value of an object with automatic storage duration is used while > it is indeterminate (6.2.4, 6.7.8, 6.8). > > This means that use of an uninitialized stack variable is undefined > behavior, and therefore that clang can choose to do a variety of scary > things, such as not generating bytecode for "bunch of useful code" in > the below example: > > void some_func() > { > int i; > if (!i) > return; > // bunch of useful code > } > > To add insult to injury, if some_func above is a helper function for > some BPF program, clang can choose to not generate an "exit" insn, > causing verifier to fail with "last insn is not an exit or jmp". Going > from that verification failure to the root cause of uninitialized use > is certain to be frustrating. > > This patch adds -Wuninitialized to the cflags for selftest BPF progs and > fixes up existing instances of uninitialized use. > > [0]: https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > Cc: David Vernet <void@manifault.com> > Cc: Tejun Heo <tj@kernel.org> Acked-by: David Vernet <void@manifault.com> > --- > tools/testing/selftests/bpf/Makefile | 2 +- > tools/testing/selftests/bpf/progs/rbtree.c | 2 +- > tools/testing/selftests/bpf/progs/rbtree_fail.c | 5 +++-- > .../selftests/bpf/progs/test_kfunc_dynptr_param.c | 2 +- > .../testing/selftests/bpf/progs/test_sk_lookup_kern.c | 2 +- > tools/testing/selftests/bpf/progs/test_tunnel_kern.c | 10 +++++----- > 6 files changed, 12 insertions(+), 11 deletions(-) > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > index f40606a85a0f..eab3cf5399f5 100644 > --- a/tools/testing/selftests/bpf/Makefile > +++ b/tools/testing/selftests/bpf/Makefile > @@ -357,7 +357,7 @@ BPF_CFLAGS = -g -Werror -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) \ > -I$(abspath $(OUTPUT)/../usr/include) > > CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \ > - -Wno-compare-distinct-pointer-types > + -Wno-compare-distinct-pointer-types -Wuninitialized > > $(OUTPUT)/test_l4lb_noinline.o: BPF_CFLAGS += -fno-inline > $(OUTPUT)/test_xdp_noinline.o: BPF_CFLAGS += -fno-inline > diff --git a/tools/testing/selftests/bpf/progs/rbtree.c b/tools/testing/selftests/bpf/progs/rbtree.c > index e5db1a4287e5..4c90aa6abddd 100644 > --- a/tools/testing/selftests/bpf/progs/rbtree.c > +++ b/tools/testing/selftests/bpf/progs/rbtree.c > @@ -75,7 +75,7 @@ SEC("tc") > long rbtree_add_and_remove(void *ctx) > { > struct bpf_rb_node *res = NULL; > - struct node_data *n, *m; > + struct node_data *n, *m = NULL; > > n = bpf_obj_new(typeof(*n)); > if (!n) > diff --git a/tools/testing/selftests/bpf/progs/rbtree_fail.c b/tools/testing/selftests/bpf/progs/rbtree_fail.c > index bf3cba115897..3368f4b05ca0 100644 > --- a/tools/testing/selftests/bpf/progs/rbtree_fail.c > +++ b/tools/testing/selftests/bpf/progs/rbtree_fail.c > @@ -232,8 +232,9 @@ long rbtree_api_first_release_unlock_escape(void *ctx) > > bpf_spin_lock(&glock); > res = bpf_rbtree_first(&groot); > - if (res) > - n = container_of(res, struct node_data, node); > + if (!res) > + return -1; > + n = container_of(res, struct node_data, node); > bpf_spin_unlock(&glock); > > bpf_spin_lock(&glock); > diff --git a/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c b/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c > index 2fbef3cc7ad8..2dde8e3fe4c9 100644 > --- a/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c > +++ b/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c > @@ -48,7 +48,7 @@ SEC("?lsm.s/bpf") > __failure __msg("arg#0 expected pointer to stack or dynptr_ptr") > int BPF_PROG(not_ptr_to_stack, int cmd, union bpf_attr *attr, unsigned int size) > { > - unsigned long val; > + unsigned long val = 0; > > return bpf_verify_pkcs7_signature((struct bpf_dynptr *)val, > (struct bpf_dynptr *)val, NULL); > diff --git a/tools/testing/selftests/bpf/progs/test_sk_lookup_kern.c b/tools/testing/selftests/bpf/progs/test_sk_lookup_kern.c > index b502e5c92e33..6ccf6d546074 100644 > --- a/tools/testing/selftests/bpf/progs/test_sk_lookup_kern.c > +++ b/tools/testing/selftests/bpf/progs/test_sk_lookup_kern.c > @@ -23,8 +23,8 @@ static struct bpf_sock_tuple *get_tuple(void *data, __u64 nh_off, > bool *ipv4) > { > struct bpf_sock_tuple *result; > + __u64 ihl_len = 0; > __u8 proto = 0; > - __u64 ihl_len; > > if (eth_proto == bpf_htons(ETH_P_IP)) { > struct iphdr *iph = (struct iphdr *)(data + nh_off); > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c > index 508da4a23c4f..95b4aa0928ba 100644 > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c Coincidentally, this seems to also be failing on master. > @@ -324,11 +324,11 @@ int ip4ip6erspan_get_tunnel(struct __sk_buff *skb) > SEC("tc") > int vxlan_set_tunnel_dst(struct __sk_buff *skb) > { > - int ret; > struct bpf_tunnel_key key; > struct vxlan_metadata md; > __u32 index = 0; > __u32 *local_ip = NULL; > + int ret = 0; > > local_ip = bpf_map_lookup_elem(&local_ip_map, &index); > if (!local_ip) { > @@ -363,11 +363,11 @@ int vxlan_set_tunnel_dst(struct __sk_buff *skb) > SEC("tc") > int vxlan_set_tunnel_src(struct __sk_buff *skb) > { > - int ret; > struct bpf_tunnel_key key; > struct vxlan_metadata md; > __u32 index = 0; > __u32 *local_ip = NULL; > + int ret = 0; > > local_ip = bpf_map_lookup_elem(&local_ip_map, &index); > if (!local_ip) { > @@ -494,9 +494,9 @@ SEC("tc") > int ip6vxlan_set_tunnel_dst(struct __sk_buff *skb) > { > struct bpf_tunnel_key key; > - int ret; > __u32 index = 0; > __u32 *local_ip; > + int ret = 0; > > local_ip = bpf_map_lookup_elem(&local_ip_map, &index); > if (!local_ip) { > @@ -525,9 +525,9 @@ SEC("tc") > int ip6vxlan_set_tunnel_src(struct __sk_buff *skb) > { > struct bpf_tunnel_key key; > - int ret; > __u32 index = 0; > __u32 *local_ip; > + int ret = 0; > > local_ip = bpf_map_lookup_elem(&local_ip_map, &index); > if (!local_ip) { > @@ -556,9 +556,9 @@ SEC("tc") > int ip6vxlan_get_tunnel_src(struct __sk_buff *skb) > { > struct bpf_tunnel_key key; > - int ret; > __u32 index = 0; > __u32 *local_ip; > + int ret = 0; > > local_ip = bpf_map_lookup_elem(&local_ip_map, &index); > if (!local_ip) { > -- > 2.30.2 >
On Thu, Mar 02, 2023 at 03:23:22PM -0800, Alexei Starovoitov wrote: > On Thu, Mar 2, 2023 at 3:19 PM Dave Marchevsky <davemarchevsky@fb.com> wrote: > > > > --- a/tools/testing/selftests/bpf/progs/rbtree_fail.c > > +++ b/tools/testing/selftests/bpf/progs/rbtree_fail.c > > @@ -232,8 +232,9 @@ long rbtree_api_first_release_unlock_escape(void *ctx) > > > > bpf_spin_lock(&glock); > > res = bpf_rbtree_first(&groot); > > - if (res) > > - n = container_of(res, struct node_data, node); > > + if (!res) > > + return -1; > > The verifier cannot be ok with this return... I hope... This is a negative testcase which correctly fails, though the error message wasn't what I was expecting to see: __failure __msg("rbtree_remove node input must be non-owning ref") Something about the lock still being held seems much more intuitive. > > > + n = container_of(res, struct node_data, node); > > bpf_spin_unlock(&glock);
On 3/2/23 6:23 PM, Alexei Starovoitov wrote: > On Thu, Mar 2, 2023 at 3:19 PM Dave Marchevsky <davemarchevsky@fb.com> wrote: >> >> --- a/tools/testing/selftests/bpf/progs/rbtree_fail.c >> +++ b/tools/testing/selftests/bpf/progs/rbtree_fail.c >> @@ -232,8 +232,9 @@ long rbtree_api_first_release_unlock_escape(void *ctx) >> >> bpf_spin_lock(&glock); >> res = bpf_rbtree_first(&groot); >> - if (res) >> - n = container_of(res, struct node_data, node); >> + if (!res) >> + return -1; > > The verifier cannot be ok with this return... I hope... > FWIW it's because the test expects verification failure and the branch taken by verifier produces the expected message before evaluating other branch and complaining about retval: 0: R1=ctx(off=0,imm=0) R10=fp0 ; bpf_spin_lock(&glock); 0: (18) r1 = 0xffff888103c70320 ; R1_w=map_value(off=16,ks=4,vs=40,imm=0) 2: (85) call bpf_spin_lock#93 ; ; res = bpf_rbtree_first(&groot); 3: (18) r1 = 0xffff888103c70310 ; R1_w=map_value(off=0,ks=4,vs=40,imm=0) 5: (85) call bpf_rbtree_first#121960 6: (bf) r6 = r0 ; R0_w=ptr_or_null_node_data(id=1,non_own_ref,off=16,imm=0) R6_w=ptr_or_null_node_data(id=1,non_own_ref,off=16,imm=0) 7: (b7) r0 = -1 ; R0_w=-1 ; if (!res) 8: (15) if r6 == 0x0 goto pc+14 ; R6_w=ptr_node_data(non_own_ref,off=16,imm=0) ; bpf_spin_unlock(&glock); 9: (18) r1 = 0xffff888103c70320 ; R1_w=map_value(off=16,ks=4,vs=40,imm=0) 11: (85) call bpf_spin_unlock#94 ; ; bpf_spin_lock(&glock); 12: (18) r1 = 0xffff888103c70320 ; R1_w=map_value(off=16,ks=4,vs=40,imm=0) 14: (85) call bpf_spin_lock#93 ; ; bpf_rbtree_remove(&groot, &n->node); 15: (18) r1 = 0xffff888103c70310 ; R1_w=map_value(off=0,ks=4,vs=40,imm=0) 17: (bf) r2 = r6 ; R2_w=scalar(id=2) R6=scalar(id=2) 18: (85) call bpf_rbtree_remove#121964 rbtree_remove node input must be non-owning ref Regardless, fixed in v2 >> + n = container_of(res, struct node_data, node); >> bpf_spin_unlock(&glock);
On 3/2/23 6:29 PM, David Vernet wrote: > On Thu, Mar 02, 2023 at 03:23:22PM -0800, Alexei Starovoitov wrote: >> On Thu, Mar 2, 2023 at 3:19 PM Dave Marchevsky <davemarchevsky@fb.com> wrote: >>> >>> --- a/tools/testing/selftests/bpf/progs/rbtree_fail.c >>> +++ b/tools/testing/selftests/bpf/progs/rbtree_fail.c >>> @@ -232,8 +232,9 @@ long rbtree_api_first_release_unlock_escape(void *ctx) >>> >>> bpf_spin_lock(&glock); >>> res = bpf_rbtree_first(&groot); >>> - if (res) >>> - n = container_of(res, struct node_data, node); >>> + if (!res) >>> + return -1; >> >> The verifier cannot be ok with this return... I hope... > > This is a negative testcase which correctly fails, though the error > message wasn't what I was expecting to see: > > __failure __msg("rbtree_remove node input must be non-owning ref") > > Something about the lock still being held seems much more intuitive. > It's necessary to call bpf_rbtree_remove w/ lock held. This test expects to fail because non-owning ref "n" is clobbered after the critical section where it's returned by bpf_rbtree_first ends. >> >>> + n = container_of(res, struct node_data, node); >>> bpf_spin_unlock(&glock);
On Thu, Mar 02, 2023 at 06:41:55PM -0500, Dave Marchevsky wrote: > On 3/2/23 6:29 PM, David Vernet wrote: > > On Thu, Mar 02, 2023 at 03:23:22PM -0800, Alexei Starovoitov wrote: > >> On Thu, Mar 2, 2023 at 3:19 PM Dave Marchevsky <davemarchevsky@fb.com> wrote: > >>> > >>> --- a/tools/testing/selftests/bpf/progs/rbtree_fail.c > >>> +++ b/tools/testing/selftests/bpf/progs/rbtree_fail.c > >>> @@ -232,8 +232,9 @@ long rbtree_api_first_release_unlock_escape(void *ctx) > >>> > >>> bpf_spin_lock(&glock); > >>> res = bpf_rbtree_first(&groot); > >>> - if (res) > >>> - n = container_of(res, struct node_data, node); > >>> + if (!res) > >>> + return -1; > >> > >> The verifier cannot be ok with this return... I hope... > > > > This is a negative testcase which correctly fails, though the error > > message wasn't what I was expecting to see: > > > > __failure __msg("rbtree_remove node input must be non-owning ref") > > > > Something about the lock still being held seems much more intuitive. > > > > It's necessary to call bpf_rbtree_remove w/ lock held. This test expects > to fail because non-owning ref "n" is clobbered after the critical > section where it's returned by bpf_rbtree_first ends. Oh, I see. I think that would arguably be a bit more clear if we added a bpf_spin_unlock() to that return case then. Ideally for a negative test we can keep the number of bugs being tested to 1. I assume that was Alexei's point, which clearly went over my head. > > >> > >>> + n = container_of(res, struct node_data, node); > >>> bpf_spin_unlock(&glock);
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index f40606a85a0f..eab3cf5399f5 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -357,7 +357,7 @@ BPF_CFLAGS = -g -Werror -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) \ -I$(abspath $(OUTPUT)/../usr/include) CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \ - -Wno-compare-distinct-pointer-types + -Wno-compare-distinct-pointer-types -Wuninitialized $(OUTPUT)/test_l4lb_noinline.o: BPF_CFLAGS += -fno-inline $(OUTPUT)/test_xdp_noinline.o: BPF_CFLAGS += -fno-inline diff --git a/tools/testing/selftests/bpf/progs/rbtree.c b/tools/testing/selftests/bpf/progs/rbtree.c index e5db1a4287e5..4c90aa6abddd 100644 --- a/tools/testing/selftests/bpf/progs/rbtree.c +++ b/tools/testing/selftests/bpf/progs/rbtree.c @@ -75,7 +75,7 @@ SEC("tc") long rbtree_add_and_remove(void *ctx) { struct bpf_rb_node *res = NULL; - struct node_data *n, *m; + struct node_data *n, *m = NULL; n = bpf_obj_new(typeof(*n)); if (!n) diff --git a/tools/testing/selftests/bpf/progs/rbtree_fail.c b/tools/testing/selftests/bpf/progs/rbtree_fail.c index bf3cba115897..3368f4b05ca0 100644 --- a/tools/testing/selftests/bpf/progs/rbtree_fail.c +++ b/tools/testing/selftests/bpf/progs/rbtree_fail.c @@ -232,8 +232,9 @@ long rbtree_api_first_release_unlock_escape(void *ctx) bpf_spin_lock(&glock); res = bpf_rbtree_first(&groot); - if (res) - n = container_of(res, struct node_data, node); + if (!res) + return -1; + n = container_of(res, struct node_data, node); bpf_spin_unlock(&glock); bpf_spin_lock(&glock); diff --git a/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c b/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c index 2fbef3cc7ad8..2dde8e3fe4c9 100644 --- a/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c +++ b/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c @@ -48,7 +48,7 @@ SEC("?lsm.s/bpf") __failure __msg("arg#0 expected pointer to stack or dynptr_ptr") int BPF_PROG(not_ptr_to_stack, int cmd, union bpf_attr *attr, unsigned int size) { - unsigned long val; + unsigned long val = 0; return bpf_verify_pkcs7_signature((struct bpf_dynptr *)val, (struct bpf_dynptr *)val, NULL); diff --git a/tools/testing/selftests/bpf/progs/test_sk_lookup_kern.c b/tools/testing/selftests/bpf/progs/test_sk_lookup_kern.c index b502e5c92e33..6ccf6d546074 100644 --- a/tools/testing/selftests/bpf/progs/test_sk_lookup_kern.c +++ b/tools/testing/selftests/bpf/progs/test_sk_lookup_kern.c @@ -23,8 +23,8 @@ static struct bpf_sock_tuple *get_tuple(void *data, __u64 nh_off, bool *ipv4) { struct bpf_sock_tuple *result; + __u64 ihl_len = 0; __u8 proto = 0; - __u64 ihl_len; if (eth_proto == bpf_htons(ETH_P_IP)) { struct iphdr *iph = (struct iphdr *)(data + nh_off); diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c index 508da4a23c4f..95b4aa0928ba 100644 --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c @@ -324,11 +324,11 @@ int ip4ip6erspan_get_tunnel(struct __sk_buff *skb) SEC("tc") int vxlan_set_tunnel_dst(struct __sk_buff *skb) { - int ret; struct bpf_tunnel_key key; struct vxlan_metadata md; __u32 index = 0; __u32 *local_ip = NULL; + int ret = 0; local_ip = bpf_map_lookup_elem(&local_ip_map, &index); if (!local_ip) { @@ -363,11 +363,11 @@ int vxlan_set_tunnel_dst(struct __sk_buff *skb) SEC("tc") int vxlan_set_tunnel_src(struct __sk_buff *skb) { - int ret; struct bpf_tunnel_key key; struct vxlan_metadata md; __u32 index = 0; __u32 *local_ip = NULL; + int ret = 0; local_ip = bpf_map_lookup_elem(&local_ip_map, &index); if (!local_ip) { @@ -494,9 +494,9 @@ SEC("tc") int ip6vxlan_set_tunnel_dst(struct __sk_buff *skb) { struct bpf_tunnel_key key; - int ret; __u32 index = 0; __u32 *local_ip; + int ret = 0; local_ip = bpf_map_lookup_elem(&local_ip_map, &index); if (!local_ip) { @@ -525,9 +525,9 @@ SEC("tc") int ip6vxlan_set_tunnel_src(struct __sk_buff *skb) { struct bpf_tunnel_key key; - int ret; __u32 index = 0; __u32 *local_ip; + int ret = 0; local_ip = bpf_map_lookup_elem(&local_ip_map, &index); if (!local_ip) { @@ -556,9 +556,9 @@ SEC("tc") int ip6vxlan_get_tunnel_src(struct __sk_buff *skb) { struct bpf_tunnel_key key; - int ret; __u32 index = 0; __u32 *local_ip; + int ret = 0; local_ip = bpf_map_lookup_elem(&local_ip_map, &index); if (!local_ip) {
Per C99 standard [0], Section 6.7.8, Paragraph 10: If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate. And in the same document, in appendix "J.2 Undefined behavior": The behavior is undefined in the following circumstances: [...] The value of an object with automatic storage duration is used while it is indeterminate (6.2.4, 6.7.8, 6.8). This means that use of an uninitialized stack variable is undefined behavior, and therefore that clang can choose to do a variety of scary things, such as not generating bytecode for "bunch of useful code" in the below example: void some_func() { int i; if (!i) return; // bunch of useful code } To add insult to injury, if some_func above is a helper function for some BPF program, clang can choose to not generate an "exit" insn, causing verifier to fail with "last insn is not an exit or jmp". Going from that verification failure to the root cause of uninitialized use is certain to be frustrating. This patch adds -Wuninitialized to the cflags for selftest BPF progs and fixes up existing instances of uninitialized use. [0]: https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> Cc: David Vernet <void@manifault.com> Cc: Tejun Heo <tj@kernel.org> --- tools/testing/selftests/bpf/Makefile | 2 +- tools/testing/selftests/bpf/progs/rbtree.c | 2 +- tools/testing/selftests/bpf/progs/rbtree_fail.c | 5 +++-- .../selftests/bpf/progs/test_kfunc_dynptr_param.c | 2 +- .../testing/selftests/bpf/progs/test_sk_lookup_kern.c | 2 +- tools/testing/selftests/bpf/progs/test_tunnel_kern.c | 10 +++++----- 6 files changed, 12 insertions(+), 11 deletions(-)