On Thu, Feb 17, 2022 at 8:21 AM <sdf@xxxxxxxxxx> wrote: > > As far as api the attach should probably be to a cgroup+lsm_hook pair. > > link_create.target_fd will be cgroup_fd. > > At prog load time attach_btf_id should probably be one > > of existing bpf_lsm_* hooks. > > Feels wrong to duplicate the whole set into lsm_cgroup_sock set. > > lsm_cgroup_sock is there to further limit which particular lsm > hooks BPF_LSM_CGROUP_SOCK can use. I guess I can maybe look at > BTF's first argument to verify that it's 'struct socket'? Let > me try to see whether it's a good alternative.. That's a great idea. We probably would need 2 flavors of __cgroup_bpf_run_lsm_sock wrapper. One for 'struct socket *' and another for 'struct sock *'. In lsm hooks they come as the first argument and BTF will tell us what it is. There are exceptions like socket_create hook which would have to use current->cgroup. So ideally we can have a single attach_type BPF_LSM_CGROUP and use proper wrapper socket/sock/current depending on BTF of the lsm hook. > > Would it be fast enough? > > We went through a bunch of optimization for normal cgroup and ended with: > > if (cgroup_bpf_enabled(CGROUP_INET_INGRESS) && > > cgroup_bpf_sock_enabled(sk, CGROUP_INET_INGRESS)) > > Here the trampoline code plus call into __cgroup_bpf_run_lsm_sock > > will be there for all cgroups. > > Since cgroup specific check will be inside BPF_PROG_RUN_ARRAY_CG. > > I suspect it's ok, since the link_create will be for few specific lsm > > hooks > > which are typically not in the fast path. > > Unlike traditional cgroup hook like ingress that is hot. > > Right, cgroup_bpf_enabled() is not needed because lsm is by definition > off/unattached by default. Seems like we can add cgroup_bpf_sock_enabled() > to > __cgroup_bpf_run_lsm_sock. I guess we can, but that feels redundant. If we attach the wrapper to a particular lsm hook then cgroup_bpf_sock is surely enabled. > > > For BPF_LSM_CGROUP_TASK it will take cgroup from current instead of sock, > > right? > > Right. Seems like the only difference is where we get the cgroup pointer > from: current vs sock->cgroup. Although, I'm a bit unsure whether to > allow hooks that are clearly sock-cgroup-based to use > BPF_LSM_CGROUP_TASK. For example, should we allow > BPF_LSM_CGROUP_TASK to attach to that socket_post_create? I'd prohibit that > at > least initially to avoid some subtle 'why sometimes my > programs trigger on the wrong cgroup' types of issues. Agree. With a single BPF_LSM_CGROUP attach type will get correct behavior automatically. Looking forward to non rfc patches. Exciting feature!
