| Message ID | 20200630171241.2523875-1-yhs@fb.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: fix an incorrect branch elimination by verifier | expand |
Yonghong Song wrote: > Add two tests for PTR_TO_BTF_ID vs. null ptr comparison, > one for PTR_TO_BTF_ID in the ctx structure and the > other for PTR_TO_BTF_ID after one level pointer chasing. > In both cases, the test ensures condition is not > removed. > > For example, for this test > struct bpf_fentry_test_t { > struct bpf_fentry_test_t *a; > }; > int BPF_PROG(test7, struct bpf_fentry_test_t *arg) > { > if (arg == 0) > test7_result = 1; > return 0; > } > Before the previous verifier change, we have xlated codes: > int test7(long long unsigned int * ctx): > ; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) > 0: (79) r1 = *(u64 *)(r1 +0) > ; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) > 1: (b4) w0 = 0 > 2: (95) exit > After the previous verifier change, we have: > int test7(long long unsigned int * ctx): > ; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) > 0: (79) r1 = *(u64 *)(r1 +0) > ; if (arg == 0) > 1: (55) if r1 != 0x0 goto pc+4 > ; test7_result = 1; > 2: (18) r1 = map[id:6][0]+48 > 4: (b7) r2 = 1 > 5: (7b) *(u64 *)(r1 +0) = r2 > ; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) > 6: (b4) w0 = 0 > 7: (95) exit > > Cc: Andrii Nakryiko <andriin@fb.com> > Cc: John Fastabend <john.fastabend@gmail.com> > Cc: Wenbo Zhang <ethercflow@gmail.com> > Signed-off-by: Yonghong Song <yhs@fb.com> > --- Thanks. Acked-by: John Fastabend <john.fastabend@gmail.com>
On Tue, Jun 30, 2020 at 11:46 AM Yonghong Song <yhs@fb.com> wrote: > > Add two tests for PTR_TO_BTF_ID vs. null ptr comparison, > one for PTR_TO_BTF_ID in the ctx structure and the > other for PTR_TO_BTF_ID after one level pointer chasing. > In both cases, the test ensures condition is not > removed. > > For example, for this test > struct bpf_fentry_test_t { > struct bpf_fentry_test_t *a; > }; > int BPF_PROG(test7, struct bpf_fentry_test_t *arg) > { > if (arg == 0) > test7_result = 1; > return 0; > } > Before the previous verifier change, we have xlated codes: > int test7(long long unsigned int * ctx): > ; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) > 0: (79) r1 = *(u64 *)(r1 +0) > ; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) > 1: (b4) w0 = 0 > 2: (95) exit > After the previous verifier change, we have: > int test7(long long unsigned int * ctx): > ; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) > 0: (79) r1 = *(u64 *)(r1 +0) > ; if (arg == 0) > 1: (55) if r1 != 0x0 goto pc+4 > ; test7_result = 1; > 2: (18) r1 = map[id:6][0]+48 > 4: (b7) r2 = 1 > 5: (7b) *(u64 *)(r1 +0) = r2 > ; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) > 6: (b4) w0 = 0 > 7: (95) exit > > Cc: Andrii Nakryiko <andriin@fb.com> > Cc: John Fastabend <john.fastabend@gmail.com> > Cc: Wenbo Zhang <ethercflow@gmail.com> > Signed-off-by: Yonghong Song <yhs@fb.com> > --- LGTM, two nits below. Acked-by: Andrii Nakryiko <andriin@fb.com> > net/bpf/test_run.c | 19 +++++++++++++++- > .../selftests/bpf/prog_tests/fentry_fexit.c | 2 +- > .../testing/selftests/bpf/progs/fentry_test.c | 22 +++++++++++++++++++ > .../testing/selftests/bpf/progs/fexit_test.c | 22 +++++++++++++++++++ > 4 files changed, 63 insertions(+), 2 deletions(-) > [...] > diff --git a/tools/testing/selftests/bpf/progs/fentry_test.c b/tools/testing/selftests/bpf/progs/fentry_test.c > index 9365b686f84b..5f645fdaba6f 100644 > --- a/tools/testing/selftests/bpf/progs/fentry_test.c > +++ b/tools/testing/selftests/bpf/progs/fentry_test.c > @@ -55,3 +55,25 @@ int BPF_PROG(test6, __u64 a, void *b, short c, int d, void * e, __u64 f) > e == (void *)20 && f == 21; > return 0; > } > + > +struct bpf_fentry_test_t { > + struct bpf_fentry_test_t *a; > +}; nit: __attribute__((preserve_access_index)) ? > + > +__u64 test7_result = 0; > +SEC("fentry/bpf_fentry_test7") > +int BPF_PROG(test7, struct bpf_fentry_test_t *arg) > +{ > + if (arg == 0) > + test7_result = 1; > + return 0; > +} > + > +__u64 test8_result = 0; > +SEC("fentry/bpf_fentry_test8") > +int BPF_PROG(test8, struct bpf_fentry_test_t *arg) > +{ > + if (arg->a == 0) > + test8_result = 1; > + return 0; > +} > diff --git a/tools/testing/selftests/bpf/progs/fexit_test.c b/tools/testing/selftests/bpf/progs/fexit_test.c > index bd1e17d8024c..0952affb22a6 100644 > --- a/tools/testing/selftests/bpf/progs/fexit_test.c > +++ b/tools/testing/selftests/bpf/progs/fexit_test.c > @@ -56,3 +56,25 @@ int BPF_PROG(test6, __u64 a, void *b, short c, int d, void *e, __u64 f, int ret) > e == (void *)20 && f == 21 && ret == 111; > return 0; > } > + > +struct bpf_fentry_test_t { > + struct bpf_fentry_test *a; > +}; same nit > + > +__u64 test7_result = 0; > +SEC("fexit/bpf_fentry_test7") > +int BPF_PROG(test7, struct bpf_fentry_test_t *arg) > +{ > + if (arg == 0) > + test7_result = 1; > + return 0; > +} > + > +__u64 test8_result = 0; > +SEC("fexit/bpf_fentry_test8") > +int BPF_PROG(test8, struct bpf_fentry_test_t *arg) > +{ > + if (arg->a == 0) > + test8_result = 1; > + return 0; > +} > -- > 2.24.1 >
On 6/30/20 12:23 PM, Andrii Nakryiko wrote: > On Tue, Jun 30, 2020 at 11:46 AM Yonghong Song <yhs@fb.com> wrote: >> >> Add two tests for PTR_TO_BTF_ID vs. null ptr comparison, >> one for PTR_TO_BTF_ID in the ctx structure and the >> other for PTR_TO_BTF_ID after one level pointer chasing. >> In both cases, the test ensures condition is not >> removed. >> >> For example, for this test >> struct bpf_fentry_test_t { >> struct bpf_fentry_test_t *a; >> }; >> int BPF_PROG(test7, struct bpf_fentry_test_t *arg) >> { >> if (arg == 0) >> test7_result = 1; >> return 0; >> } >> Before the previous verifier change, we have xlated codes: >> int test7(long long unsigned int * ctx): >> ; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) >> 0: (79) r1 = *(u64 *)(r1 +0) >> ; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) >> 1: (b4) w0 = 0 >> 2: (95) exit >> After the previous verifier change, we have: >> int test7(long long unsigned int * ctx): >> ; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) >> 0: (79) r1 = *(u64 *)(r1 +0) >> ; if (arg == 0) >> 1: (55) if r1 != 0x0 goto pc+4 >> ; test7_result = 1; >> 2: (18) r1 = map[id:6][0]+48 >> 4: (b7) r2 = 1 >> 5: (7b) *(u64 *)(r1 +0) = r2 >> ; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) >> 6: (b4) w0 = 0 >> 7: (95) exit >> >> Cc: Andrii Nakryiko <andriin@fb.com> >> Cc: John Fastabend <john.fastabend@gmail.com> >> Cc: Wenbo Zhang <ethercflow@gmail.com> >> Signed-off-by: Yonghong Song <yhs@fb.com> >> --- > > LGTM, two nits below. > > Acked-by: Andrii Nakryiko <andriin@fb.com> > >> net/bpf/test_run.c | 19 +++++++++++++++- >> .../selftests/bpf/prog_tests/fentry_fexit.c | 2 +- >> .../testing/selftests/bpf/progs/fentry_test.c | 22 +++++++++++++++++++ >> .../testing/selftests/bpf/progs/fexit_test.c | 22 +++++++++++++++++++ >> 4 files changed, 63 insertions(+), 2 deletions(-) >> > > [...] > >> diff --git a/tools/testing/selftests/bpf/progs/fentry_test.c b/tools/testing/selftests/bpf/progs/fentry_test.c >> index 9365b686f84b..5f645fdaba6f 100644 >> --- a/tools/testing/selftests/bpf/progs/fentry_test.c >> +++ b/tools/testing/selftests/bpf/progs/fentry_test.c >> @@ -55,3 +55,25 @@ int BPF_PROG(test6, __u64 a, void *b, short c, int d, void * e, __u64 f) >> e == (void *)20 && f == 21; >> return 0; >> } >> + >> +struct bpf_fentry_test_t { >> + struct bpf_fentry_test_t *a; >> +}; > > nit: __attribute__((preserve_access_index)) ? Yes. Why not. Will send a followup once the patch circulates back to bpf-next. > >> + >> +__u64 test7_result = 0; >> +SEC("fentry/bpf_fentry_test7") >> +int BPF_PROG(test7, struct bpf_fentry_test_t *arg) >> +{ >> + if (arg == 0) >> + test7_result = 1; >> + return 0; >> +} >> + [...]
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index bfd4ccd80847..b03c469cd01f 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -147,6 +147,20 @@ int noinline bpf_fentry_test6(u64 a, void *b, short c, int d, void *e, u64 f) return a + (long)b + c + d + (long)e + f; } +struct bpf_fentry_test_t { + struct bpf_fentry_test_t *a; +}; + +int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg) +{ + return (long)arg; +} + +int noinline bpf_fentry_test8(struct bpf_fentry_test_t *arg) +{ + return (long)arg->a; +} + int noinline bpf_modify_return_test(int a, int *b) { *b += 1; @@ -185,6 +199,7 @@ int bpf_prog_test_run_tracing(struct bpf_prog *prog, const union bpf_attr *kattr, union bpf_attr __user *uattr) { + struct bpf_fentry_test_t arg = {}; u16 side_effect = 0, ret = 0; int b = 2, err = -EFAULT; u32 retval = 0; @@ -197,7 +212,9 @@ int bpf_prog_test_run_tracing(struct bpf_prog *prog, bpf_fentry_test3(4, 5, 6) != 15 || bpf_fentry_test4((void *)7, 8, 9, 10) != 34 || bpf_fentry_test5(11, (void *)12, 13, 14, 15) != 65 || - bpf_fentry_test6(16, (void *)17, 18, 19, (void *)20, 21) != 111) + bpf_fentry_test6(16, (void *)17, 18, 19, (void *)20, 21) != 111 || + bpf_fentry_test7((struct bpf_fentry_test_t *)0) != 0 || + bpf_fentry_test8(&arg) != 0) goto out; break; case BPF_MODIFY_RETURN: diff --git a/tools/testing/selftests/bpf/prog_tests/fentry_fexit.c b/tools/testing/selftests/bpf/prog_tests/fentry_fexit.c index 83493bd5745c..109d0345a2be 100644 --- a/tools/testing/selftests/bpf/prog_tests/fentry_fexit.c +++ b/tools/testing/selftests/bpf/prog_tests/fentry_fexit.c @@ -36,7 +36,7 @@ void test_fentry_fexit(void) fentry_res = (__u64 *)fentry_skel->bss; fexit_res = (__u64 *)fexit_skel->bss; printf("%lld\n", fentry_skel->bss->test1_result); - for (i = 0; i < 6; i++) { + for (i = 0; i < 8; i++) { CHECK(fentry_res[i] != 1, "result", "fentry_test%d failed err %lld\n", i + 1, fentry_res[i]); CHECK(fexit_res[i] != 1, "result", diff --git a/tools/testing/selftests/bpf/progs/fentry_test.c b/tools/testing/selftests/bpf/progs/fentry_test.c index 9365b686f84b..5f645fdaba6f 100644 --- a/tools/testing/selftests/bpf/progs/fentry_test.c +++ b/tools/testing/selftests/bpf/progs/fentry_test.c @@ -55,3 +55,25 @@ int BPF_PROG(test6, __u64 a, void *b, short c, int d, void * e, __u64 f) e == (void *)20 && f == 21; return 0; } + +struct bpf_fentry_test_t { + struct bpf_fentry_test_t *a; +}; + +__u64 test7_result = 0; +SEC("fentry/bpf_fentry_test7") +int BPF_PROG(test7, struct bpf_fentry_test_t *arg) +{ + if (arg == 0) + test7_result = 1; + return 0; +} + +__u64 test8_result = 0; +SEC("fentry/bpf_fentry_test8") +int BPF_PROG(test8, struct bpf_fentry_test_t *arg) +{ + if (arg->a == 0) + test8_result = 1; + return 0; +} diff --git a/tools/testing/selftests/bpf/progs/fexit_test.c b/tools/testing/selftests/bpf/progs/fexit_test.c index bd1e17d8024c..0952affb22a6 100644 --- a/tools/testing/selftests/bpf/progs/fexit_test.c +++ b/tools/testing/selftests/bpf/progs/fexit_test.c @@ -56,3 +56,25 @@ int BPF_PROG(test6, __u64 a, void *b, short c, int d, void *e, __u64 f, int ret) e == (void *)20 && f == 21 && ret == 111; return 0; } + +struct bpf_fentry_test_t { + struct bpf_fentry_test *a; +}; + +__u64 test7_result = 0; +SEC("fexit/bpf_fentry_test7") +int BPF_PROG(test7, struct bpf_fentry_test_t *arg) +{ + if (arg == 0) + test7_result = 1; + return 0; +} + +__u64 test8_result = 0; +SEC("fexit/bpf_fentry_test8") +int BPF_PROG(test8, struct bpf_fentry_test_t *arg) +{ + if (arg->a == 0) + test8_result = 1; + return 0; +}
Add two tests for PTR_TO_BTF_ID vs. null ptr comparison, one for PTR_TO_BTF_ID in the ctx structure and the other for PTR_TO_BTF_ID after one level pointer chasing. In both cases, the test ensures condition is not removed. For example, for this test struct bpf_fentry_test_t { struct bpf_fentry_test_t *a; }; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) { if (arg == 0) test7_result = 1; return 0; } Before the previous verifier change, we have xlated codes: int test7(long long unsigned int * ctx): ; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) 0: (79) r1 = *(u64 *)(r1 +0) ; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) 1: (b4) w0 = 0 2: (95) exit After the previous verifier change, we have: int test7(long long unsigned int * ctx): ; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) 0: (79) r1 = *(u64 *)(r1 +0) ; if (arg == 0) 1: (55) if r1 != 0x0 goto pc+4 ; test7_result = 1; 2: (18) r1 = map[id:6][0]+48 4: (b7) r2 = 1 5: (7b) *(u64 *)(r1 +0) = r2 ; int BPF_PROG(test7, struct bpf_fentry_test_t *arg) 6: (b4) w0 = 0 7: (95) exit Cc: Andrii Nakryiko <andriin@fb.com> Cc: John Fastabend <john.fastabend@gmail.com> Cc: Wenbo Zhang <ethercflow@gmail.com> Signed-off-by: Yonghong Song <yhs@fb.com> --- net/bpf/test_run.c | 19 +++++++++++++++- .../selftests/bpf/prog_tests/fentry_fexit.c | 2 +- .../testing/selftests/bpf/progs/fentry_test.c | 22 +++++++++++++++++++ .../testing/selftests/bpf/progs/fexit_test.c | 22 +++++++++++++++++++ 4 files changed, 63 insertions(+), 2 deletions(-)