Messages in this thread | | | Subject | Re: [PATCH 1/2] bpf: add a bpf_override_function helper | From | Alexei Starovoitov <> | Date | Sun, 12 Nov 2017 14:49:22 +0800 |
| |
On 11/11/17 4:14 PM, 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.)
yeah. modifying arbitrary function return pushes bpf outside of its safety guarantees and in that sense doing the same override_return could be done from a kernel module if kernel provides the x64 side of the facility introduced by this patch. On the other side adding parts of this feature to the kernel only to be used by external kernel module is quite ugly too and not something that was ever done before. How about we restrict this bpf_override_return() only to the functions which callers expect to handle errors ? We can add something similar to NOKPROBE_SYMBOL(). Like ALLOW_RETURN_OVERRIDE() and on btrfs side mark the functions we're going to test with this feature. Then 'not crashing kernel' requirement will be preserved. btrfs or whatever else we will be testing with override_return will be functioning in 'stress test' mode and if bpf program is not careful and returns error all the time then one particular subsystem (like btrfs) will not be functional, but the kernel will not be crashing. Thoughts?
| |