Re: [PATCH 1/2] bpf: add a bpf_override_function helper

[Josef Bacik <josef@toxicpanda.com>]

On Sat, Nov 11, 2017 at 09:14:55AM +0100, Ingo Molnar wrote:
> 
> * Josef Bacik <josef@toxicpanda.com> wrote:
> 
> > On Fri, Nov 10, 2017 at 10:34:59AM +0100, Ingo Molnar wrote:
> > > 
> > > * Josef Bacik <josef@toxicpanda.com> wrote:
> > > 
> > > > @@ -551,6 +578,10 @@ static const struct bpf_func_proto *kprobe_prog_func_proto(enum bpf_func_id func
> > > >  		return &bpf_get_stackid_proto;
> > > >  	case BPF_FUNC_perf_event_read_value:
> > > >  		return &bpf_perf_event_read_value_proto;
> > > > +	case BPF_FUNC_override_return:
> > > > +		pr_warn_ratelimited("%s[%d] is installing a program with bpf_override_return helper that may cause unexpected behavior!",
> > > > +				    current->comm, task_pid_nr(current));
> > > > +		return &bpf_override_return_proto;
> > > 
> > > So if this new functionality is used we'll always print this into the syslog?
> > > 
> > > The warning is also a bit passive aggressive about informing the user: what 
> > > unexpected behavior can happen, what is the worst case?
> > > 
> > 
> > It's modeled after the other warnings bpf will spit out, but with this feature
> > you are skipping a function and instead returning some arbitrary value, so
> > anything could go wrong if you mess something up.  For instance I screwed up my
> > initial test case and made every IO submitted return an error instead of just on
> > the one file system I was attempting to test, so all sorts of hilarity ensued.
> 
> Ok, then for the x86 bits:
> 
>   NAK-ed-by: Ingo Molnar <mingo@kernel.org>
> 
> One of the major advantages of having an in-kernel BPF sandbox is to never crash 
> the kernel - and allowing BPF programs to just randomly modify the return value of 
> kernel functions sounds immensely broken to me.
> 
> (And yes, I realize that kprobes are used here as a vehicle, but the point 
> remains.)
>

Only root can use this feature, and did you read the first email?  The whole
point of this is that error path checkig fucking sucks, and this gives us the
ability to systematically check our error paths and make the kernel way more
robust than it currently is.  Can things go wrong?  Sure, that's why its a
config option and root only.  You only want to turn this on for testing and not
have it on in production.  This is a valuable tool and well worth the risk.
Thanks,

Josef