Message-ID: <3a732a8d-6e4f-0154-e317-795baa64022d@fb.com>
Date:   Thu, 19 May 2022 16:34:22 -0700
From:   Yonghong Song <yhs@...com>
To:     Stanislav Fomichev <sdf@...gle.com>, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Cc:     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 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?

Powered by blists - more mailing lists