Andrii Nakryiko wrote: > On Mon, Sep 27, 2021 at 11:20 AM John Fastabend > <john.fastabend@xxxxxxxxx> 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@xxxxxx> 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
