From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexei Starovoitov Subject: Re: [RFC PATCH bpf-next v2 1/4] tracing/kprobe: bpf: Check error injectable event is on function entry Date: Wed, 27 Dec 2017 20:32:07 -0800 Message-ID: References: <151427438796.32561.4235654585430455286.stgit@devbox> <151427441954.32561.8731119329264462024.stgit@devbox> <20171227015730.jjggymg4uqllteuy@ast-mbp> <20171227145628.53f68f391b2108d6df118ca7@kernel.org> <20171228113434.eb182c348fc69853fec934ee@kernel.org> <03e0ebb7-0b2a-4235-3408-c0d59a1ba4c2@fb.com> <20171227231644.168abc0f@vmware.local.home> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Cc: Masami Hiramatsu , Alexei Starovoitov , Josef Bacik , , , , , , , , , , Josef Bacik , Akinobu Mita To: Steven Rostedt Return-path: Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:53660 "EHLO mx0b-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753327AbdL1Ecp (ORCPT ); Wed, 27 Dec 2017 23:32:45 -0500 In-Reply-To: <20171227231644.168abc0f@vmware.local.home> Sender: netdev-owner@vger.kernel.org List-ID: On 12/27/17 8:16 PM, Steven Rostedt wrote: > On Wed, 27 Dec 2017 19:45:42 -0800 > Alexei Starovoitov wrote: > >> I don't think that's the case. My reading of current >> trace_kprobe_ftrace() -> arch_check_ftrace_location() >> is that it will not be true for old mcount case. > > In the old mcount case, you can't use ftrace to return without calling > the function. That is, no modification of the return ip, unless you > created a trampoline that could handle arbitrary stack frames, and > remove them from the stack before returning back to the function. correct. I was saying that trace_kprobe_ftrace() won't let us do bpf_override_return with old mcount. >> >> As far as the rest of your arguments it very much puzzles me that >> you claim that this patch suppose to work based on historical >> reasoning whereas you did NOT test it. > > I believe that Masami is saying that the modification of the IP from > kprobes has been very well tested. But I'm guessing that you still want > a test case for using kprobes in this particular instance. It's not the > implementation of modifying the IP that you are worried about, but the > implementation of BPF using it in this case. Right? exactly. No doubt that old code works. But it doesn't mean that bpf_override_return() will continue to work in kprobes that are not ftrace based. I suspect Josef's existing test case will cover this situation. Probably only special .config is needed to disable ftrace, so "kprobe on entry but not ftrace" check will kick in. But I didn't get an impression that this situation was tested. Instead I see only logical reasoning that it's _supposed_ to work. That's not enough.