| Message ID | 20181129.193241.368862367320252782.davem@davemloft.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Improve verifier test coverage on sparc64 et al. | expand |
On Thu, Nov 29, 2018 at 07:32:41PM -0800, David Miller wrote: > > Often we want to write tests cases that check things like bad context > offset accesses. And one way to do this is to use an odd offset on, > for example, a 32-bit load. > > This unfortunately triggers the alignment checks first on platforms > that do not set CONFIG_EFFICIENT_UNALIGNED_ACCESS. So the test > case see the alignment failure rather than what it was testing for. > > It is often not completely possible to respect the original intention > of the test, or even test the same exact thing, while solving the > alignment issue. > > Another option could have been to check the alignment after the > context and other validations are performed by the verifier, but > that is a non-trivial change to the verifier. > > Signed-off-by: David S. Miller <davem@davemloft.net> > --- > include/uapi/linux/bpf.h | 10 ++++++++++ > kernel/bpf/syscall.c | 7 ++++++- > kernel/bpf/verifier.c | 2 ++ > tools/include/uapi/linux/bpf.h | 10 ++++++++++ > tools/lib/bpf/bpf.c | 12 +++++++++--- > tools/lib/bpf/bpf.h | 6 +++--- > tools/testing/selftests/bpf/test_align.c | 2 +- > tools/testing/selftests/bpf/test_verifier.c | 1 + > 8 files changed, 42 insertions(+), 8 deletions(-) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index 426b5c8..c9647ea 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -232,6 +232,16 @@ enum bpf_attach_type { > */ > #define BPF_F_STRICT_ALIGNMENT (1U << 0) > > +/* If BPF_F_ANY_ALIGNMENT is used in BPF_PROF_LOAD command, the > + * verifier will allow any alignment whatsoever. This bypasses > + * what CONFIG_EFFICIENT_UNALIGNED_ACCESS would cause it to do. I think majority of user space folks who read uapi/bpf.h have no idea what that kernel config does. Could you reword the comment here to say that this flag is only effective on architectures and like sparc and mips that don't have efficient unaligned access and ignored on x86/arm64 ? > + * It is mostly used for testing when we want to validate the > + * context and memory access aspects of the validator, but because > + * of an unaligned access the alignment check would trigger before > + * the one we are interested in. > + */ > +#define BPF_F_ANY_ALIGNMENT (1U << 1) > + > /* when bpf_ldimm64->src_reg == BPF_PSEUDO_MAP_FD, bpf_ldimm64->imm == fd */ > #define BPF_PSEUDO_MAP_FD 1 > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index cf5040f..cae65bb 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -1450,9 +1450,14 @@ static int bpf_prog_load(union bpf_attr *attr) > if (CHECK_ATTR(BPF_PROG_LOAD)) > return -EINVAL; > > - if (attr->prog_flags & ~BPF_F_STRICT_ALIGNMENT) > + if (attr->prog_flags & ~(BPF_F_STRICT_ALIGNMENT | BPF_F_ANY_ALIGNMENT)) > return -EINVAL; > > + if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && > + (attr->prog_flags & BPF_F_ANY_ALIGNMENT) && > + !capable(CAP_SYS_ADMIN)) > + return -EPERM; I guess we don't want to add: if (IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && (attr->prog_flags & BPF_F_ANY_ALIGNMENT)) return -EINVAL; so that test_verifier.c can just unconditionally pass this flag on all archs ? Kinda weird that this knob doesn't do anything on x86. But I guess it's fine. > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > index 03f9bcc..d4f76d2 100644 > --- a/tools/lib/bpf/bpf.c > +++ b/tools/lib/bpf/bpf.c > @@ -232,9 +232,11 @@ int bpf_load_program(enum bpf_prog_type type, const struct bpf_insn *insns, > > int bpf_verify_program(enum bpf_prog_type type, const struct bpf_insn *insns, > size_t insns_cnt, int strict_alignment, > - const char *license, __u32 kern_version, > - char *log_buf, size_t log_buf_sz, int log_level) > + int any_alignment, const char *license, > + __u32 kern_version, char *log_buf, size_t log_buf_sz, > + int log_level) > { > + __u32 prog_flags = 0; > union bpf_attr attr; > > bzero(&attr, sizeof(attr)); > @@ -247,7 +249,11 @@ int bpf_verify_program(enum bpf_prog_type type, const struct bpf_insn *insns, > attr.log_level = log_level; > log_buf[0] = 0; > attr.kern_version = kern_version; > - attr.prog_flags = strict_alignment ? BPF_F_STRICT_ALIGNMENT : 0; > + if (strict_alignment) > + prog_flags |= BPF_F_STRICT_ALIGNMENT; > + if (any_alignment) > + prog_flags |= BPF_F_ANY_ALIGNMENT; > + attr.prog_flags = prog_flags; instead of adding another argument may be replace 'int strict_alignment' with '__u32 prog_flags' ? and future flags won't need tweaks to bpf_verify_program() api.
From: Alexei Starovoitov <alexei.starovoitov@gmail.com> Date: Fri, 30 Nov 2018 13:58:20 -0800 > On Thu, Nov 29, 2018 at 07:32:41PM -0800, David Miller wrote: >> +/* If BPF_F_ANY_ALIGNMENT is used in BPF_PROF_LOAD command, the >> + * verifier will allow any alignment whatsoever. This bypasses >> + * what CONFIG_EFFICIENT_UNALIGNED_ACCESS would cause it to do. > > I think majority of user space folks who read uapi/bpf.h have no idea > what that kernel config does. > Could you reword the comment here to say that this flag is only > effective on architectures and like sparc and mips that don't > have efficient unaligned access and ignored on x86/arm64 ? Ok. >> + if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && >> + (attr->prog_flags & BPF_F_ANY_ALIGNMENT) && >> + !capable(CAP_SYS_ADMIN)) >> + return -EPERM; > > I guess we don't want to add: > if (IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && > (attr->prog_flags & BPF_F_ANY_ALIGNMENT)) > return -EINVAL; > > so that test_verifier.c can just unconditionally pass this flag > on all archs ? Right. >> @@ -247,7 +249,11 @@ int bpf_verify_program(enum bpf_prog_type type, const struct bpf_insn *insns, >> attr.log_level = log_level; >> log_buf[0] = 0; >> attr.kern_version = kern_version; >> - attr.prog_flags = strict_alignment ? BPF_F_STRICT_ALIGNMENT : 0; >> + if (strict_alignment) >> + prog_flags |= BPF_F_STRICT_ALIGNMENT; >> + if (any_alignment) >> + prog_flags |= BPF_F_ANY_ALIGNMENT; >> + attr.prog_flags = prog_flags; > > instead of adding another argument may be replace 'int strict_alignment' > with '__u32 prog_flags' ? > and future flags won't need tweaks to bpf_verify_program() api. Yeah the number of arguments are getting out of control, I'll do that.
From: Alexei Starovoitov <alexei.starovoitov@gmail.com> Date: Fri, 30 Nov 2018 13:58:20 -0800 > On Thu, Nov 29, 2018 at 07:32:41PM -0800, David Miller wrote: >> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h >> index 426b5c8..c9647ea 100644 >> --- a/include/uapi/linux/bpf.h >> +++ b/include/uapi/linux/bpf.h >> @@ -232,6 +232,16 @@ enum bpf_attach_type { >> */ >> #define BPF_F_STRICT_ALIGNMENT (1U << 0) >> >> +/* If BPF_F_ANY_ALIGNMENT is used in BPF_PROF_LOAD command, the >> + * verifier will allow any alignment whatsoever. This bypasses >> + * what CONFIG_EFFICIENT_UNALIGNED_ACCESS would cause it to do. > > I think majority of user space folks who read uapi/bpf.h have no idea > what that kernel config does. > Could you reword the comment here to say that this flag is only > effective on architectures and like sparc and mips that don't > have efficient unaligned access and ignored on x86/arm64 ? I just want to point out in passing that your feeback applies also to the comment above BPF_F_STRICT_ALIGNMENT, which I used as a model for my comment.
On Fri, Nov 30, 2018 at 08:44:20PM -0800, David Miller wrote: > From: Alexei Starovoitov <alexei.starovoitov@gmail.com> > Date: Fri, 30 Nov 2018 13:58:20 -0800 > > > On Thu, Nov 29, 2018 at 07:32:41PM -0800, David Miller wrote: > >> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > >> index 426b5c8..c9647ea 100644 > >> --- a/include/uapi/linux/bpf.h > >> +++ b/include/uapi/linux/bpf.h > >> @@ -232,6 +232,16 @@ enum bpf_attach_type { > >> */ > >> #define BPF_F_STRICT_ALIGNMENT (1U << 0) > >> > >> +/* If BPF_F_ANY_ALIGNMENT is used in BPF_PROF_LOAD command, the > >> + * verifier will allow any alignment whatsoever. This bypasses > >> + * what CONFIG_EFFICIENT_UNALIGNED_ACCESS would cause it to do. > > > > I think majority of user space folks who read uapi/bpf.h have no idea > > what that kernel config does. > > Could you reword the comment here to say that this flag is only > > effective on architectures and like sparc and mips that don't > > have efficient unaligned access and ignored on x86/arm64 ? > > I just want to point out in passing that your feeback applies also to > the comment above BPF_F_STRICT_ALIGNMENT, which I used as a model for > my comment. Good point. Missed that earlier. NET_IP_ALIGN is even more cryptic and it's not the same as HAVE_EFFICIENT_UNALIGNED_ACCESS. Example: s390 We need to reword it.
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 426b5c8..c9647ea 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -232,6 +232,16 @@ enum bpf_attach_type { */ #define BPF_F_STRICT_ALIGNMENT (1U << 0) +/* If BPF_F_ANY_ALIGNMENT is used in BPF_PROF_LOAD command, the + * verifier will allow any alignment whatsoever. This bypasses + * what CONFIG_EFFICIENT_UNALIGNED_ACCESS would cause it to do. + * It is mostly used for testing when we want to validate the + * context and memory access aspects of the validator, but because + * of an unaligned access the alignment check would trigger before + * the one we are interested in. + */ +#define BPF_F_ANY_ALIGNMENT (1U << 1) + /* when bpf_ldimm64->src_reg == BPF_PSEUDO_MAP_FD, bpf_ldimm64->imm == fd */ #define BPF_PSEUDO_MAP_FD 1 diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index cf5040f..cae65bb 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -1450,9 +1450,14 @@ static int bpf_prog_load(union bpf_attr *attr) if (CHECK_ATTR(BPF_PROG_LOAD)) return -EINVAL; - if (attr->prog_flags & ~BPF_F_STRICT_ALIGNMENT) + if (attr->prog_flags & ~(BPF_F_STRICT_ALIGNMENT | BPF_F_ANY_ALIGNMENT)) return -EINVAL; + if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && + (attr->prog_flags & BPF_F_ANY_ALIGNMENT) && + !capable(CAP_SYS_ADMIN)) + return -EPERM; + /* copy eBPF program license from user space */ if (strncpy_from_user(license, u64_to_user_ptr(attr->license), sizeof(license) - 1) < 0) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6dd4195..2fefeae 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -6350,6 +6350,8 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr) env->strict_alignment = !!(attr->prog_flags & BPF_F_STRICT_ALIGNMENT); if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) env->strict_alignment = true; + if (attr->prog_flags & BPF_F_ANY_ALIGNMENT) + env->strict_alignment = false; ret = replace_map_fd_with_map_ptr(env); if (ret < 0) diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 426b5c8..c9647ea 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -232,6 +232,16 @@ enum bpf_attach_type { */ #define BPF_F_STRICT_ALIGNMENT (1U << 0) +/* If BPF_F_ANY_ALIGNMENT is used in BPF_PROF_LOAD command, the + * verifier will allow any alignment whatsoever. This bypasses + * what CONFIG_EFFICIENT_UNALIGNED_ACCESS would cause it to do. + * It is mostly used for testing when we want to validate the + * context and memory access aspects of the validator, but because + * of an unaligned access the alignment check would trigger before + * the one we are interested in. + */ +#define BPF_F_ANY_ALIGNMENT (1U << 1) + /* when bpf_ldimm64->src_reg == BPF_PSEUDO_MAP_FD, bpf_ldimm64->imm == fd */ #define BPF_PSEUDO_MAP_FD 1 diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c index 03f9bcc..d4f76d2 100644 --- a/tools/lib/bpf/bpf.c +++ b/tools/lib/bpf/bpf.c @@ -232,9 +232,11 @@ int bpf_load_program(enum bpf_prog_type type, const struct bpf_insn *insns, int bpf_verify_program(enum bpf_prog_type type, const struct bpf_insn *insns, size_t insns_cnt, int strict_alignment, - const char *license, __u32 kern_version, - char *log_buf, size_t log_buf_sz, int log_level) + int any_alignment, const char *license, + __u32 kern_version, char *log_buf, size_t log_buf_sz, + int log_level) { + __u32 prog_flags = 0; union bpf_attr attr; bzero(&attr, sizeof(attr)); @@ -247,7 +249,11 @@ int bpf_verify_program(enum bpf_prog_type type, const struct bpf_insn *insns, attr.log_level = log_level; log_buf[0] = 0; attr.kern_version = kern_version; - attr.prog_flags = strict_alignment ? BPF_F_STRICT_ALIGNMENT : 0; + if (strict_alignment) + prog_flags |= BPF_F_STRICT_ALIGNMENT; + if (any_alignment) + prog_flags |= BPF_F_ANY_ALIGNMENT; + attr.prog_flags = prog_flags; return sys_bpf(BPF_PROG_LOAD, &attr, sizeof(attr)); } diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h index 26a5153..7b7d7a1 100644 --- a/tools/lib/bpf/bpf.h +++ b/tools/lib/bpf/bpf.h @@ -91,9 +91,9 @@ LIBBPF_API int bpf_load_program(enum bpf_prog_type type, LIBBPF_API int bpf_verify_program(enum bpf_prog_type type, const struct bpf_insn *insns, size_t insns_cnt, int strict_alignment, - const char *license, __u32 kern_version, - char *log_buf, size_t log_buf_sz, - int log_level); + int any_alignment, const char *license, + __u32 kern_version, char *log_buf, + size_t log_buf_sz, int log_level); LIBBPF_API int bpf_map_update_elem(int fd, const void *key, const void *value, __u64 flags); diff --git a/tools/testing/selftests/bpf/test_align.c b/tools/testing/selftests/bpf/test_align.c index 5f377ec..e7f2155 100644 --- a/tools/testing/selftests/bpf/test_align.c +++ b/tools/testing/selftests/bpf/test_align.c @@ -620,7 +620,7 @@ static int do_test_single(struct bpf_align_test *test) prog_len = probe_filter_length(prog); fd_prog = bpf_verify_program(prog_type ? : BPF_PROG_TYPE_SOCKET_FILTER, - prog, prog_len, 1, "GPL", 0, + prog, prog_len, 1, 0, "GPL", 0, bpf_vlog, sizeof(bpf_vlog), 2); if (fd_prog < 0 && test->result != REJECT) { printf("Failed to load program.\n"); diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c index 5dd4410..c3de824 100644 --- a/tools/testing/selftests/bpf/test_verifier.c +++ b/tools/testing/selftests/bpf/test_verifier.c @@ -14219,6 +14219,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv, fd_prog = bpf_verify_program(prog_type, prog, prog_len, test->flags & F_LOAD_WITH_STRICT_ALIGNMENT, + 0, "GPL", 0, bpf_vlog, sizeof(bpf_vlog), 1); expected_ret = unpriv && test->result_unpriv != UNDEF ?
Often we want to write tests cases that check things like bad context offset accesses. And one way to do this is to use an odd offset on, for example, a 32-bit load. This unfortunately triggers the alignment checks first on platforms that do not set CONFIG_EFFICIENT_UNALIGNED_ACCESS. So the test case see the alignment failure rather than what it was testing for. It is often not completely possible to respect the original intention of the test, or even test the same exact thing, while solving the alignment issue. Another option could have been to check the alignment after the context and other validations are performed by the verifier, but that is a non-trivial change to the verifier. Signed-off-by: David S. Miller <davem@davemloft.net> --- include/uapi/linux/bpf.h | 10 ++++++++++ kernel/bpf/syscall.c | 7 ++++++- kernel/bpf/verifier.c | 2 ++ tools/include/uapi/linux/bpf.h | 10 ++++++++++ tools/lib/bpf/bpf.c | 12 +++++++++--- tools/lib/bpf/bpf.h | 6 +++--- tools/testing/selftests/bpf/test_align.c | 2 +- tools/testing/selftests/bpf/test_verifier.c | 1 + 8 files changed, 42 insertions(+), 8 deletions(-)