DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 457032173F
Date: Thu, 11 Jan 2018 21:07:35 +0900
From: Masami Hiramatsu <mhiramat@kernel.org>
To: Jessica Yu <jeyu@kernel.org>, Ingo Molnar <mingo@kernel.org>
Cc: mhiramat@kernel.org,
        Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>,
        "David S . Miller" <davem@davemloft.net>,
        Petr Mladek <pmladek@suse.com>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Joe Lawrence <joe.lawrence@redhat.com>,
        Jiri Kosina <jikos@kernel.org>,
        Miroslav Benes <mbenes@suse.cz>,
        Steven Rostedt <rostedt@goodmis.org>,
        live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 0/2] kprobes: improve error handling when
 arming/disarming kprobes
Message-Id: <20180111210735.8c88f5f22d35ca8acd44168c@kernel.org>
In-Reply-To: <20180109235124.30886-1-jeyu@kernel.org>
References: <20180109235124.30886-1-jeyu@kernel.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org

On Wed, 10 Jan 2018 00:51:22 +0100
Jessica Yu <jeyu@kernel.org> 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. 

This is not only systemtap, but also both bpf and ftrace (and perf-probe)
will face the same problem.

This series is good to me. Thanks!

Thank you,

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


-- 
Masami Hiramatsu <mhiramat@kernel.org>