Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755935AbdL2IUf (ORCPT ); Fri, 29 Dec 2017 03:20:35 -0500 Received: from mail.kernel.org ([198.145.29.99]:44280 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755856AbdL2IU1 (ORCPT ); Fri, 29 Dec 2017 03:20:27 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5BBBB218BE Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=mhiramat@kernel.org Date: Fri, 29 Dec 2017 17:20:22 +0900 From: Masami Hiramatsu To: Alexei Starovoitov Cc: Steven Rostedt , Alexei Starovoitov , Josef Bacik , , , , , , , , , , Josef Bacik , Akinobu Mita Subject: Re: [RFC PATCH bpf-next v2 1/4] tracing/kprobe: bpf: Check error injectable event is on function entry Message-Id: <20171229172022.8b184d85d3787bc7f8d1c45a@kernel.org> In-Reply-To: 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> <20171228172027.4a8f2f0cf0506499acd26738@kernel.org> X-Mailer: Sylpheed 3.5.1 (GTK+ 2.24.31; x86_64-redhat-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3236 Lines: 77 On Thu, 28 Dec 2017 17:03:24 -0800 Alexei Starovoitov wrote: > On 12/28/17 12:20 AM, Masami Hiramatsu wrote: > > On Wed, 27 Dec 2017 20:32:07 -0800 > > Alexei Starovoitov wrote: > > > >> 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. > > > > No, trace_kprobe_ftrace() just checks the given address will be > > managed by ftrace. you can see arch_check_ftrace_location() in kernel/kprobes.c. > > > > FYI, CONFIG_KPROBES_ON_FTRACE depends on DYNAMIC_FTRACE_WITH_REGS, and > > DYNAMIC_FTRACE_WITH_REGS doesn't depend on CC_USING_FENTRY. > > This means if you compile kernel with old gcc and enable DYNAMIC_FTRACE, > > kprobes uses ftrace on mcount address which is NOT the entry point > > of target function. > > ok. fair enough. I think we can gate the feature to !mcount only. > > > On the other hand, changing IP feature has been implemented originaly > > by kprobes with int3 (sw breakpoint). This means you can use kprobes > > at correct address (the entry address of the function) you can hijack > > the function, as jprobe did. > > > >>>> 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. > > > > Right. If you need to test it, you can run Josef's test case without > > CONFIG_DYNAMIC_FTRACE. > > It should be obvious that the person who submits the patch > must run the tests. > > >> 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. > > > > OK, so would you just ask me to run samples/bpf ? > > Please run Josef's test in the !ftrace setup. Yes, I'll add the result of the test case. Thank you, -- Masami Hiramatsu