Date:   Thu, 19 May 2022 16:39:55 -0700
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Yonghong Song <yhs@...com>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org, ast@...nel.org,
        daniel@...earbox.net, andrii@...nel.org, kafai@...com,
        kpsingh@...nel.org, jakub@...udflare.com
Subject: Re: [PATCH bpf-next v7 00/11] bpf: cgroup_sock lsm flavor

On Thu, May 19, 2022 at 4:34 PM Yonghong Song <yhs@...com> wrote:
>
>
>
> On 5/18/22 3:55 PM, Stanislav Fomichev wrote:
> > This series implements new lsm flavor for attaching per-cgroup programs to
> > existing lsm hooks. The cgroup is taken out of 'current', unless
> > the first argument of the hook is 'struct socket'. In this case,
> > the cgroup association is taken out of socket. The attachment
> > looks like a regular per-cgroup attachment: we add new BPF_LSM_CGROUP
> > attach type which, together with attach_btf_id, signals per-cgroup lsm.
> > Behind the scenes, we allocate trampoline shim program and
> > attach to lsm. This program looks up cgroup from current/socket
> > and runs cgroup's effective prog array. The rest of the per-cgroup BPF
> > stays the same: hierarchy, local storage, retval conventions
> > (return 1 == success).
> >
> > Current limitations:
> > * haven't considered sleepable bpf; can be extended later on
> > * not sure the verifier does the right thing with null checks;
> >    see latest selftest for details
> > * total of 10 (global) per-cgroup LSM attach points
> >
> > Cc: ast@...nel.org
> > Cc: daniel@...earbox.net
> > Cc: kafai@...com
> > Cc: kpsingh@...nel.org
> > Cc: jakub@...udflare.com
> >
> > v7:
> > - there were a lot of comments last time, hope I didn't forget anything,
> >    some of the bigger ones:
> >    - Martin: use/extend BTF_SOCK_TYPE_SOCKET
> >    - Martin: expose bpf_set_retval
> >    - Martin: reject 'return 0' at the verifier for 'void' hooks
> >    - Martin: prog_query returns all BPF_LSM_CGROUP, prog_info
> >      returns attach_btf_func_id
> >    - Andrii: split libbpf changes
> >    - Andrii: add field access test to test_progs, not test_verifier (still
> >      using asm though)
> > - things that I haven't addressed, stating them here explicitly, let
> >    me know if some of these are still problematic:
> >    1. Andrii: exposing only link-based api: seems like the changes
> >       to support non-link-based ones are minimal, couple of lines,
> >       so seems like it worth having it?
> >    2. Alexei: applying cgroup_atype for all cgroup hooks, not only
> >       cgroup lsm: looks a bit harder to apply everywhere that I
> >       originally thought; with lsm cgroup, we have a shim_prog pointer where
> >       we store cgroup_atype; for non-lsm programs, we don't have a
> >       trace program where to store it, so we still need some kind
> >       of global table to map from "static" hook to "dynamic" slot.
> >       So I'm dropping this "can be easily extended" clause from the
> >       description for now. I have converted this whole machinery
> >       to an RCU-managed list to remove synchronize_rcu().
> > - also note that I had to introduce new bpf_shim_tramp_link and
> >    moved refcnt there; we need something to manage new bpf_tramp_link
> >
> > v6:
> > - remove active count & stats for shim program (Martin KaFai Lau)
> > - remove NULL/error check for btf_vmlinux (Martin)
> > - don't check cgroup_atype in bpf_cgroup_lsm_shim_release (Martin)
> > - use old_prog (instead of passed one) in __cgroup_bpf_detach (Martin)
> > - make sure attach_btf_id is the same in __cgroup_bpf_replace (Martin)
> > - enable cgroup local storage and test it (Martin)
> > - properly implement prog query and add bpftool & tests (Martin)
> > - prohibit non-shared cgroup storage mode for BPF_LSM_CGROUP (Martin)
> >
> > v5:
> > - __cgroup_bpf_run_lsm_socket remove NULL sock/sk checks (Martin KaFai Lau)
> > - __cgroup_bpf_run_lsm_{socket,current} s/prog/shim_prog/ (Martin)
> > - make sure bpf_lsm_find_cgroup_shim works for hooks without args (Martin)
> > - __cgroup_bpf_attach make sure attach_btf_id is the same when replacing (Martin)
> > - call bpf_cgroup_lsm_shim_release only for LSM_CGROUP (Martin)
> > - drop BPF_LSM_CGROUP from bpf_attach_type_to_tramp (Martin)
> > - drop jited check from cgroup_shim_find (Martin)
> > - new patch to convert cgroup_bpf to hlist_node (Jakub Sitnicki)
> > - new shim flavor for 'struct sock' + list of exceptions (Martin)
> >
> > v4:
> > - fix build when jit is on but syscall is off
> >
> > v3:
> > - add BPF_LSM_CGROUP to bpftool
> > - use simple int instead of refcnt_t (to avoid use-after-free
> >    false positive)
> >
> > v2:
> > - addressed build bot failures
> >
> > Stanislav Fomichev (11):
> >    bpf: add bpf_func_t and trampoline helpers
> >    bpf: convert cgroup_bpf.progs to hlist
> >    bpf: per-cgroup lsm flavor
> >    bpf: minimize number of allocated lsm slots per program
> >    bpf: implement BPF_PROG_QUERY for BPF_LSM_CGROUP
> >    bpf: allow writing to a subset of sock fields from lsm progtype
> >    libbpf: implement bpf_prog_query_opts
> >    libbpf: add lsm_cgoup_sock type
> >    bpftool: implement cgroup tree for BPF_LSM_CGROUP
> >    selftests/bpf: lsm_cgroup functional test
> >    selftests/bpf: verify lsm_cgroup struct sock access
> >
> >   arch/x86/net/bpf_jit_comp.c                   |  24 +-
> >   include/linux/bpf-cgroup-defs.h               |  11 +-
> >   include/linux/bpf-cgroup.h                    |   9 +-
> >   include/linux/bpf.h                           |  36 +-
> >   include/linux/bpf_lsm.h                       |   8 +
> >   include/linux/btf_ids.h                       |   3 +-
> >   include/uapi/linux/bpf.h                      |   6 +
> >   kernel/bpf/bpf_lsm.c                          | 103 ++++
> >   kernel/bpf/btf.c                              |  11 +
> >   kernel/bpf/cgroup.c                           | 487 +++++++++++++++---
> >   kernel/bpf/core.c                             |   2 +
> >   kernel/bpf/syscall.c                          |  14 +-
> >   kernel/bpf/trampoline.c                       | 244 ++++++++-
> >   kernel/bpf/verifier.c                         |  31 +-
> >   tools/bpf/bpftool/cgroup.c                    |  77 ++-
> >   tools/bpf/bpftool/common.c                    |   1 +
> >   tools/include/linux/btf_ids.h                 |   4 +-
> >   tools/include/uapi/linux/bpf.h                |   6 +
> >   tools/lib/bpf/bpf.c                           |  42 +-
> >   tools/lib/bpf/bpf.h                           |  15 +
> >   tools/lib/bpf/libbpf.c                        |   2 +
> >   tools/lib/bpf/libbpf.map                      |   1 +
> >   .../selftests/bpf/prog_tests/lsm_cgroup.c     | 346 +++++++++++++
> >   .../testing/selftests/bpf/progs/lsm_cgroup.c  | 160 ++++++
> >   24 files changed, 1480 insertions(+), 163 deletions(-)
> >   create mode 100644 tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> >   create mode 100644 tools/testing/selftests/bpf/progs/lsm_cgroup.c
>
> There are 4 test failures for test_progs in CI.
>
> https://github.com/kernel-patches/bpf/runs/6511113546?check_suite_focus=true
> All have error messages like:
>      At program exit the register R0 has value (0xffffffff; 0x0) should
> have been in (0x0; 0x1)
>
> Could you take a look?

Ugh, definitely, thanks for the pointer!

Powered by blists - more mailing lists