Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753150AbdL1IUd (ORCPT ); Thu, 28 Dec 2017 03:20:33 -0500 Received: from mail.kernel.org ([198.145.29.99]:47426 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751457AbdL1IUb (ORCPT ); Thu, 28 Dec 2017 03:20:31 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E159A21707 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: Thu, 28 Dec 2017 17:20:27 +0900 From: Masami Hiramatsu To: Alexei Starovoitov Cc: Steven Rostedt , Masami Hiramatsu , 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: <20171228172027.4a8f2f0cf0506499acd26738@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> X-Mailer: Sylpheed 3.5.0 (GTK+ 2.24.30; x86_64-pc-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: 2691 Lines: 63 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. 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. > 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 ? Thanks, -- Masami Hiramatsu