Re: [PATCH bpf-next v9 03/10] bpf: per-cgroup lsm flavor

[Martin KaFai Lau <kafai@fb.com>]

On Fri, Jun 17, 2022 at 11:28:15AM -0700, Stanislav Fomichev wrote:
> On Thu, Jun 16, 2022 at 3:25 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > On Fri, Jun 10, 2022 at 09:57:56AM -0700, Stanislav Fomichev wrote:
> > > Allow attaching to lsm hooks in the cgroup context.
> > >
> > > Attaching to per-cgroup LSM works exactly like attaching
> > > to other per-cgroup hooks. New BPF_LSM_CGROUP is added
> > > to trigger new mode; the actual lsm hook we attach to is
> > > signaled via existing attach_btf_id.
> > >
> > > For the hooks that have 'struct socket' or 'struct sock' as its first
> > > argument, we use the cgroup associated with that socket. For the rest,
> > > we use 'current' cgroup (this is all on default hierarchy == v2 only).
> > > Note that for some hooks that work on 'struct sock' we still
> > > take the cgroup from 'current' because some of them work on the socket
> > > that hasn't been properly initialized yet.
> > >
> > > Behind the scenes, we allocate a shim program that is attached
> > > to the trampoline and runs cgroup effective BPF programs array.
> > > This shim has some rudimentary ref counting and can be shared
> > > between several programs attaching to the same per-cgroup lsm hook.
> > nit. The 'per-cgroup' may be read as each cgroup has its own shim.
> > It will be useful to rephrase it a little.
> 
> I'll put the following, LMK if still not clear.
> 
> This shim has some rudimentary ref counting and can be shared
> between several programs attaching to the same lsm hook from
> different cgroups.
SG.

> > > @@ -839,8 +953,11 @@ static int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog,
> > >       if (hlist_empty(progs))
> > >               /* last program was detached, reset flags to zero */
> > >               cgrp->bpf.flags[atype] = 0;
> > > -     if (old_prog)
> > > +     if (old_prog) {
> > > +             if (type == BPF_LSM_CGROUP)
> > > +                     bpf_trampoline_unlink_cgroup_shim(old_prog);
> > I think the same bpf_trampoline_unlink_cgroup_shim() needs to be done
> > in bpf_cgroup_link_release()?  It should be done just after
> > WARN_ON(__cgroup_bpf_detach()).
> 
> Oooh, I missed that, I thought that old_prog would have the pointer to
> the old program even if it's a link :-(
> 
> Do you mind if I handle it in __cgroup_bpf_detach as well? Or do you
> think it's cleaner to do in bpf_cgroup_link_release?
> 
> if (old_prog) {
>   ...
> } else if (link) {
>   if (type == BPF_LSM_CGROUP)
>     bpf_trampoline_unlink_cgroup_shim(link->link.prog);
> }
I think this will be similar to the earlier revision.

I was thinking doing it in bpf_cgroup_link_release() for link
where I know the bpf_prog_put() will be handled by bpf_link_free()
as other link's release/detach functions do.  Otherwise, the first
reading impression is why there is no bpf_prog_put() in
__cgroup_bpf_detach() for the link case.

No strong opinion here.  Either way is fine.  Mostly personal impression
when reading the code in the first pass.  Reading it closely should be
able to figure out in either way.

> 
> > [ ... ]
> >
> > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > > index aeb31137b2ed..a237be4f8bb3 100644
> > > --- a/kernel/bpf/syscall.c
> > > +++ b/kernel/bpf/syscall.c
> > > @@ -3416,6 +3416,8 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type)
> > >               return BPF_PROG_TYPE_SK_LOOKUP;
> > >       case BPF_XDP:
> > >               return BPF_PROG_TYPE_XDP;
> > > +     case BPF_LSM_CGROUP:
> > > +             return BPF_PROG_TYPE_LSM;
> > >       default:
> > >               return BPF_PROG_TYPE_UNSPEC;
> > >       }
> > > @@ -3469,6 +3471,11 @@ static int bpf_prog_attach(const union bpf_attr *attr)
> > >       case BPF_PROG_TYPE_CGROUP_SOCKOPT:
> > >       case BPF_PROG_TYPE_CGROUP_SYSCTL:
> > >       case BPF_PROG_TYPE_SOCK_OPS:
> > > +     case BPF_PROG_TYPE_LSM:
> > > +             if (ptype == BPF_PROG_TYPE_LSM &&
> > > +                 prog->expected_attach_type != BPF_LSM_CGROUP)
> > Check this in bpf_prog_attach_check_attach_type() where
> > other expected_attach_type are enforced.
> 
> It was there initially but I moved it here because
> bpf_prog_attach_check_attach_type() is called from link_create() as
> well where the range of acceptable expected_attach_type(s) is larger.
Ah. ic.  Thanks for the explanation.

> > > diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
> > > index 57890b357f85..2345b502b439 100644
> > > --- a/tools/include/linux/btf_ids.h
> > > +++ b/tools/include/linux/btf_ids.h
> > > @@ -172,7 +172,9 @@ extern struct btf_id_set name;
> > >       BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)          \
> > >       BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP6, tcp6_sock)                    \
> > >       BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP, udp_sock)                      \
> > > -     BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP6, udp6_sock)
> > > +     BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP6, udp6_sock)                    \
> > > +     BTF_SOCK_TYPE(BTF_SOCK_TYPE_UNIX, unix_sock)                    \
> > The existing tools's btf_ids.h looks more outdated from
> > the kernel's btf_ids.h.  unix_sock is missing which is added back here.
> > mptcp_sock is missing also but not added.  I assume the latter test
> > needs unix_sock here ?
> 
> I haven't added mptcp_sock because I was added recently.
> 
> I don't think we really need to do the changes to tools/btf_ids.h, but
> it still might be worth trying to keep them in sync?
Yeah.  Other than this list, it seems other parts of this file is out of sync
also.  May be just do an individual patch in this series to do a
copy-and-paste update of the whole file.