Re: [PATCH bpf-next v2 08/10] tools/libbpf: Add support for BPF_PROG_TYPE_LSM

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, Jan 16, 2020 at 4:49 AM KP Singh <kpsingh@chromium.org> wrote:
>
> Thanks for the review Andrii!
>
> I will incorporate the fixes in the next revision.
>
> On 15-Jan 13:19, Andrii Nakryiko wrote:
> > On Wed, Jan 15, 2020 at 9:13 AM KP Singh <kpsingh@chromium.org> wrote:
> > >
> > > From: KP Singh <kpsingh@google.com>
> > >
> > > * Add functionality in libbpf to attach eBPF program to LSM hooks
> > > * Lookup the index of the LSM hook in security_hook_heads and pass it in
> > >   attr->lsm_hook_index
> > >
> > > Signed-off-by: KP Singh <kpsingh@google.com>
> > > ---
> > >  tools/lib/bpf/bpf.c      |   6 +-
> > >  tools/lib/bpf/bpf.h      |   1 +
> > >  tools/lib/bpf/libbpf.c   | 143 ++++++++++++++++++++++++++++++++++-----
> > >  tools/lib/bpf/libbpf.h   |   4 ++
> > >  tools/lib/bpf/libbpf.map |   3 +
> > >  5 files changed, 138 insertions(+), 19 deletions(-)
> > >

[...]

> >
> > > +{
> > > +       struct btf *btf = bpf_find_kernel_btf();
> >
> > ok, it's probably time to do this right. Let's ensure we load kernel
> > BTF just once, keep it inside bpf_object while we need it and then
> > release it after successful load. We are at the point where all the
> > new types of program is loading/releasing kernel BTF for every section
> > and it starts to feel very wasteful.
>
> Sure, will give it a shot in v3.

thanks!

[...]

> >
> > > +               if (!strcmp(btf__name_by_offset(btf, m->name_off), name))
> > > +                       return j + 1;
> >
> > I looked briefly through kernel-side patch introducing lsm_hook_index,
> > but it didn't seem to explain why this index needs to be (unnaturally)
> > 1-based. So asking here first as I'm looking through libbpf changes?
>
> The lsm_hook_idx is one-based as it makes it easy to validate the
> input. If we make it zero-based it's hard to check if the user
> intended to attach to the LSM hook at index 0 or did not set it.

Think about providing FDs. 0 is a valid, though rarely
intended/correct value. Yet we don't make all FD arguments
artificially 1-based, right? This extra +1/-1 translation just makes
for more confusing interface, IMO. If user "accidentally" guessed type
signature of very first hook, well, so be it... If not, BPF verifier
will politely refuse. Seems like enough protection.

>
> We are then up to the verifier to reject the loaded program which
> may or may not match the signature of the hook at lsm_hook_idx = 0.
>
> I will clarify this in the commit log as well.
>

[...]