On Tue, May 10, 2022 at 10:31 AM Stanislav Fomichev [off-list ref] wrote:

On Mon, May 9, 2022 at 4:44 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Mon, May 9, 2022 at 4:38 PM Stanislav Fomichev [off-list ref] wrote:

quoted

On Mon, May 9, 2022 at 2:54 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Fri, Apr 29, 2022 at 2:16 PM Stanislav Fomichev [off-list ref] wrote:

quoted

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 <redacted>
---
 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

[...]

quoted

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.