Re: [BUGFIX PATCH bpf-next] error-injection: Fix to prohibit jump optimization

[Daniel Borkmann]

On 03/12/2018 03:06 PM, Masami Hiramatsu wrote:
> On Mon, 12 Mar 2018 11:44:21 +0100
> Daniel Borkmann <dan...@iogearbox.net> wrote:
>> On 03/12/2018 11:27 AM, Masami Hiramatsu wrote:
>>> On Mon, 12 Mar 2018 19:00:49 +0900
>>> Masami Hiramatsu <mhira...@kernel.org> wrote:
>>>
>>>> Since the kprobe which was optimized by jump can not change
>>>> the execution path, the kprobe for error-injection must not
>>>> be optimized. To prohibit it, set a dummy post-handler as
>>>> officially stated in Documentation/kprobes.txt.
>>>
>>> Note that trace-probe based BPF is not affected, because it
>>> ensures the trace-probe is based on ftrace, which is not
>>> jump optimized.
>>
>> Thanks for the fix! I presume this should go via bpf instead of bpf-next
>> tree since 4b1a29a7f542 ("error-injection: Support fault injection 
>> framework")
>> is in Linus' tree as well. Unless there are objection I would rather route
>> it that way so it would be for 4.16.
> 
> Ah, right! It should go into 4.16. It should be applicable cleanly either tree
> since there is only the above commit on kernel/fail_function.c :)

Applied to bpf tree, thanks 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: [BUGFIX PATCH bpf-next] error-injection: Fix to prohibit jump optimization

[Daniel Borkmann]

Hi Masami,

On 03/12/2018 11:27 AM, Masami Hiramatsu wrote:
> On Mon, 12 Mar 2018 19:00:49 +0900
> Masami Hiramatsu  wrote:
> 
>> Since the kprobe which was optimized by jump can not change
>> the execution path, the kprobe for error-injection must not
>> be optimized. To prohibit it, set a dummy post-handler as
>> officially stated in Documentation/kprobes.txt.
> 
> Note that trace-probe based BPF is not affected, because it
> ensures the trace-probe is based on ftrace, which is not
> jump optimized.

Thanks for the fix! I presume this should go via bpf instead of bpf-next
tree since 4b1a29a7f542 ("error-injection: Support fault injection framework")
is in Linus' tree as well. Unless there are objection I would rather route
it that way so it would be for 4.16.

Thanks,
Daniel

> Thanks,
> 
>>
>> Fixes: 4b1a29a7f542 ("error-injection: Support fault injection framework")
>> Signed-off-by: Masami Hiramatsu 
>> ---
>>  kernel/fail_function.c |   10 ++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/kernel/fail_function.c b/kernel/fail_function.c
>> index 21b0122cb39c..1d5632d8bbcc 100644
>> --- a/kernel/fail_function.c
>> +++ b/kernel/fail_function.c
>> @@ -14,6 +14,15 @@
>>  
>>  static int fei_kprobe_handler(struct kprobe *kp, struct pt_regs *regs);
>>  
>> +static void fei_post_handler(struct kprobe *kp, struct pt_regs *regs,
>> + unsigned long flags)
>> +{
>> +/*
>> + * A dummy post handler is required to prohibit optimizing, because
>> + * jump optimization does not support execution path overriding.
>> + */
>> +}
>> +
>>  struct fei_attr {
>>  struct list_head list;
>>  struct kprobe kp;
>> @@ -56,6 +65,7 @@ static struct fei_attr *fei_attr_new(const char *sym, 
>> unsigned long addr)
>>  return NULL;
>>  }
>>  attr->kp.pre_handler = fei_kprobe_handler;
>> +attr->kp.post_handler = fei_post_handler;
>>  attr->retval = adjust_error_retval(addr, 0);
>>  INIT_LIST_HEAD(>list);
>>  }
>>
> 
> 

--
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

[Daniel Borkmann]

On 12/20/2017 08:13 AM, Masami Hiramatsu wrote:
> On Tue, 19 Dec 2017 18:14:17 -0800
> Alexei Starovoitov  wrote:
[...]
>> Please make your suggestion as patches based on top of bpf-next.
> 
> bpf-next seems already pick this series. Would you mean I revert it and
> write new patch?

No, please submit as follow-ups instead, thanks 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: [PATCH v10 3/5] bpf: add a bpf_override_function helper

[Daniel Borkmann]

On 12/18/2017 10:51 AM, Masami Hiramatsu wrote:
> On Fri, 15 Dec 2017 14:12:54 -0500
> Josef Bacik  wrote:
>> From: Josef Bacik 
>>
>> Error injection is sloppy and very ad-hoc.  BPF could fill this niche
>> perfectly with it's kprobe functionality.  We could make sure errors are
>> only triggered in specific call chains that we care about with very
>> specific situations.  Accomplish this with the bpf_override_funciton
>> helper.  This will modify the probe'd callers return value to the
>> specified value and set the PC to an override function that simply
>> returns, bypassing the originally probed function.  This gives us a nice
>> clean way to implement systematic error injection for all of our code
>> paths.
> 
> OK, got it. I think the error_injectable function list should be defined
> in kernel/trace/bpf_trace.c because only bpf calls it and needs to care
> the "safeness".
> 
> [...]
>> diff --git a/arch/x86/kernel/kprobes/ftrace.c 
>> b/arch/x86/kernel/kprobes/ftrace.c
>> index 8dc0161cec8f..1ea748d682fd 100644
>> --- a/arch/x86/kernel/kprobes/ftrace.c
>> +++ b/arch/x86/kernel/kprobes/ftrace.c
>> @@ -97,3 +97,17 @@ int arch_prepare_kprobe_ftrace(struct kprobe *p)
>>  p->ainsn.boostable = false;
>>  return 0;
>>  }
>> +
>> +asmlinkage void override_func(void);
>> +asm(
>> +".type override_func, @function\n"
>> +"override_func:\n"
>> +"   ret\n"
>> +".size override_func, .-override_func\n"
>> +);
>> +
>> +void arch_ftrace_kprobe_override_function(struct pt_regs *regs)
>> +{
>> +regs->ip = (unsigned long)_func;
>> +}
>> +NOKPROBE_SYMBOL(arch_ftrace_kprobe_override_function);
> 
> Calling this as "override_function" is meaningless. This is a function
> which just return. So I think combination of just_return_func() and
> arch_bpf_override_func_just_return() will be better.
> 
> Moreover, this arch/x86/kernel/kprobes/ftrace.c is an archtecture
> dependent implementation of kprobes, not bpf.

Josef, please work out any necessary cleanups that would still need
to be addressed based on Masami's feedback and send them as follow-up
patches, thanks.

> Hmm, arch/x86/net/bpf_jit_comp.c will be better place?

(No, it's JIT only and I'd really prefer to keep it that way, mixing
 this would result in a huge mess.)
--
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

[Daniel Borkmann]

On 12/15/2017 09:34 PM, Alexei Starovoitov wrote:
[...]
> 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?

+1
--
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 v8 0/5] Add the ability to do BPF directed error injection

[Daniel Borkmann]

On 12/08/2017 09:24 PM, Josef Bacik wrote:
> On Fri, Dec 08, 2017 at 04:35:44PM +0100, Daniel Borkmann wrote:
>> On 12/06/2017 05:12 PM, Josef Bacik wrote:
>>> Jon noticed that I had a typo in my _ASM_KPROBE_ERROR_INJECT macro.  I went 
>>> to
>>> figure out why the compiler didn't catch it and it's because it was not used
>>> anywhere.  I had copied it from the trace blacklist code without 
>>> understanding
>>> where it was used as cscope didn't find the original macro I was looking 
>>> for, so
>>> I assumed it was some voodoo and left it in place.  Turns out cscope failed 
>>> me
>>> and I didn't need the macro at all, the trace blacklist thing I was looking 
>>> at
>>> was for marking assembly functions as blacklisted and I have no intention of
>>> marking assembly functions as error injectable at the moment.
>>>
>>> v7->v8:
>>> - removed the _ASM_KPROBE_ERROR_INJECT since it was not needed.
>>
>> The series doesn't apply cleanly to the bpf-next tree, so one last respin 
>> with
>> a rebase would unfortunately still be required, thanks!
> 
> I've rebased and let it sit in my git tree to make sure kbuild test bot didn't
> blow up, can you pull from
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/josef/btrfs-next.git 
> bpf-override-return
> 
> or do you want me to repost the whole series?  Thanks,

Yeah, the patches would need to end up on netdev, so once kbuild bot went
through fine after your rebase, please send the series.

Thanks,
Daniel
--
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 v8 0/5] Add the ability to do BPF directed error injection

[Daniel Borkmann]

On 12/06/2017 05:12 PM, Josef Bacik wrote:
> Jon noticed that I had a typo in my _ASM_KPROBE_ERROR_INJECT macro.  I went to
> figure out why the compiler didn't catch it and it's because it was not used
> anywhere.  I had copied it from the trace blacklist code without understanding
> where it was used as cscope didn't find the original macro I was looking for, 
> so
> I assumed it was some voodoo and left it in place.  Turns out cscope failed me
> and I didn't need the macro at all, the trace blacklist thing I was looking at
> was for marking assembly functions as blacklisted and I have no intention of
> marking assembly functions as error injectable at the moment.
> 
> v7->v8:
> - removed the _ASM_KPROBE_ERROR_INJECT since it was not needed.

The series doesn't apply cleanly to the bpf-next tree, so one last respin with
a rebase would unfortunately still be required, 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 1/5] add infrastructure for tagging functions as error injectable

[Daniel Borkmann]

On 11/28/2017 09:02 PM, Josef Bacik wrote:
> On Tue, Nov 28, 2017 at 11:58:41AM -0700, Jonathan Corbet wrote:
>> On Wed, 22 Nov 2017 16:23:30 -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 
>>> ---
>>>  arch/x86/include/asm/asm.h|   6 ++
>>>  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 +-
>>>  7 files changed, 201 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
>>> index b0dc91f4bedc..340f4cc43255 100644
>>> --- a/arch/x86/include/asm/asm.h
>>> +++ b/arch/x86/include/asm/asm.h
>>> @@ -85,6 +85,12 @@
>>> _ASM_PTR (entry);   \
>>> .popsection
>>>  
>>> +# define _ASM_KPROBE_ERROR_INJECT(entry)   \
>>> +   .pushsection "_kprobe_error_inject_list","aw" ; \
>>> +   _ASM_ALIGN ;\
>>> +   _ASM_PTR (entry);   \
>>> +   .popseciton
>>
>> So this stuff is not my area of greatest expertise, but I do have to wonder
>> how ".popseciton" can work ... ?
> 
> Well fuck, do you want me to send a increment Daniel/Alexei or resend this 
> patch
> fixed?  Thanks,

Sorry for late reply, please rebase + respin the whole series with
this fixed. There were also few typos in the cover letter / commit
messages that would be good to get fixed along the way.

Also, could you debug why this wasn't caught at compile/runtime during
testing?

Thanks a lot,
Daniel
--
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

[Daniel Borkmann]

On 11/22/2017 10:23 PM, 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.
> 
> 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,

Ok, given the remaining feedback from Ingo was addressed and therefore
the series acked, I've applied it to bpf-next tree, 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

Re: [PATCH v7 3/5] bpf: add a bpf_override_function helper

[Daniel Borkmann]

On 11/22/2017 10:23 PM, Josef Bacik wrote:
> From: Josef Bacik <jba...@fb.com>
> 
> Error injection is sloppy and very ad-hoc.  BPF could fill this niche
> perfectly with it's kprobe functionality.  We could make sure errors are
> only triggered in specific call chains that we care about with very
> specific situations.  Accomplish this with the bpf_override_funciton
> helper.  This will modify the probe'd callers return value to the
> specified value and set the PC to an override function that simply
> returns, bypassing the originally probed function.  This gives us a nice
> clean way to implement systematic error injection for all of our code
> paths.
> 
> Acked-by: Alexei Starovoitov <a...@kernel.org>
> Acked-by: Ingo Molnar <mi...@kernel.org>
> Signed-off-by: Josef Bacik <jba...@fb.com>

Series looks good to me as well; BPF bits:

Acked-by: Daniel Borkmann <dan...@iogearbox.net>
--
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