Messages in this thread | | | Date | Fri, 29 Dec 2017 17:20:22 +0900 | From | Masami Hiramatsu <> | Subject | Re: [RFC PATCH bpf-next v2 1/4] tracing/kprobe: bpf: Check error injectable event is on function entry |
| |
On Thu, 28 Dec 2017 17:03:24 -0800 Alexei Starovoitov <ast@fb.com> wrote:
> On 12/28/17 12:20 AM, Masami Hiramatsu wrote: > > On Wed, 27 Dec 2017 20:32:07 -0800 > > Alexei Starovoitov <ast@fb.com> wrote: > > > >> On 12/27/17 8:16 PM, Steven Rostedt wrote: > >>> On Wed, 27 Dec 2017 19:45:42 -0800 > >>> Alexei Starovoitov <ast@fb.com> 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 <mhiramat@kernel.org>
| |