Date:   Mon, 27 Sep 2021 18:33:44 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        John Fastabend <john.fastabend@...il.com>
Cc:     Dave Marchevsky <davemarchevsky@...com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, Yonghong Song <yhs@...com>
Subject: Re: [RFC PATCH bpf-next 0/2] bpf: keep track of prog verification
 stats

Andrii Nakryiko wrote:
> On Mon, Sep 27, 2021 at 11:20 AM John Fastabend
> <john.fastabend@...il.com> wrote:
> >
> > Dave Marchevsky wrote:
> > > On 9/23/21 10:02 PM, Andrii Nakryiko wrote:
> > > > On Thu, Sep 23, 2021 at 6:27 PM Dave Marchevsky <davemarchevsky@...com> wrote:
> > > >>
> > > >> On 9/23/21 4:51 PM, Alexei Starovoitov wrote:
> > > >>> On Mon, Sep 20, 2021 at 08:11:10AM -0700, Dave Marchevsky wrote:
> > > >>>> The verifier currently logs some useful statistics in
> > > >>>> print_verification_stats. Although the text log is an effective feedback
> > > >>>> tool for an engineer iterating on a single application, it would also be
> > > >>>> useful to enable tracking these stats in a more structured form for
> > > >>>> fleetwide or historical analysis, which this patchset attempts to do.
> > > >>>>

[...] 

> > >
> > > Seems reasonable to me - and attaching a BPF program to the tracepoint to
> > > grab data is delightfully meta :)
> > >
> > > I'll do a pass on alternate implementation with _just_ tracepoint, no
> > > prog_info or fdinfo, can add minimal or full stats to those later if
> > > necessary.
> >
> > We can also use a hook point here to enforce policy on allowing the
> > BPF program to load or not using the stats here. For now basic
> > insn is a good start to allow larger/smaller programs to be loaded,
> > but we might add other info like call bitmask, features, types, etc.
> > If one of the arguments is the bpf_attr struct we can just read
> > lots of useful program info out directly.
> >
> > We would need something different from a tracepoint though to let
> > it return a reject|accept code. How about a new hook type that
> > has something similar to sockops that lets us just return an
> > accept or reject code?
> >
> > By doing this we can check loader signatures here to be sure the
> > loader is signed or otherwise has correct permissions to be loading
> > whatever type of bpf program is here.
> 
> For signing and generally preventing some BPF programs from loading
> (e.g., if there is some malicious BPF program that takes tons of
> memory to be validated), wouldn't you want to check that before BPF
> verifier spent all those resources on verification? So maybe there
> will be another hook before BPF prog is validated for that? Basically,
> if you don't trust any BPF program unless it is signed, I'd expect you
> check signature before BPF verifier does its heavy job.

Agree, for basic sig check or anything that just wants to look at
the task_struct storage for some attributes before we verify is
more efficient. The only reason I suggested after is if we wanted
to start auditing/enforcing on calls or map read/writes, etc. these
we would need the verifier to help tabulate.

When I hacked it in for experimenting I put the hook in the sys
bpf load path before the verifier runs. That seemed to work for
the simpler sig check cases I was running.

OTOH though if we have a system with lots of BPF failed loads this
would indicate a more serious problem that an admin should fix
so might be nicer code-wise to just have a single hook after verifier
vs optimizing to two one in front and one after. 

> 
> >
> > Thanks,
> > John

Powered by blists - more mailing lists