On Tue, May 10, 2022 at 10:31 AM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
>
> On Mon, May 9, 2022 at 4:44 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Mon, May 9, 2022 at 4:38 PM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
> > >
> > > On Mon, May 9, 2022 at 2:54 PM Andrii Nakryiko
> > > <andrii.nakryiko@xxxxxxxxx> wrote:
> > > >
> > > > On Fri, Apr 29, 2022 at 2:16 PM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
> > > > >
> > > > > sk_priority & sk_mark are writable, the rest is readonly.
> > > > >
> > > > > Add new ldx_offset fixups to lookup the offset of struct field.
> > > > > Allow using test.kfunc regardless of prog_type.
> > > > >
> > > > > One interesting thing here is that the verifier doesn't
> > > > > really force me to add NULL checks anywhere :-/
> > > > >
> > > > > Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> > > > > ---
> > > > > tools/testing/selftests/bpf/test_verifier.c | 54 ++++++++++++++++++-
> > > > > .../selftests/bpf/verifier/lsm_cgroup.c | 34 ++++++++++++
> > > > > 2 files changed, 87 insertions(+), 1 deletion(-)
> > > > > create mode 100644 tools/testing/selftests/bpf/verifier/lsm_cgroup.c
> > > > >
> > > >
> > > > [...]
> > > >
> > > > > diff --git a/tools/testing/selftests/bpf/verifier/lsm_cgroup.c b/tools/testing/selftests/bpf/verifier/lsm_cgroup.c
> > > > > new file mode 100644
> > > > > index 000000000000..af0efe783511
> > > > > --- /dev/null
> > > > > +++ b/tools/testing/selftests/bpf/verifier/lsm_cgroup.c
> > > > > @@ -0,0 +1,34 @@
> > > > > +#define SK_WRITABLE_FIELD(tp, field, size, res) \
> > > > > +{ \
> > > > > + .descr = field, \
> > > > > + .insns = { \
> > > > > + /* r1 = *(u64 *)(r1 + 0) */ \
> > > > > + BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, 0), \
> > > > > + /* r1 = *(u64 *)(r1 + offsetof(struct socket, sk)) */ \
> > > > > + BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, 0), \
> > > > > + /* r2 = *(u64 *)(r1 + offsetof(struct sock, <field>)) */ \
> > > > > + BPF_LDX_MEM(size, BPF_REG_2, BPF_REG_1, 0), \
> > > > > + /* *(u64 *)(r1 + offsetof(struct sock, <field>)) = r2 */ \
> > > > > + BPF_STX_MEM(size, BPF_REG_1, BPF_REG_2, 0), \
> > > > > + BPF_MOV64_IMM(BPF_REG_0, 1), \
> > > > > + BPF_EXIT_INSN(), \
> > > > > + }, \
> > > > > + .result = res, \
> > > > > + .errstr = res ? "no write support to 'struct sock' at off" : "", \
> > > > > + .prog_type = BPF_PROG_TYPE_LSM, \
> > > > > + .expected_attach_type = BPF_LSM_CGROUP, \
> > > > > + .kfunc = "socket_post_create", \
> > > > > + .fixup_ldx = { \
> > > > > + { "socket", "sk", 1 }, \
> > > > > + { tp, field, 2 }, \
> > > > > + { tp, field, 3 }, \
> > > > > + }, \
> > > > > +}
> > > > > +
> > > > > +SK_WRITABLE_FIELD("sock_common", "skc_family", BPF_H, REJECT),
> > > > > +SK_WRITABLE_FIELD("sock", "sk_sndtimeo", BPF_DW, REJECT),
> > > > > +SK_WRITABLE_FIELD("sock", "sk_priority", BPF_W, ACCEPT),
> > > > > +SK_WRITABLE_FIELD("sock", "sk_mark", BPF_W, ACCEPT),
> > > > > +SK_WRITABLE_FIELD("sock", "sk_pacing_rate", BPF_DW, REJECT),
> > > > > +
> > > >
> > > > have you tried writing it as C program and adding the test to
> > > > test_progs? Does something not work there?
> > >
> > > Seems like it should work, I don't see any issues with writing 5
> > > programs to test each field.
> > > But test_verified still feels like a better fit? Any reason in
> > > particular you'd prefer test_progs over test_verifier?
> >
> > Adding that fixup_ldx->strct special handling didn't feel like the
> > best fit, tbh. test_progs is generally much nicer to deal with in
> > terms of CI and in terms of comprehending what's going on and
> > supporting the code longer term.
>
> This is not new, right? We already have a bunch of fixup_xxx things.
I'm not saying it's wrong, but we don't have to keep adding extra
custom fixup_xxx things and having hand crafted assembly test cases if
we can do C tests, right? BPF assembly tests are sometimes necessary
if we need to craft some special conditions which are hard to
guarantee from Clang side during C to BPF assembly translation. But
this one doesn't seem to be the case.
> I can try to move this into test_progs in largely the same manner if
> you prefer, having a C file per field seems like an overkill.
You don't need a separate C file for each case. See what Joanne does
with SEC("?...") for dynptr tests, or what Kumar did for his kptr
tests. You can put multiple negative tests as separate BPF programs in
one file with auto-load disabled through SEC("?...") and then
open/load skeleton each time for each program, one at a time.
