Re: [PATCH v5 0/2] kprobes: improve error handling when arming/disarming kprobes

[Joe Lawrence]

Was this patch ever picked up in the tip tree?  (Maybe I'm not looking
in the right branch?)

Thanks,

-- Joe


On 01/19/2018 02:06 AM, Masami Hiramatsu wrote:
> Hi Ingo,
> 
> Could you pick this to tip tree?
> 
> Thank you,
> 
> On Wed, 10 Jan 2018 00:51:22 +0100
> Jessica Yu <jeyu@xxxxxxxxxx> wrote:
> 
>> Hi,
>>
>> This patchset attempts to improve error handling when arming or disarming
>> ftrace-based kprobes. The current behavior is to simply WARN when ftrace
>> (un-)registration fails, without propagating the error code. This can lead
>> to confusing situations where, for example, register_kprobe()/enable_kprobe()
>> would return 0 indicating success even if arming via ftrace had failed. In
>> this scenario we'd end up with a non-functioning kprobe even though kprobe
>> registration (or enablement) returned success. In this patchset, we take
>> errors from ftrace into account and propagate the error when we cannot arm
>> or disarm a kprobe.
>>
>> Below is an example that illustrates the problem using livepatch and
>> systemtap (which uses kprobes underneath). Both livepatch and kprobes use
>> ftrace ops with the IPMODIFY flag set, so registration at the same
>> function entry is limited to only one ftrace user. 
>>
>> Before
>> ------
>> # modprobe livepatch-sample 	# patches cmdline_proc_show, ftrace ops has IPMODIFY set
>> # stap -e 'probe kernel.function("cmdline_proc_show").call { printf ("cmdline_proc_show\n"); }'
>>
>>    .. (nothing prints after reading /proc/cmdline) ..
>>
>> The systemtap handler doesn't execute due to a kprobe arming failure caused
>> by a ftrace IPMODIFY conflict with livepatch, and there isn't an obvious
>> indication of error from systemtap (because register_kprobe() returned
>> success) unless the user inspects dmesg.
>>
>> After
>> -----
>> # modprobe livepatch-sample 
>> # stap -e 'probe kernel.function("cmdline_proc_show").call { printf ("cmdline_proc_show\n"); }'
>> WARNING: probe kernel.function("cmdline_proc_show@/home/jeyu/work/linux-next/fs/proc/cmdline.c:6").call (address 0xffffffffa82fe910) registration error (rc -16)
>>
>> Although the systemtap handler doesn't execute (as it shouldn't), the
>> ftrace error is propagated and now systemtap prints a visible error message
>> stating that (kprobe) registration had failed (because register_kprobe()
>> returned an error), along with the propagated error code.
>>
>> This patchset was based on Petr Mladek's original patchset (patches 2 and 3)
>> back in 2015, which improved kprobes error handling, found here:
>>
>>    https://lkml.org/lkml/2015/2/26/452
>>
>> However, further work on this had been paused since then and the patches
>> were not upstreamed.
>>
>> This patchset has been lightly sanity-tested (on linux-next) with kprobes,
>> kretprobes, and optimized kprobes. It passes the kprobes smoke test, but
>> more testing is greatly appreciated.
>>
>> Changes from v4:
>>  - Switch from WARN() to pr_debug() in arm_kprobe_ftrace() so the stack
>>    dumps don't pollute dmesg, as IPMODIFY conflicts can occur in normal usage
>>  - Added Masami's ack to the first patch
>>
>> Changes from v3:
>>  - Have (dis)arm_kprobe_ftrace() return -ENODEV instead of 0 in case of
>>    !CONFIG_KPROBES_ON_FTRACE
>>  - Add total count of all probes tried in (dis)arm_all_kprobes()
>>
>> Changes from v2:
>>  - Add missing synchronize rcu in register_aggr_kprobe()
>>  - s/kprobes/probes/ on error message in (dis)arm_all_kprobes()
>>
>> Changes from v1:
>> - Don't arm the kprobe before adding it to the kprobe table, otherwise
>>   we'll temporarily see a stray breakpoint.
>> - Remove kprobe from the kprobe_table and call synchronize_sched() if
>>   arming during register_kprobe() fails.
>> - add Masami's ack on the 2nd patch (unchanged from v1)
>>
>> ---
>> Jessica Yu (2):
>>   kprobes: propagate error from arm_kprobe_ftrace()
>>   kprobes: propagate error from disarm_kprobe_ftrace()
>>
>>  kernel/kprobes.c | 178 +++++++++++++++++++++++++++++++++++++++----------------
>>  1 file changed, 128 insertions(+), 50 deletions(-)
>>
>> -- 
>> 2.13.6
>>
> 
>