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: Wed, 15 Nov 2017 08:34:56 +0100 Message-ID: <20171115073456.2dx4l2onbxn3ekzu@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> <20171112103824.433mm7caxsuhoj2g@gmail.com> <20171113155752.yhzxm4kpihg4ns65@destiny> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Alexei Starovoitov , 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: Josef Bacik Return-path: Content-Disposition: inline In-Reply-To: <20171113155752.yhzxm4kpihg4ns65@destiny> Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org * Josef Bacik wrote: > > > 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. > > I'm going to want to annotate kmalloc, so it's still going to be possible to > make things go horribly wrong, is this still going to be ok with you? Obviously > I want to use this for btrfs, but really what I used this for originally was an > NBD problem where I had to do special handling for getting EINTR back from > kernel_sendmsg, which was a pain to trigger properly without this patch. Opt-in > is going to make it so we're just flagging important function calls anwyay > because those are the ones that fail rarely and that we want to test, which puts > us back in the same situation you are worried about, so it doesn't make much > sense to me to do it this way. Thanks, I suppose - let's see how it goes? The important factor is the opt-in aspect I believe. Technically the kernel should never crash on a kmalloc() failure either, although obviously things can go horribly wrong from user-space's perspective. Thanks, Ingo