Re: [PATCH bpf-next v5 0/5] Separate error injection table from kprobes
On Sat, Jan 13, 2018 at 02:53:33AM +0900, Masami Hiramatsu wrote: > Hi, > > Here are the 5th version of patches to moving error injection > table from kprobes. This version fixes a bug and update > fail-function to support multiple function error injection. > > Here is the previous version: > > https://patchwork.ozlabs.org/cover/858663/ > > Changes in v5: > - [3/5] Fix a bug that within_error_injection returns false always. > - [5/5] Update to support multiple function error injection. > > Thank you, Applied to bpf-next, Thank you Masami. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH bpf-next v2 1/4] tracing/kprobe: bpf: Check error injectable event is on function entry
On 12/29/17 12:20 AM, Masami Hiramatsu wrote: Please run Josef's test in the !ftrace setup. Yes, I'll add the result of the test case. if Josef's test is passing in !ftrace config, please resend your patches. I think 2 and 3 were nice simplifications. and patch 1 is good too if it's passes the test. Would be great to get them in for this merge window. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH bpf-next v2 4/4] error-injection: Support fault injection framework
On 12/27/17 11:51 PM, Masami Hiramatsu wrote:
Then what happen if the user set invalid retval to those functions?
even if we limit the injectable functions, it can cause a problem,
for example,
obj = func_return_object();
if (!obj) {
handling_error...;
}
obj->field = x;
In this case, obviously func_return_object() must return NULL if there is
an error, not -ENOMEM. But without the correct retval information, how would
you check the BPF code doesn't cause a trouble?
Currently it seems you are expecting only the functions which return error code.
ret = func_return_state();
if (ret < 0) {
handling_error...;
}
But how we can distinguish those?
If we have the error range for each function, we can ensure what is
*correct* error code, NULL or errno, or any other error numbers. :)
messing up return values may cause problems and range check is
not going to magically help.
The caller may handle only a certain set of errors or interpret
some of them like EBUSY as a signal to retry.
It's plain impossible to make sure that kernel will be functional
after error injection has been made.
Like kmalloc() unconditionally returning NULL will be deadly
for the kernel, hence this patch 4/4 has very limited practical
use. The bpf program need to make intelligent decisions when
to return an error and what kind of error to return.
Doing blank range check adds a false sense of additional safety.
More so it wastes kilobytes of memory to do this check, hence nack.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH bpf-next v2 1/4] tracing/kprobe: bpf: Check error injectable event is on function entry
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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH bpf-next v2 1/4] tracing/kprobe: bpf: Check error injectable event is on function entry
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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH bpf-next v2 4/4] error-injection: Support fault injection framework
On 12/27/17 5:38 PM, Masami Hiramatsu wrote: On Wed, 27 Dec 2017 14:49:46 -0800 Alexei Starovoitov wrote: On 12/27/17 12:09 AM, Masami Hiramatsu wrote: On Tue, 26 Dec 2017 18:12:56 -0800 Alexei Starovoitov wrote: On Tue, Dec 26, 2017 at 04:48:25PM +0900, Masami Hiramatsu wrote: Support in-kernel fault-injection framework via debugfs. This allows you to inject a conditional error to specified function using debugfs interfaces. Signed-off-by: Masami Hiramatsu --- Documentation/fault-injection/fault-injection.txt |5 + kernel/Makefile |1 kernel/fail_function.c| 169 + lib/Kconfig.debug | 10 + 4 files changed, 185 insertions(+) create mode 100644 kernel/fail_function.c diff --git a/Documentation/fault-injection/fault-injection.txt b/Documentation/fault-injection/fault-injection.txt index 918972babcd8..6243a588dd71 100644 --- a/Documentation/fault-injection/fault-injection.txt +++ b/Documentation/fault-injection/fault-injection.txt @@ -30,6 +30,11 @@ o fail_mmc_request injects MMC data errors on devices permitted by setting debugfs entries under /sys/kernel/debug/mmc0/fail_mmc_request +o fail_function + + injects error return on specific functions by setting debugfs entries + under /sys/kernel/debug/fail_function. No boot option supported. I like it. Could you document it a bit better? Yes, I will do in next series. In particular retval is configurable, but without an example no one will be able to figure out how to use it. Ah, right. BTW, as I pointed in the covermail, should we store the expected error value range into the injectable list? e.g. ALLOW_ERROR_INJECTION(open_ctree, -1, -MAX_ERRNO) And provide APIs to check/get it. I'm afraid such check would be too costly. Right now we have only two functions marked but I expect hundreds more will be added in the near future as soon as developers realize the potential of such error injection. All of ALLOW_ERROR_INJECTION marks add 8 byte overhead each to .data. Multiple by 1k and we have 8k of data spent on marks. If we add max/min range marks that doubles it for very little use. I think marking function only is enough. Sorry, I don't think so. Even if it takes 16 bytes more for each points, I don't think it is any overhead for machines in these days. Even if so, we can provide a kconfig to reduce it. I mean, we are living in GB-order memory are, and it will be bigger in the future. Why we have to worry about hundreds of 16bytes memory pieces? It will take a few KB, and even if we mark thousands of functions, it never reaches 1MB, in GB memory pool. :) Of course, for many small-footprint embedded devices (like having less than 128MB memory), this feature can be a overhead. But they can cut off the table by kconfig. I still disagree on wasting 16-byte * num_of_funcs of .data here. The trade-off of usability vs memory just not worth it. Sorry. Please focus on testing your changes instead. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH bpf-next v2 1/4] tracing/kprobe: bpf: Check error injectable event is on function entry
On 12/27/17 6:34 PM, Masami Hiramatsu wrote:
On Wed, 27 Dec 2017 14:46:24 -0800
Alexei Starovoitov wrote:
On 12/26/17 9:56 PM, Masami Hiramatsu wrote:
On Tue, 26 Dec 2017 17:57:32 -0800
Alexei Starovoitov wrote:
On Tue, Dec 26, 2017 at 04:46:59PM +0900, Masami Hiramatsu wrote:
Check whether error injectable event is on function entry or not.
Currently it checks the event is ftrace-based kprobes or not,
but that is wrong. It should check if the event is on the entry
of target function. Since error injection will override a function
to just return with modified return value, that operation must
be done before the target function starts making stackframe.
As a side effect, bpf error injection is no need to depend on
function-tracer. It can work with sw-breakpoint based kprobe
events too.
Signed-off-by: Masami Hiramatsu
---
kernel/trace/Kconfig|2 --
kernel/trace/bpf_trace.c|6 +++---
kernel/trace/trace_kprobe.c |8 +---
kernel/trace/trace_probe.h | 12 ++--
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index ae3a2d519e50..6400e1bf97c5 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -533,9 +533,7 @@ config FUNCTION_PROFILER
config BPF_KPROBE_OVERRIDE
bool "Enable BPF programs to override a kprobed function"
depends on BPF_EVENTS
- depends on KPROBES_ON_FTRACE
depends on HAVE_KPROBE_OVERRIDE
- depends on DYNAMIC_FTRACE_WITH_REGS
default n
help
Allows BPF to override the execution of a probed function and
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index f6d2327ecb59..d663660f8392 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -800,11 +800,11 @@ int perf_event_attach_bpf_prog(struct perf_event *event,
int ret = -EEXIST;
/*
-* Kprobe override only works for ftrace based kprobes, and only if they
-* are on the opt-in list.
+* Kprobe override only works if they are on the function entry,
+* and only if they are on the opt-in list.
*/
if (prog->kprobe_override &&
- (!trace_kprobe_ftrace(event->tp_event) ||
+ (!trace_kprobe_on_func_entry(event->tp_event) ||
!trace_kprobe_error_injectable(event->tp_event)))
return -EINVAL;
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 91f4b57dab82..265e3e27e8dc 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -88,13 +88,15 @@ static nokprobe_inline unsigned long
trace_kprobe_nhit(struct trace_kprobe *tk)
return nhit;
}
-int trace_kprobe_ftrace(struct trace_event_call *call)
+bool trace_kprobe_on_func_entry(struct trace_event_call *call)
{
struct trace_kprobe *tk = (struct trace_kprobe *)call->data;
- return kprobe_ftrace(&tk->rp.kp);
+
+ return kprobe_on_func_entry(tk->rp.kp.addr, tk->rp.kp.symbol_name,
+ tk->rp.kp.offset);
That would be nice, but did you test this?
Yes, because the jprobe, which was only official user of modifying execution
path using kprobe, did same way to check. (and kretprobe also does it)
My understanding that kprobe will restore all regs and
here we need to override return ip _and_ value.
yes, no problem. kprobe restore all regs from pt_regs, including regs->ip.
Could you add a patch with the test the way Josef did
or describe the steps to test this new mode?
Would you mean below patch? If so, it should work without any change.
[PATCH v10 4/5] samples/bpf: add a test for bpf_override_return
yeah. I expect bpf_override_return test to work as-is.
I'm asking for the test for new functionality added by this patch.
In particular kprobe on func entry without ftrace.
How did you test it?
This function is used in kretprobe and jprobe. Jprobe was the user of
"modifying instruction pointer to another function" in kprobes.
If it doesn't work, jprobe also doesn't work, this means you can not
modify IP by kprobes anymore.
Anyway, until linux-4.13, that was well tested by kprobe smoke test.
and how I can repeat the test?
I'm still not sure that it works correctly.
That works correctly because it checks given address is on the entry
point (the 1st instruction) of a function, using kallsyms.
The reason why I made another flag for ftrace was, there are 2 modes
for ftrace dynamic instrumentation, fentry and mcount.
With new fentry mode, ftrace will be put on the first instruction
of the function, so it will work as you expected.
With traditional gcc mcount, ftrace will be called after making call
frame for _mcount(). This means if you modify ip, it will not work
or cause a trouble because _mcount call frame is still on the stack.
So, current ftrace-based checker doesn't work, it depends on the ca
Re: [RFC PATCH bpf-next v2 4/4] error-injection: Support fault injection framework
On 12/27/17 12:09 AM, Masami Hiramatsu wrote: On Tue, 26 Dec 2017 18:12:56 -0800 Alexei Starovoitov wrote: On Tue, Dec 26, 2017 at 04:48:25PM +0900, Masami Hiramatsu wrote: Support in-kernel fault-injection framework via debugfs. This allows you to inject a conditional error to specified function using debugfs interfaces. Signed-off-by: Masami Hiramatsu --- Documentation/fault-injection/fault-injection.txt |5 + kernel/Makefile |1 kernel/fail_function.c| 169 + lib/Kconfig.debug | 10 + 4 files changed, 185 insertions(+) create mode 100644 kernel/fail_function.c diff --git a/Documentation/fault-injection/fault-injection.txt b/Documentation/fault-injection/fault-injection.txt index 918972babcd8..6243a588dd71 100644 --- a/Documentation/fault-injection/fault-injection.txt +++ b/Documentation/fault-injection/fault-injection.txt @@ -30,6 +30,11 @@ o fail_mmc_request injects MMC data errors on devices permitted by setting debugfs entries under /sys/kernel/debug/mmc0/fail_mmc_request +o fail_function + + injects error return on specific functions by setting debugfs entries + under /sys/kernel/debug/fail_function. No boot option supported. I like it. Could you document it a bit better? Yes, I will do in next series. In particular retval is configurable, but without an example no one will be able to figure out how to use it. Ah, right. BTW, as I pointed in the covermail, should we store the expected error value range into the injectable list? e.g. ALLOW_ERROR_INJECTION(open_ctree, -1, -MAX_ERRNO) And provide APIs to check/get it. I'm afraid such check would be too costly. Right now we have only two functions marked but I expect hundreds more will be added in the near future as soon as developers realize the potential of such error injection. All of ALLOW_ERROR_INJECTION marks add 8 byte overhead each to .data. Multiple by 1k and we have 8k of data spent on marks. If we add max/min range marks that doubles it for very little use. I think marking function only is enough. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH bpf-next v2 1/4] tracing/kprobe: bpf: Check error injectable event is on function entry
On 12/26/17 9:56 PM, Masami Hiramatsu wrote:
On Tue, 26 Dec 2017 17:57:32 -0800
Alexei Starovoitov wrote:
On Tue, Dec 26, 2017 at 04:46:59PM +0900, Masami Hiramatsu wrote:
Check whether error injectable event is on function entry or not.
Currently it checks the event is ftrace-based kprobes or not,
but that is wrong. It should check if the event is on the entry
of target function. Since error injection will override a function
to just return with modified return value, that operation must
be done before the target function starts making stackframe.
As a side effect, bpf error injection is no need to depend on
function-tracer. It can work with sw-breakpoint based kprobe
events too.
Signed-off-by: Masami Hiramatsu
---
kernel/trace/Kconfig|2 --
kernel/trace/bpf_trace.c|6 +++---
kernel/trace/trace_kprobe.c |8 +---
kernel/trace/trace_probe.h | 12 ++--
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index ae3a2d519e50..6400e1bf97c5 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -533,9 +533,7 @@ config FUNCTION_PROFILER
config BPF_KPROBE_OVERRIDE
bool "Enable BPF programs to override a kprobed function"
depends on BPF_EVENTS
- depends on KPROBES_ON_FTRACE
depends on HAVE_KPROBE_OVERRIDE
- depends on DYNAMIC_FTRACE_WITH_REGS
default n
help
Allows BPF to override the execution of a probed function and
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index f6d2327ecb59..d663660f8392 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -800,11 +800,11 @@ int perf_event_attach_bpf_prog(struct perf_event *event,
int ret = -EEXIST;
/*
-* Kprobe override only works for ftrace based kprobes, and only if they
-* are on the opt-in list.
+* Kprobe override only works if they are on the function entry,
+* and only if they are on the opt-in list.
*/
if (prog->kprobe_override &&
- (!trace_kprobe_ftrace(event->tp_event) ||
+ (!trace_kprobe_on_func_entry(event->tp_event) ||
!trace_kprobe_error_injectable(event->tp_event)))
return -EINVAL;
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 91f4b57dab82..265e3e27e8dc 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -88,13 +88,15 @@ static nokprobe_inline unsigned long
trace_kprobe_nhit(struct trace_kprobe *tk)
return nhit;
}
-int trace_kprobe_ftrace(struct trace_event_call *call)
+bool trace_kprobe_on_func_entry(struct trace_event_call *call)
{
struct trace_kprobe *tk = (struct trace_kprobe *)call->data;
- return kprobe_ftrace(&tk->rp.kp);
+
+ return kprobe_on_func_entry(tk->rp.kp.addr, tk->rp.kp.symbol_name,
+ tk->rp.kp.offset);
That would be nice, but did you test this?
Yes, because the jprobe, which was only official user of modifying execution
path using kprobe, did same way to check. (and kretprobe also does it)
My understanding that kprobe will restore all regs and
here we need to override return ip _and_ value.
yes, no problem. kprobe restore all regs from pt_regs, including regs->ip.
Could you add a patch with the test the way Josef did
or describe the steps to test this new mode?
Would you mean below patch? If so, it should work without any change.
[PATCH v10 4/5] samples/bpf: add a test for bpf_override_return
yeah. I expect bpf_override_return test to work as-is.
I'm asking for the test for new functionality added by this patch.
In particular kprobe on func entry without ftrace.
How did you test it?
and how I can repeat the test?
I'm still not sure that it works correctly.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH bpf-next v2 4/4] error-injection: Support fault injection framework
On Tue, Dec 26, 2017 at 04:48:25PM +0900, Masami Hiramatsu wrote: > Support in-kernel fault-injection framework via debugfs. > This allows you to inject a conditional error to specified > function using debugfs interfaces. > > Signed-off-by: Masami Hiramatsu > --- > Documentation/fault-injection/fault-injection.txt |5 + > kernel/Makefile |1 > kernel/fail_function.c| 169 > + > lib/Kconfig.debug | 10 + > 4 files changed, 185 insertions(+) > create mode 100644 kernel/fail_function.c > > diff --git a/Documentation/fault-injection/fault-injection.txt > b/Documentation/fault-injection/fault-injection.txt > index 918972babcd8..6243a588dd71 100644 > --- a/Documentation/fault-injection/fault-injection.txt > +++ b/Documentation/fault-injection/fault-injection.txt > @@ -30,6 +30,11 @@ o fail_mmc_request >injects MMC data errors on devices permitted by setting >debugfs entries under /sys/kernel/debug/mmc0/fail_mmc_request > > +o fail_function > + > + injects error return on specific functions by setting debugfs entries > + under /sys/kernel/debug/fail_function. No boot option supported. I like it. Could you document it a bit better? In particular retval is configurable, but without an example no one will be able to figure out how to use it. I think you can drop RFC tag from the next version of these patches. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH bpf-next v2 3/4] error-injection: Separate error-injection from kprobe
On Tue, Dec 26, 2017 at 04:47:55PM +0900, Masami Hiramatsu wrote: > Since error-injection framework is not limited to be used > by kprobes, nor bpf. Other kernel subsystems can use it > freely for checking safeness of error-injection, e.g. > livepatch, ftrace etc. > So this separate error-injection framework from kprobes. > > Some differences has been made: > > - "kprobe" word is removed from any APIs/structures. > - BPF_ALLOW_ERROR_INJECTION() is renamed to > ALLOW_ERROR_INJECTION() since it is not limited for BPF too. > - CONFIG_FUNCTION_ERROR_INJECTION is the config item of this > feature. It is automatically enabled if the arch supports > error injection feature for kprobe or ftrace etc. > > Signed-off-by: Masami Hiramatsu > --- > Changes in v2: >- Fix the override function name to override_function_with_return() >- Show only function name in the list, user don't have to care about > it's size, since function override only happens at the entry. looks like nice cleanup. Acked-by: Alexei Starovoitov -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH bpf-next v2 2/4] tracing/kprobe: bpf: Compare instruction pointer with original one
On Tue, Dec 26, 2017 at 04:47:26PM +0900, Masami Hiramatsu wrote:
> Compare instruction pointer with original one on the
> stack instead using per-cpu bpf_kprobe_override flag.
>
> This patch also consolidates reset_current_kprobe() and
> preempt_enable_no_resched() blocks. Those can be done
> in one place.
>
> Signed-off-by: Masami Hiramatsu
> ---
> kernel/trace/bpf_trace.c|1 -
> kernel/trace/trace_kprobe.c | 21 +++--
> 2 files changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index d663660f8392..cefa9b0e396c 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -83,7 +83,6 @@ EXPORT_SYMBOL_GPL(trace_call_bpf);
> #ifdef CONFIG_BPF_KPROBE_OVERRIDE
> BPF_CALL_2(bpf_override_return, struct pt_regs *, regs, unsigned long, rc)
> {
> - __this_cpu_write(bpf_kprobe_override, 1);
> regs_set_return_value(regs, rc);
> arch_ftrace_kprobe_override_function(regs);
> return 0;
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 265e3e27e8dc..a7c7035963f2 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -42,8 +42,6 @@ struct trace_kprobe {
> (offsetof(struct trace_kprobe, tp.args) + \
> (sizeof(struct probe_arg) * (n)))
>
> -DEFINE_PER_CPU(int, bpf_kprobe_override);
> -
> static nokprobe_inline bool trace_kprobe_is_return(struct trace_kprobe *tk)
> {
> return tk->rp.handler != NULL;
> @@ -1204,6 +1202,7 @@ kprobe_perf_func(struct trace_kprobe *tk, struct
> pt_regs *regs)
> int rctx;
>
> if (bpf_prog_array_valid(call)) {
> + unsigned long orig_ip = instruction_pointer(regs);
> int ret;
>
> ret = trace_call_bpf(call, regs);
> @@ -1211,12 +1210,13 @@ kprobe_perf_func(struct trace_kprobe *tk, struct
> pt_regs *regs)
> /*
>* We need to check and see if we modified the pc of the
>* pt_regs, and if so clear the kprobe and return 1 so that we
> - * don't do the instruction skipping. Also reset our state so
> - * we are clean the next pass through.
> + * don't do the single stepping.
> + * The ftrace kprobe handler leaves it up to us to re-enable
> + * preemption here before returning if we've modified the ip.
>*/
> - if (__this_cpu_read(bpf_kprobe_override)) {
> - __this_cpu_write(bpf_kprobe_override, 0);
> + if (orig_ip != instruction_pointer(regs)) {
> reset_current_kprobe();
> + preempt_enable_no_resched();
This is great idea.
Acked-by: Alexei Starovoitov
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH bpf-next v2 1/4] tracing/kprobe: bpf: Check error injectable event is on function entry
On Tue, Dec 26, 2017 at 04:46:59PM +0900, Masami Hiramatsu wrote:
> Check whether error injectable event is on function entry or not.
> Currently it checks the event is ftrace-based kprobes or not,
> but that is wrong. It should check if the event is on the entry
> of target function. Since error injection will override a function
> to just return with modified return value, that operation must
> be done before the target function starts making stackframe.
>
> As a side effect, bpf error injection is no need to depend on
> function-tracer. It can work with sw-breakpoint based kprobe
> events too.
>
> Signed-off-by: Masami Hiramatsu
> ---
> kernel/trace/Kconfig|2 --
> kernel/trace/bpf_trace.c|6 +++---
> kernel/trace/trace_kprobe.c |8 +---
> kernel/trace/trace_probe.h | 12 ++--
> 4 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index ae3a2d519e50..6400e1bf97c5 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -533,9 +533,7 @@ config FUNCTION_PROFILER
> config BPF_KPROBE_OVERRIDE
> bool "Enable BPF programs to override a kprobed function"
> depends on BPF_EVENTS
> - depends on KPROBES_ON_FTRACE
> depends on HAVE_KPROBE_OVERRIDE
> - depends on DYNAMIC_FTRACE_WITH_REGS
> default n
> help
>Allows BPF to override the execution of a probed function and
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index f6d2327ecb59..d663660f8392 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -800,11 +800,11 @@ int perf_event_attach_bpf_prog(struct perf_event *event,
> int ret = -EEXIST;
>
> /*
> - * Kprobe override only works for ftrace based kprobes, and only if they
> - * are on the opt-in list.
> + * Kprobe override only works if they are on the function entry,
> + * and only if they are on the opt-in list.
>*/
> if (prog->kprobe_override &&
> - (!trace_kprobe_ftrace(event->tp_event) ||
> + (!trace_kprobe_on_func_entry(event->tp_event) ||
>!trace_kprobe_error_injectable(event->tp_event)))
> return -EINVAL;
>
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 91f4b57dab82..265e3e27e8dc 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -88,13 +88,15 @@ static nokprobe_inline unsigned long
> trace_kprobe_nhit(struct trace_kprobe *tk)
> return nhit;
> }
>
> -int trace_kprobe_ftrace(struct trace_event_call *call)
> +bool trace_kprobe_on_func_entry(struct trace_event_call *call)
> {
> struct trace_kprobe *tk = (struct trace_kprobe *)call->data;
> - return kprobe_ftrace(&tk->rp.kp);
> +
> + return kprobe_on_func_entry(tk->rp.kp.addr, tk->rp.kp.symbol_name,
> + tk->rp.kp.offset);
That would be nice, but did you test this?
My understanding that kprobe will restore all regs and
here we need to override return ip _and_ value.
Could you add a patch with the test the way Josef did
or describe the steps to test this new mode?
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 1/5] add infrastructure for tagging functions as error injectable
On 12/20/17 3:00 AM, Masami Hiramatsu wrote:
On Fri, 15 Dec 2017 14:12:52 -0500
Josef Bacik wrote:
From: Josef Bacik
Using BPF we can override kprob'ed functions and return arbitrary
values. Obviously this can be a bit unsafe, so make this feature opt-in
for functions. Simply tag a function with KPROBE_ERROR_INJECT_SYMBOL in
order to give BPF access to that function for error injection purposes.
Signed-off-by: Josef Bacik
Acked-by: Ingo Molnar
---
include/asm-generic/vmlinux.lds.h | 10 +++
include/linux/bpf.h | 11 +++
include/linux/kprobes.h | 1 +
include/linux/module.h| 5 ++
kernel/kprobes.c | 163 ++
kernel/module.c | 6 +-
6 files changed, 195 insertions(+), 1 deletion(-)
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index ee8b707d9fa9..a2e8582d094a 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -136,6 +136,15 @@
#define KPROBE_BLACKLIST()
#endif
+#ifdef CONFIG_BPF_KPROBE_OVERRIDE
+#define ERROR_INJECT_LIST(). = ALIGN(8);
\
+
VMLINUX_SYMBOL(__start_kprobe_error_inject_list) = .; \
+ KEEP(*(_kprobe_error_inject_list))
\
+ VMLINUX_SYMBOL(__stop_kprobe_error_inject_list)
= .;
+#else
+#define ERROR_INJECT_LIST()
+#endif
+
#ifdef CONFIG_EVENT_TRACING
#define FTRACE_EVENTS(). = ALIGN(8);
\
VMLINUX_SYMBOL(__start_ftrace_events) = .; \
@@ -564,6 +573,7 @@
FTRACE_EVENTS() \
TRACE_SYSCALLS()\
KPROBE_BLACKLIST() \
+ ERROR_INJECT_LIST() \
MEM_DISCARD(init.rodata)\
CLK_OF_TABLES() \
RESERVEDMEM_OF_TABLES() \
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index e55e4255a210..7f4d2a953173 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -576,4 +576,15 @@ extern const struct bpf_func_proto
bpf_sock_map_update_proto;
void bpf_user_rnd_init_once(void);
u64 bpf_user_rnd_u32(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
+#if defined(__KERNEL__) && !defined(__ASSEMBLY__)
+#ifdef CONFIG_BPF_KPROBE_OVERRIDE
+#define BPF_ALLOW_ERROR_INJECTION(fname) \
+static unsigned long __used\
+ __attribute__((__section__("_kprobe_error_inject_list"))) \
+ _eil_addr_##fname = (unsigned long)fname;
+#else
+#define BPF_ALLOW_ERROR_INJECTION(fname)
+#endif
+#endif
This part shows this feature belongs to bpf, if it is a part of kprobes,
it should be defined in include/asm-generic/kprobes.h as NOKPROBE_SYMBOL
does.
Why this is defined in BPF, but list is under kprobes?
because Ingo specifically requested that macro that marks the function
will be in bpf.h, so any .c file that starts adding such marks will
include that file instead of pulling stuff from kprobe.
So there is no direct relationship with kprobe.
For example, kprobe user modules can OVERRIDE any functions.
And there is no generic error injection code in the kernel
except for the bpf currently.
_currently_ is key word.
Of course, I can accept this code if you accept that I make a
generic error injection code on ftrace without BPF.
what stops other pieces of kernel to use the same technique?
The bpf verifier coupled together with opt-in
per-function marks via BPF_ALLOW_ERROR_INJECTION
give _safe_ way to do error injection.
I can imagine how you can hack kprobe text based interface to
use the same technique, but I suggest to wait and see how we
build on it in bpf land before replicating things in
pure kprobe land.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 1/5] add infrastructure for tagging functions as error injectable
On 12/19/17 11:13 PM, Masami Hiramatsu wrote: On Tue, 19 Dec 2017 18:14:17 -0800 Alexei Starovoitov wrote: On 12/18/17 10:29 PM, Masami Hiramatsu wrote: +#if defined(__KERNEL__) && !defined(__ASSEMBLY__) +#ifdef CONFIG_BPF_KPROBE_OVERRIDE BTW, CONFIG_BPF_KPROBE_OVERRIDE is also confusable name. Since this feature override a function to just return with some return value (as far as I understand, or would you also plan to modify execution path inside a function?), I think it should be better CONFIG_BPF_FUNCTION_OVERRIDE or CONFIG_BPF_EXECUTION_OVERRIDE. I don't think such renaming makes sense. The feature is overriding kprobe by changing how kprobe returns. It doesn't override BPF_FUNCTION or BPF_EXECUTION. No, I meant this is BPF's feature which override FUNCTION, so BPF is a kind of namespace. (Is that only for a function entry because it can not tweak stackframe at this morment?) The kernel enters and exists bpf program as normal. Yeah, but that bpf program modifies instruction pointer, am I correct? no. bpf side is asking kprobe side to modify it. bpf cannot do such things as modifying IP or any other register directly. Indeed, BPF is based on kprobes, but it seems you are limiting it with ftrace (function-call trace) (I'm not sure the reason why), so using "kprobes" for this feature seems strange for me. do you have an idea how kprobe override can happen when kprobe placed in the middle of the function? For example, if you know a basic block in the function, maybe you can skip a block or something like that. But nowadays it is somewhat hard because optimizer mixed it up. still missing how that can work... -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 1/5] add infrastructure for tagging functions as error injectable
On 12/18/17 10:29 PM, Masami Hiramatsu wrote: +#if defined(__KERNEL__) && !defined(__ASSEMBLY__) +#ifdef CONFIG_BPF_KPROBE_OVERRIDE BTW, CONFIG_BPF_KPROBE_OVERRIDE is also confusable name. Since this feature override a function to just return with some return value (as far as I understand, or would you also plan to modify execution path inside a function?), I think it should be better CONFIG_BPF_FUNCTION_OVERRIDE or CONFIG_BPF_EXECUTION_OVERRIDE. I don't think such renaming makes sense. The feature is overriding kprobe by changing how kprobe returns. It doesn't override BPF_FUNCTION or BPF_EXECUTION. The kernel enters and exists bpf program as normal. Indeed, BPF is based on kprobes, but it seems you are limiting it with ftrace (function-call trace) (I'm not sure the reason why), so using "kprobes" for this feature seems strange for me. do you have an idea how kprobe override can happen when kprobe placed in the middle of the function? Please make your suggestion as patches based on top of bpf-next. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 3/5] bpf: add a bpf_override_function helper
On 12/15/17 11:12 AM, Josef Bacik wrote:
+#ifdef CONFIG_BPF_KPROBE_OVERRIDE
+BPF_CALL_2(bpf_override_return, struct pt_regs *, regs, unsigned long, rc)
+{
+ __this_cpu_write(bpf_kprobe_override, 1);
+ regs_set_return_value(regs, rc);
+ arch_ftrace_kprobe_override_function(regs);
+ return 0;
+}
since you're doing a respin can you adopt the change I did to make
this helper fail at load time if that #config is not set
instead of runtime?
Also how big is the v9-v10 change ?
May be do it as separate patch, since previous set already sitting
in bpf-next and there are patches on top?
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 0/5] Add the ability to do BPF directed error injection
On 12/11/17 8:36 AM, Josef Bacik wrote: This is the same as v8, just rebased onto the bpf tree. v8->v9: - rebased onto the bpf tree. v7->v8: - removed the _ASM_KPROBE_ERROR_INJECT since it was not needed. v6->v7: - moved the opt-in macro to bpf.h out of kprobes.h. v5->v6: - add BPF_ALLOW_ERROR_INJECTION() tagging for functions that will support this feature. This way only functions that opt-in will be allowed to be overridden. - added a btrfs patch to allow error injection for open_ctree() so that the bpf sample actually works. v4->v5: - disallow kprobe_override programs from being put in the prog map array so we don't tail call into something we didn't check. This allows us to make the normal path still fast without a bunch of percpu operations. v3->v4: - fix a build error found by kbuild test bot (I didn't wait long enough apparently.) - Added a warning message as per Daniels suggestion. v2->v3: - added a ->kprobe_override flag to bpf_prog. - added some sanity checks to disallow attaching bpf progs that have ->kprobe_override set that aren't for ftrace kprobes. - added the trace_kprobe_ftrace helper to check if the trace_event_call is a ftrace kprobe. - renamed bpf_kprobe_state to bpf_kprobe_override, fixed it so we only read this value in the kprobe path, and thus only write to it if we're overriding or clearing the override. v1->v2: - moved things around to make sure that bpf_override_return could really only be used for an ftrace kprobe. - killed the special return values from trace_call_bpf. - renamed pc_modified to bpf_kprobe_state so bpf_override_return could tell if it was being called from an ftrace kprobe context. - reworked the logic in kprobe_perf_func to take advantage of bpf_kprobe_state. - updated the test as per Alexei's review. - Original message - A lot of our error paths are not well tested because we have no good way of injecting errors generically. Some subystems (block, memory) have ways to inject errors, but they are random so it's hard to get reproduceable results. With BPF we can add determinism to our error injection. We can use kprobes and other things to verify we are injecting errors at the exact case we are trying to test. This patch gives us the tool to actual do the error injection part. It is very simple, we just set the return value of the pt_regs we're given to whatever we provide, and then override the PC with a dummy function that simply returns. Right now this only works on x86, but it would be simple enough to expand to other architectures. Thanks, Applied, thanks Josef! While applying in the patch "bpf: add a bpf_override_function helper" I moved ifdef CONFIG_BPF_KPROBE_OVERRIDE few lines, so when it's not set the program will fail at load time with error "unknown func bpf_override_return#58" instead of returning EINVAL at run-time. That's more standard way of adding new helpers. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 0/4] Add the ability to do BPF directed error injection
On Wed, Nov 22, 2017 at 04:23:29PM -0500, Josef Bacik wrote: > This is hopefully the final version, I've addressed the comment by Igno and > added his Acks. > > v6->v7: > - moved the opt-in macro to bpf.h out of kprobes.h. Thanks Josef! All patches look great to me. We'll probably take them all into bpf-next.git to start testing together with other bpf changes and when net-next reopens will send them to Dave. Then optionally can send pull-req for the first patch only to tip if Ingo thinks that there can be conflicts with the work happening in parallel on kprobe/x86 bits. This way hopefully there will be no conflicts during the next merge window. Makes sense? > v5->v6: > - add BPF_ALLOW_ERROR_INJECTION() tagging for functions that will support this > feature. This way only functions that opt-in will be allowed to be > overridden. > - added a btrfs patch to allow error injection for open_ctree() so that the > bpf > sample actually works. > > v4->v5: > - disallow kprobe_override programs from being put in the prog map array so we > don't tail call into something we didn't check. This allows us to make the > normal path still fast without a bunch of percpu operations. > > v3->v4: > - fix a build error found by kbuild test bot (I didn't wait long enough > apparently.) > - Added a warning message as per Daniels suggestion. > > v2->v3: > - added a ->kprobe_override flag to bpf_prog. > - added some sanity checks to disallow attaching bpf progs that have > ->kprobe_override set that aren't for ftrace kprobes. > - added the trace_kprobe_ftrace helper to check if the trace_event_call is a > ftrace kprobe. > - renamed bpf_kprobe_state to bpf_kprobe_override, fixed it so we only read > this > value in the kprobe path, and thus only write to it if we're overriding or > clearing the override. > > v1->v2: > - moved things around to make sure that bpf_override_return could really only > be > used for an ftrace kprobe. > - killed the special return values from trace_call_bpf. > - renamed pc_modified to bpf_kprobe_state so bpf_override_return could tell if > it was being called from an ftrace kprobe context. > - reworked the logic in kprobe_perf_func to take advantage of > bpf_kprobe_state. > - updated the test as per Alexei's review. > > - Original message - > > A lot of our error paths are not well tested because we have no good way of > injecting errors generically. Some subystems (block, memory) have ways to > inject errors, but they are random so it's hard to get reproduceable results. > > With BPF we can add determinism to our error injection. We can use kprobes > and > other things to verify we are injecting errors at the exact case we are trying > to test. This patch gives us the tool to actual do the error injection part. > It is very simple, we just set the return value of the pt_regs we're given to > whatever we provide, and then override the PC with a dummy function that > simply > returns. > > Right now this only works on x86, but it would be simple enough to expand to > other architectures. Thanks, > > Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
