| Message ID | 20201117082638.43675-3-bjorn.topel@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | RISC-V selftest/bpf fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: From:/Signed-off-by: email name mismatch: 'From: "Björn Töpel" <bjorn.topel@gmail.com>' != 'Signed-off-by: Björn Töpel <bjorn.topel@gmail.com>' |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, Nov 17, 2020 at 12:29 AM Björn Töpel <bjorn.topel@gmail.com> wrote: > > Some architectures have strict alignment requirements. In that case, > the BPF verifier detects if a program has unaligned accesses and > rejects them. A user can pass BPF_F_ANY_ALIGNMENT to a program to > override this check. That, however, will only work when a privileged > user loads a program. A unprivileged user loading a program with this > flag will be rejected prior entering the verifier. I'd include this paragraph as a code comment right next to the check below. > > Hence, it does not make sense to load unprivileged programs without > strict alignment when testing the verifier. This patch avoids exactly > that. > > Signed-off-by: Björn Töpel <bjorn.topel@gmail.com> > --- > tools/testing/selftests/bpf/test_verifier.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c > index 9be395d9dc64..2075f6a98813 100644 > --- a/tools/testing/selftests/bpf/test_verifier.c > +++ b/tools/testing/selftests/bpf/test_verifier.c > @@ -1152,9 +1152,15 @@ static void get_unpriv_disabled() > > static bool test_as_unpriv(struct bpf_test *test) > { > - return !test->prog_type || > - test->prog_type == BPF_PROG_TYPE_SOCKET_FILTER || > - test->prog_type == BPF_PROG_TYPE_CGROUP_SKB; > + bool req_aligned = false; > + > +#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS > + req_aligned = test->flags & F_NEEDS_EFFICIENT_UNALIGNED_ACCESS; > +#endif > + return (!test->prog_type || > + test->prog_type == BPF_PROG_TYPE_SOCKET_FILTER || > + test->prog_type == BPF_PROG_TYPE_CGROUP_SKB) && > + !req_aligned; It's a bit convoluted. This seems a bit more straightforward: #ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS if (test->flags & F_NEEDS_EFFICIENT_UNALIGNED_ACCESS) return false; #endif /* the rest of logic untouched */ ? > } > > static int do_test(bool unpriv, unsigned int from, unsigned int to) > -- > 2.27.0 >
On Wed, 18 Nov 2020 at 02:43, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Nov 17, 2020 at 12:29 AM Björn Töpel <bjorn.topel@gmail.com> wrote: > > > > Some architectures have strict alignment requirements. In that case, > > the BPF verifier detects if a program has unaligned accesses and > > rejects them. A user can pass BPF_F_ANY_ALIGNMENT to a program to > > override this check. That, however, will only work when a privileged > > user loads a program. A unprivileged user loading a program with this > > flag will be rejected prior entering the verifier. > > I'd include this paragraph as a code comment right next to the check below. > > > > > Hence, it does not make sense to load unprivileged programs without > > strict alignment when testing the verifier. This patch avoids exactly > > that. > > > > Signed-off-by: Björn Töpel <bjorn.topel@gmail.com> > > --- > > tools/testing/selftests/bpf/test_verifier.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c > > index 9be395d9dc64..2075f6a98813 100644 > > --- a/tools/testing/selftests/bpf/test_verifier.c > > +++ b/tools/testing/selftests/bpf/test_verifier.c > > @@ -1152,9 +1152,15 @@ static void get_unpriv_disabled() > > > > static bool test_as_unpriv(struct bpf_test *test) > > { > > - return !test->prog_type || > > - test->prog_type == BPF_PROG_TYPE_SOCKET_FILTER || > > - test->prog_type == BPF_PROG_TYPE_CGROUP_SKB; > > + bool req_aligned = false; > > + > > +#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS > > + req_aligned = test->flags & F_NEEDS_EFFICIENT_UNALIGNED_ACCESS; > > +#endif > > + return (!test->prog_type || > > + test->prog_type == BPF_PROG_TYPE_SOCKET_FILTER || > > + test->prog_type == BPF_PROG_TYPE_CGROUP_SKB) && > > + !req_aligned; > > It's a bit convoluted. This seems a bit more straightforward: > > #ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS > if (test->flags & F_NEEDS_EFFICIENT_UNALIGNED_ACCESS) > return false; > #endif > /* the rest of logic untouched */ > > ? > Ugh. Yes, indeed. *blush* Björn
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c index 9be395d9dc64..2075f6a98813 100644 --- a/tools/testing/selftests/bpf/test_verifier.c +++ b/tools/testing/selftests/bpf/test_verifier.c @@ -1152,9 +1152,15 @@ static void get_unpriv_disabled() static bool test_as_unpriv(struct bpf_test *test) { - return !test->prog_type || - test->prog_type == BPF_PROG_TYPE_SOCKET_FILTER || - test->prog_type == BPF_PROG_TYPE_CGROUP_SKB; + bool req_aligned = false; + +#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS + req_aligned = test->flags & F_NEEDS_EFFICIENT_UNALIGNED_ACCESS; +#endif + return (!test->prog_type || + test->prog_type == BPF_PROG_TYPE_SOCKET_FILTER || + test->prog_type == BPF_PROG_TYPE_CGROUP_SKB) && + !req_aligned; } static int do_test(bool unpriv, unsigned int from, unsigned int to)
Some architectures have strict alignment requirements. In that case, the BPF verifier detects if a program has unaligned accesses and rejects them. A user can pass BPF_F_ANY_ALIGNMENT to a program to override this check. That, however, will only work when a privileged user loads a program. A unprivileged user loading a program with this flag will be rejected prior entering the verifier. Hence, it does not make sense to load unprivileged programs without strict alignment when testing the verifier. This patch avoids exactly that. Signed-off-by: Björn Töpel <bjorn.topel@gmail.com> --- tools/testing/selftests/bpf/test_verifier.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)