| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20170329002335.1d1fcc491765b632f470f99b@kernel.org>
Date: Wed, 29 Mar 2017 00:23:35 +0900
From: Masami Hiramatsu <mhiramat@...nel.org>
To: Alban Crequy <alban.crequy@...il.com>
Cc: Alban Crequy <alban@...volk.io>,
Alexei Starovoitov <ast@...nel.org>,
Jonathan Corbet <corbet@....net>,
Steven Rostedt <rostedt@...dmis.org>,
Ingo Molnar <mingo@...hat.com>,
Masami Hiramatsu <mhiramat@...nel.org>,
Arnaldo Carvalho de Melo <acme@...hat.com>,
Omar Sandoval <osandov@...com>, linux-doc@...r.kernel.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
iago@...volk.io, michael@...volk.io,
"Dorau, Lukasz" <lukasz.dorau@...el.com>
Subject: Re: [PATCH v1] tracing/kprobes: expose maxactive for kretprobe in
kprobe_events
On Tue, 28 Mar 2017 15:52:22 +0200
Alban Crequy <alban.crequy@...il.com> wrote:
> When a kretprobe is installed on a kernel function, there is a maximum
> limit of how many calls in parallel it can catch (aka "maxactive"). A
> kernel module could call register_kretprobe() and initialize maxactive
> (see example in samples/kprobes/kretprobe_example.c).
>
> But that is not exposed to userspace and it is currently not possible to
> choose maxactive when writing to /sys/kernel/debug/tracing/kprobe_events
I see, I found same issue last week...
>
> The default maxactive can be as low as 1 on single-core with a
> non-preemptive kernel. This is too low and we need to increase it not
> only for recursive functions, but for functions that sleep or resched.
>
> This patch updates the format of the command that can be written to
> kprobe_events so that maxactive can be optionally specified.
Good! this is completely same what I'm planning to add.
>
> I need this for a bpf program attached to the kretprobe of
> inet_csk_accept, which can sleep for a long time.
I'm also preparing another patch for using kmalloc(GFP_ATOMIC) in
kretprobe_pre_handler(), since this manual way is sometimes hard to
estimate how many instances needed. Anyway, since that may involve
unwilling memory allocation, this patch also needed.
>
> BugLink: https://github.com/iovisor/bcc/issues/1072
Could you also add Lukasz to Cc list, since he had reported an issue
related this one.
https://www.spinics.net/lists/linux-trace/msg00448.html
Please see my comments below.
> Signed-off-by: Alban Crequy <alban@...volk.io>
> ---
> Documentation/trace/kprobetrace.txt | 4 +++-
> kernel/trace/trace_kprobe.c | 34 +++++++++++++++++++++++++++++-----
> 2 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/trace/kprobetrace.txt b/Documentation/trace/kprobetrace.txt
> index 41ef9d8..655ca7e 100644
> --- a/Documentation/trace/kprobetrace.txt
> +++ b/Documentation/trace/kprobetrace.txt
> @@ -23,7 +23,7 @@ current_tracer. Instead of that, add probe points via
> Synopsis of kprobe_events
> -------------------------
> p[:[GRP/]EVENT] [MOD:]SYM[+offs]|MEMADDR [FETCHARGS] : Set a probe
> - r[:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS] : Set a return probe
> + r[MAXACTIVE][:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS] : Set a return probe
> -:[GRP/]EVENT : Clear a probe
>
> GRP : Group name. If omitted, use "kprobes" for it.
> @@ -32,6 +32,8 @@ Synopsis of kprobe_events
> MOD : Module name which has given SYM.
> SYM[+offs] : Symbol+offset where the probe is inserted.
> MEMADDR : Address where the probe is inserted.
> + MAXACTIVE : Maximum number of instances of the specified function that
> + can be probed simultaneously, or 0 for the default.(*)
Here, yoy don't need (*), since [MAXACTIVE] is not a FETCHARGS.
>
> FETCHARGS : Arguments. Each probe can have up to 128 args.
> %REG : Fetch register REG
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 5f688cc..807e01c 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -282,6 +282,7 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group,
> void *addr,
> const char *symbol,
> unsigned long offs,
> + int maxactive,
> int nargs, bool is_return)
> {
> struct trace_kprobe *tk;
> @@ -309,6 +310,8 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group,
> else
> tk->rp.kp.pre_handler = kprobe_dispatcher;
>
> + tk->rp.maxactive = maxactive;
> +
> if (!event || !is_good_name(event)) {
> ret = -EINVAL;
> goto error;
> @@ -598,8 +601,10 @@ static int create_trace_kprobe(int argc, char **argv)
> {
> /*
> * Argument syntax:
> - * - Add kprobe: p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS]
> - * - Add kretprobe: r[:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS]
> + * - Add kprobe:
> + * p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS]
> + * - Add kretprobe:
> + * r[MAXACTIVE][:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS]
> * Fetch args:
> * $retval : fetch return value
> * $stack : fetch stack address
> @@ -619,6 +624,7 @@ static int create_trace_kprobe(int argc, char **argv)
> int i, ret = 0;
> bool is_return = false, is_delete = false;
> char *symbol = NULL, *event = NULL, *group = NULL;
> + int maxactive = 0;
> char *arg;
> unsigned long offset = 0;
> void *addr = NULL;
> @@ -637,8 +643,26 @@ static int create_trace_kprobe(int argc, char **argv)
> return -EINVAL;
> }
>
> - if (argv[0][1] == ':') {
> + if (is_return && isdigit(argv[0][1]) && strchr(&argv[0][1], ':')) {
This only supports r[MAXACTIVE:[GRP/]EVENT]. e.g "r100" without event name.
Thank you,
> + event = strchr(&argv[0][1], ':') + 1;
> + event[-1] = '\0';
> + ret = kstrtouint(&argv[0][1], 0, &maxactive);
> + if (ret) {
> + pr_info("Failed to parse maxactive.\n");
> + return ret;
> + }
> + /* kretprobes instances are iterated over via a list. The
> + * maximum should stay reasonable.
> + */
> + if (maxactive > 1024) {
> + pr_info("Maxactive is too big.\n");
> + return -EINVAL;
> + }
> + } else if (argv[0][1] == ':') {
> event = &argv[0][2];
> + }
> +
> + if (event) {
> if (strchr(event, '/')) {
> group = event;
> event = strchr(group, '/') + 1;
> @@ -718,8 +742,8 @@ static int create_trace_kprobe(int argc, char **argv)
> is_return ? 'r' : 'p', addr);
> event = buf;
> }
> - tk = alloc_trace_kprobe(group, event, addr, symbol, offset, argc,
> - is_return);
> + tk = alloc_trace_kprobe(group, event, addr, symbol, offset, maxactive,
> + argc, is_return);
> if (IS_ERR(tk)) {
> pr_info("Failed to allocate trace_probe.(%d)\n",
> (int)PTR_ERR(tk));
> --
> 2.7.4
>
--
Masami Hiramatsu <mhiramat@...nel.org>
Powered by blists - more mailing lists