From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ingo Molnar Subject: Re: [PATCH 1/2] bpf: add a bpf_override_function helper Date: Sun, 12 Nov 2017 11:38:24 +0100 Message-ID: <20171112103824.433mm7caxsuhoj2g@gmail.com> References: <1510086523-8859-1-git-send-email-josef@toxicpanda.com> <1510086523-8859-2-git-send-email-josef@toxicpanda.com> <20171110093459.w2pvo3ntkwbmgnha@gmail.com> <20171110171428.hrw5cpxy4sgzf7mn@destiny> <20171111081455.qx4rodxldofbzypb@gmail.com> <23fd1b7a-5c7d-8b11-adc5-7e6679b6e61e@fb.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Josef Bacik , rostedt@goodmis.org, mingo@redhat.com, davem@davemloft.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, ast@kernel.org, kernel-team@fb.com, daniel@iogearbox.net, Josef Bacik To: Alexei Starovoitov Return-path: Received: from mail-wm0-f50.google.com ([74.125.82.50]:53537 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750704AbdKLKi3 (ORCPT ); Sun, 12 Nov 2017 05:38:29 -0500 Content-Disposition: inline In-Reply-To: <23fd1b7a-5c7d-8b11-adc5-7e6679b6e61e@fb.com> Sender: netdev-owner@vger.kernel.org List-ID: * Alexei Starovoitov wrote: > > 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? Yeah, that approach sounds much better to me: it should be fundamentally be opt-in, and should be documented that it should not be possible to crash the kernel via changing the return value. I'd make it a bit clearer in the naming what the purpose of the annotation is: for example would BPF_ALLOW_ERROR_INJECTION() work for you guys? I.e. I think it should generally be used to change actual integer error values - or at most user pointers, but not kernel pointers. Not enforced in a type safe manner, but the naming should give enough hints? Such return-injection BFR programs can still totally confuse user-space obviously: for example returning an IO error could corrupt application data - but that's the nature of such facilities and similar results could already be achieved via ptrace as well. But the result of a BPF program should never be _worse_ than ptrace, in terms of kernel integrity. Note that with such a safety mechanism in place no kernel message has to be generated either I suspect. In any case, my NAK would be lifted with such an approach. Thanks, Ingo