[BUGFIX PATCH] tracing/kprobes: Fix strpbrk() argument order

[BUGFIX PATCH] tracing/kprobes: Fix strpbrk() argument order

[Masami Hiramatsu]

Fix strpbrk()'s argument order, it must pass acceptable string
in 2nd argument. Note that this can cause a kernel panic where
it recovers backup character to code->data.

Fixes: a6682814f371 ("tracing/kprobes: Allow kprobe-events to record module symbol")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/trace/trace_probe.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 3ef15a6683c0..bd30e9398d2a 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -535,7 +535,7 @@ int traceprobe_update_arg(struct probe_arg *arg)
 			if (code[1].op != FETCH_OP_IMM)
 				return -EINVAL;
 
-			tmp = strpbrk("+-", code->data);
+			tmp = strpbrk(code->data, "+-");
 			if (tmp)
 				c = *tmp;
 			ret = traceprobe_split_symbol_offset(code->data,

Re: [BUGFIX PATCH] tracing/kprobes: Fix strpbrk() argument order

[Steven Rostedt]

On Thu,  1 Nov 2018 23:29:28 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Fix strpbrk()'s argument order, it must pass acceptable string
> in 2nd argument. Note that this can cause a kernel panic where
> it recovers backup character to code->data.
> 
> Fixes: a6682814f371 ("tracing/kprobes: Allow kprobe-events to record module symbol")
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks Masami,

I'm pulling this and starting to test it.

-- Steve

> ---
>  kernel/trace/trace_probe.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 3ef15a6683c0..bd30e9398d2a 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -535,7 +535,7 @@ int traceprobe_update_arg(struct probe_arg *arg)
>  			if (code[1].op != FETCH_OP_IMM)
>  				return -EINVAL;
>  
> -			tmp = strpbrk("+-", code->data);
> +			tmp = strpbrk(code->data, "+-");
>  			if (tmp)
>  				c = *tmp;
>  			ret = traceprobe_split_symbol_offset(code->data,

Re: [BUGFIX PATCH] tracing/kprobes: Fix strpbrk() argument order

[Masami Hiramatsu]

On Thu, 1 Nov 2018 15:10:14 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu,  1 Nov 2018 23:29:28 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Fix strpbrk()'s argument order, it must pass acceptable string
> > in 2nd argument. Note that this can cause a kernel panic where
> > it recovers backup character to code->data.
> > 
> > Fixes: a6682814f371 ("tracing/kprobes: Allow kprobe-events to record module symbol")
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> 
> Thanks Masami,
> 
> I'm pulling this and starting to test it.

Thank you! I still couldn't believe how this bug passed through the tests...

> 
> -- Steve
> 
> > ---
> >  kernel/trace/trace_probe.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > index 3ef15a6683c0..bd30e9398d2a 100644
> > --- a/kernel/trace/trace_probe.c
> > +++ b/kernel/trace/trace_probe.c
> > @@ -535,7 +535,7 @@ int traceprobe_update_arg(struct probe_arg *arg)
> >  			if (code[1].op != FETCH_OP_IMM)
> >  				return -EINVAL;
> >  
> > -			tmp = strpbrk("+-", code->data);
> > +			tmp = strpbrk(code->data, "+-");
> >  			if (tmp)
> >  				c = *tmp;
> >  			ret = traceprobe_split_symbol_offset(code->data,
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [BUGFIX PATCH] tracing/kprobes: Fix strpbrk() argument order

[Steven Rostedt]

On Fri, 2 Nov 2018 16:14:59 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Thu, 1 Nov 2018 15:10:14 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Thu,  1 Nov 2018 23:29:28 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >   
> > > Fix strpbrk()'s argument order, it must pass acceptable string
> > > in 2nd argument. Note that this can cause a kernel panic where
> > > it recovers backup character to code->data.
> > > 
> > > Fixes: a6682814f371 ("tracing/kprobes: Allow kprobe-events to record module symbol")
> > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>  
> > 
> > Thanks Masami,
> > 
> > I'm pulling this and starting to test it.  
> 
> Thank you! I still couldn't believe how this bug passed through the tests...

I am too. I'm running tests with and without this patch, and the patch
doesn't appear to be making much difference.

Then I tested with this:

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 3ef15a6683c0..4ddafddf1343 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -516,6 +516,7 @@ void traceprobe_free_probe_arg(struct probe_arg
*arg) kfree(code->data);
 		code++;
 	}
+	printk("free arg->code %s\n", arg->code);
 	kfree(arg->code);
 	kfree(arg->name);
 	kfree(arg->comm);
@@ -535,11 +536,15 @@ int traceprobe_update_arg(struct probe_arg *arg)
 			if (code[1].op != FETCH_OP_IMM)
 				return -EINVAL;
 
+			tmp = strpbrk(code->data, "+-");
+			printk("first tmp tmp=%s\n", tmp);
 			tmp = strpbrk("+-", code->data);
+			printk("second tmp=%s data=%s\n", tmp,
code->data); if (tmp)
 				c = *tmp;
 			ret =
traceprobe_split_symbol_offset(code->data, &offset);
+			printk("third data=%s\n", code->data);
 			if (ret)
 				return ret;
 
@@ -547,6 +552,7 @@ int traceprobe_update_arg(struct probe_arg *arg)
 				(unsigned
long)kallsyms_lookup_name(code->data); if (tmp)
 				*tmp = c;
+			printk("forth data=%s\n", code->data);
 			if (!code[1].immediate)
 				return -ENOENT;
 			code[1].immediate += offset;

And I don't see where that code->data is used elsewhere. That is, why
even bother saving the character?

-- Steve

Re: [BUGFIX PATCH] tracing/kprobes: Fix strpbrk() argument order

[Masami Hiramatsu]

On Fri, 2 Nov 2018 09:54:24 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 2 Nov 2018 16:14:59 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > On Thu, 1 Nov 2018 15:10:14 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > On Thu,  1 Nov 2018 23:29:28 +0900
> > > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >   
> > > > Fix strpbrk()'s argument order, it must pass acceptable string
> > > > in 2nd argument. Note that this can cause a kernel panic where
> > > > it recovers backup character to code->data.
> > > > 
> > > > Fixes: a6682814f371 ("tracing/kprobes: Allow kprobe-events to record module symbol")
> > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>  
> > > 
> > > Thanks Masami,
> > > 
> > > I'm pulling this and starting to test it.  
> > 
> > Thank you! I still couldn't believe how this bug passed through the tests...
> 
> I am too. I'm running tests with and without this patch, and the patch
> doesn't appear to be making much difference.

Maybe traceprobe_free_probe_arg() is silently failed.

> 
> Then I tested with this:
> 
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 3ef15a6683c0..4ddafddf1343 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -516,6 +516,7 @@ void traceprobe_free_probe_arg(struct probe_arg
> *arg) kfree(code->data);
>  		code++;
>  	}
> +	printk("free arg->code %s\n", arg->code);
>  	kfree(arg->code);
>  	kfree(arg->name);
>  	kfree(arg->comm);
> @@ -535,11 +536,15 @@ int traceprobe_update_arg(struct probe_arg *arg)
>  			if (code[1].op != FETCH_OP_IMM)
>  				return -EINVAL;
>  
> +			tmp = strpbrk(code->data, "+-");
> +			printk("first tmp tmp=%s\n", tmp);
>  			tmp = strpbrk("+-", code->data);
> +			printk("second tmp=%s data=%s\n", tmp,
> code->data); if (tmp)
>  				c = *tmp;
>  			ret =
> traceprobe_split_symbol_offset(code->data, &offset);
> +			printk("third data=%s\n", code->data);
>  			if (ret)
>  				return ret;
>  
> @@ -547,6 +552,7 @@ int traceprobe_update_arg(struct probe_arg *arg)
>  				(unsigned
> long)kallsyms_lookup_name(code->data); if (tmp)
>  				*tmp = c;
> +			printk("forth data=%s\n", code->data);
>  			if (!code[1].immediate)
>  				return -ENOENT;
>  			code[1].immediate += offset;
> 
> And I don't see where that code->data is used elsewhere. That is, why
> even bother saving the character?

Would you mean parsing the symbol+offs every time is useless?
It needs to solve the symbol address always because  traceprobe_update_arg
is called when new symbols added on the kernel (by loading modules).

Hmm, maybe I can introduce a struct like 

struct symbol_offset {
	long offset;
	char symbol[];
};

and use it instead of parsing the symbol+offset always.

Thanks,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [BUGFIX PATCH] tracing/kprobes: Fix strpbrk() argument order

[Steven Rostedt]

On Sat, 3 Nov 2018 20:54:48 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Fri, 2 Nov 2018 09:54:24 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Fri, 2 Nov 2018 16:14:59 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >   
> > > On Thu, 1 Nov 2018 15:10:14 -0400
> > > Steven Rostedt <rostedt@goodmis.org> wrote:
> > >   
> > > > On Thu,  1 Nov 2018 23:29:28 +0900
> > > > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > >     
> > > > > Fix strpbrk()'s argument order, it must pass acceptable string
> > > > > in 2nd argument. Note that this can cause a kernel panic where
> > > > > it recovers backup character to code->data.
> > > > > 
> > > > > Fixes: a6682814f371 ("tracing/kprobes: Allow kprobe-events to record module symbol")
> > > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>    
> > > > 
> > > > Thanks Masami,
> > > > 
> > > > I'm pulling this and starting to test it.    
> > > 
> > > Thank you! I still couldn't believe how this bug passed through the tests...  
> > 
> > I am too. I'm running tests with and without this patch, and the patch
> > doesn't appear to be making much difference.  
> 
> Maybe traceprobe_free_probe_arg() is silently failed.

I don't see how.

> 
> > 
> > Then I tested with this:
> > 
> > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > index 3ef15a6683c0..4ddafddf1343 100644
> > --- a/kernel/trace/trace_probe.c
> > +++ b/kernel/trace/trace_probe.c
> > @@ -516,6 +516,7 @@ void traceprobe_free_probe_arg(struct probe_arg
> > *arg) kfree(code->data);
> >  		code++;
> >  	}
> > +	printk("free arg->code %s\n", arg->code);
> >  	kfree(arg->code);
> >  	kfree(arg->name);
> >  	kfree(arg->comm);
> > @@ -535,11 +536,15 @@ int traceprobe_update_arg(struct probe_arg *arg)
> >  			if (code[1].op != FETCH_OP_IMM)
> >  				return -EINVAL;
> >  
> > +			tmp = strpbrk(code->data, "+-");
> > +			printk("first tmp tmp=%s\n", tmp);
> >  			tmp = strpbrk("+-", code->data);
> > +			printk("second tmp=%s data=%s\n", tmp,
> > code->data); if (tmp)
> >  				c = *tmp;
> >  			ret =
> > traceprobe_split_symbol_offset(code->data, &offset);
> > +			printk("third data=%s\n", code->data);
> >  			if (ret)
> >  				return ret;
> >  
> > @@ -547,6 +552,7 @@ int traceprobe_update_arg(struct probe_arg *arg)
> >  				(unsigned
> > long)kallsyms_lookup_name(code->data); if (tmp)
> >  				*tmp = c;
> > +			printk("forth data=%s\n", code->data);
> >  			if (!code[1].immediate)
> >  				return -ENOENT;
> >  			code[1].immediate += offset;
> > 
> > And I don't see where that code->data is used elsewhere. That is, why
> > even bother saving the character?  
> 
> Would you mean parsing the symbol+offs every time is useless?
> It needs to solve the symbol address always because  traceprobe_update_arg
> is called when new symbols added on the kernel (by loading modules).

OK, so it is called multiple times? That is when a module is loaded?
Because I couldn't get this called multiple times.

I'll try that out and if that's the case, then yeah, this needs to be
fixed (otherwise, I was thinking we could just remove the strpbrk()
altogether).


> 
> Hmm, maybe I can introduce a struct like 
> 
> struct symbol_offset {
> 	long offset;
> 	char symbol[];
> };
> 
> and use it instead of parsing the symbol+offset always.

The parsing should be fine. The issue I had was that I couldn't trigger
it to happen more than once. That's why this passed testing. We didn't
do something that required it to be called more than once and that is
here the bug would trigger.

-- Steve

Re: [BUGFIX PATCH] tracing/kprobes: Fix strpbrk() argument order

[Masami Hiramatsu]

On Sat, 3 Nov 2018 09:17:54 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:


> > > Then I tested with this:
> > > 
> > > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > > index 3ef15a6683c0..4ddafddf1343 100644
> > > --- a/kernel/trace/trace_probe.c
> > > +++ b/kernel/trace/trace_probe.c
> > > @@ -516,6 +516,7 @@ void traceprobe_free_probe_arg(struct probe_arg
> > > *arg) kfree(code->data);
> > >  		code++;
> > >  	}
> > > +	printk("free arg->code %s\n", arg->code);
> > >  	kfree(arg->code);
> > >  	kfree(arg->name);
> > >  	kfree(arg->comm);
> > > @@ -535,11 +536,15 @@ int traceprobe_update_arg(struct probe_arg *arg)
> > >  			if (code[1].op != FETCH_OP_IMM)
> > >  				return -EINVAL;
> > >  
> > > +			tmp = strpbrk(code->data, "+-");
> > > +			printk("first tmp tmp=%s\n", tmp);
> > >  			tmp = strpbrk("+-", code->data);
> > > +			printk("second tmp=%s data=%s\n", tmp,
> > > code->data); if (tmp)
> > >  				c = *tmp;
> > >  			ret =
> > > traceprobe_split_symbol_offset(code->data, &offset);
> > > +			printk("third data=%s\n", code->data);
> > >  			if (ret)
> > >  				return ret;
> > >  
> > > @@ -547,6 +552,7 @@ int traceprobe_update_arg(struct probe_arg *arg)
> > >  				(unsigned
> > > long)kallsyms_lookup_name(code->data); if (tmp)
> > >  				*tmp = c;
> > > +			printk("forth data=%s\n", code->data);
> > >  			if (!code[1].immediate)
> > >  				return -ENOENT;
> > >  			code[1].immediate += offset;
> > > 
> > > And I don't see where that code->data is used elsewhere. That is, why
> > > even bother saving the character?  
> > 
> > Would you mean parsing the symbol+offs every time is useless?
> > It needs to solve the symbol address always because  traceprobe_update_arg
> > is called when new symbols added on the kernel (by loading modules).
> 
> OK, so it is called multiple times? That is when a module is loaded?
> Because I couldn't get this called multiple times.

Sorry I mislead you.
See trace_kprobe_module_callback(), which calls __register_trace_kprobe()
if the probepoint is on the module. And __register_trace_kprobe() calls
traceprobe_update_arg().
So, if you call it multiple time, it should be with the probe point is
on the module too.

e.g.

 echo "p newmod:function @var_symbol+offset" > kprobe_events

can be called multiple times if we load/unload "newmod" module.

> I'll try that out and if that's the case, then yeah, this needs to be
> fixed (otherwise, I was thinking we could just remove the strpbrk()
> altogether).
> 
> 
> > 
> > Hmm, maybe I can introduce a struct like 
> > 
> > struct symbol_offset {
> > 	long offset;
> > 	char symbol[];
> > };
> > 
> > and use it instead of parsing the symbol+offset always.
> 
> The parsing should be fine. The issue I had was that I couldn't trigger
> it to happen more than once. That's why this passed testing. We didn't
> do something that required it to be called more than once and that is
> here the bug would trigger.

Hmm, I hit it by kprobe_args_syntax.tc, so hit once is enough to kick
this bug. Maybe some config option makes "+-" readonly, which previously
didn't enabled.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

[RFC PATCH] tracing/kprobes: Avoid parsing symbol+offset when updating arguments

[Masami Hiramatsu]

Introduce symbol_offset data structure for avoiding symbol+offset
parsing when updating arguments.

For kprobe events, "@symbol+offset" is supported, that requires
to be updated when new module is loaded because @symbol address
can be solved at that point. Currently kprobe events saves
the "symbol+offset" string and parse it repeatedly, but that
is inefficient.
This introduces symbol_offset data structure which can save the
offset and symbol separated, so that we don't need to parse it
anymore.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/trace/trace_probe.c |   49 +++++++++++++++++++++++++-------------------
 kernel/trace/trace_probe.h |    7 +++++-
 2 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index bd30e9398d2a..0a3af7d6e133 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -201,6 +201,26 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
 	return ret;
 }
 
+static struct symbol_offset *allocate_symbol_offset(char *sym_offs_str)
+{
+	int ret;
+	long offset = 0;
+	struct symbol_offset *sof;
+
+	ret = traceprobe_split_symbol_offset(sym_offs_str, &offset);
+	if (ret)
+		return ERR_PTR(ret);
+
+	sof = kzalloc(sizeof(struct symbol_offset) + strlen(sym_offs_str) + 1,
+			GFP_KERNEL);
+	if (!sof)
+		return ERR_PTR(-ENOMEM);
+
+	sof->offset = offset;
+	strcpy(sof->symbol, sym_offs_str);
+	return sof;
+}
+
 /* Recursive argument parser */
 static int
 parse_probe_arg(char *arg, const struct fetch_type *type,
@@ -253,9 +273,9 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
 
 			/* Preserve symbol for updating */
 			code->op = FETCH_NOP_SYMBOL;
-			code->data = kstrdup(arg + 1, GFP_KERNEL);
-			if (!code->data)
-				return -ENOMEM;
+			code->symoffs = allocate_symbol_offset(arg + 1);
+			if (IS_ERR(code->symoffs))
+				return PTR_ERR(code->symoffs);
 			if (++code == end)
 				return -E2BIG;
 
@@ -483,7 +503,7 @@ int traceprobe_parse_probe_arg(char *arg, ssize_t *size,
 	if (ret) {
 		for (code = tmp; code < tmp + FETCH_INSN_MAX; code++)
 			if (code->op == FETCH_NOP_SYMBOL)
-				kfree(code->data);
+				kfree(code->symoffs);
 	}
 	kfree(tmp);
 
@@ -513,7 +533,7 @@ void traceprobe_free_probe_arg(struct probe_arg *arg)
 
 	while (code && code->op != FETCH_OP_END) {
 		if (code->op == FETCH_NOP_SYMBOL)
-			kfree(code->data);
+			kfree(code->symoffs);
 		code++;
 	}
 	kfree(arg->code);
@@ -525,31 +545,18 @@ void traceprobe_free_probe_arg(struct probe_arg *arg)
 int traceprobe_update_arg(struct probe_arg *arg)
 {
 	struct fetch_insn *code = arg->code;
-	long offset;
-	char *tmp;
-	char c;
-	int ret = 0;
 
 	while (code && code->op != FETCH_OP_END) {
 		if (code->op == FETCH_NOP_SYMBOL) {
 			if (code[1].op != FETCH_OP_IMM)
 				return -EINVAL;
 
-			tmp = strpbrk(code->data, "+-");
-			if (tmp)
-				c = *tmp;
-			ret = traceprobe_split_symbol_offset(code->data,
-							     &offset);
-			if (ret)
-				return ret;
-
 			code[1].immediate =
-				(unsigned long)kallsyms_lookup_name(code->data);
-			if (tmp)
-				*tmp = c;
+				(unsigned long)kallsyms_lookup_name(
+						code->symoffs->symbol);
 			if (!code[1].immediate)
 				return -ENOENT;
-			code[1].immediate += offset;
+			code[1].immediate += code->symoffs->offset;
 		}
 		code++;
 	}
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 974afc1a3e73..942424d05427 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -103,6 +103,11 @@ enum fetch_op {
 	FETCH_NOP_SYMBOL,	/* Unresolved Symbol holder */
 };
 
+struct symbol_offset {
+	long offset;
+	char symbol[];
+};
+
 struct fetch_insn {
 	enum fetch_op op;
 	union {
@@ -117,7 +122,7 @@ struct fetch_insn {
 			unsigned char rshift;
 		};
 		unsigned long immediate;
-		void *data;
+		struct symbol_offset *symoffs;
 	};
 };
 

Re: [RFC PATCH] tracing/kprobes: Avoid parsing symbol+offset when updating arguments

[Steven Rostedt]

On Sun,  4 Nov 2018 01:03:34 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Introduce symbol_offset data structure for avoiding symbol+offset
> parsing when updating arguments.
> 
> For kprobe events, "@symbol+offset" is supported, that requires
> to be updated when new module is loaded because @symbol address
> can be solved at that point. Currently kprobe events saves
> the "symbol+offset" string and parse it repeatedly, but that
> is inefficient.

Do we care if it's inefficient? It's only done on module load and at
the beginning. That's far from a fast path.

If anything, the original solution saves memory, which is a bigger
concern than speed in this case.

-- Steve


> This introduces symbol_offset data structure which can save the
> offset and symbol separated, so that we don't need to parse it
> anymore.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_probe.c |   49 +++++++++++++++++++++++++-------------------
>  kernel/trace/trace_probe.h |    7 +++++-
>  2 files changed, 34 insertions(+), 22 deletions(-)
> 
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index bd30e9398d2a..0a3af7d6e133 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -201,6 +201,26 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
>  	return ret;
>  }
>  
> +static struct symbol_offset *allocate_symbol_offset(char *sym_offs_str)
> +{
> +	int ret;
> +	long offset = 0;
> +	struct symbol_offset *sof;
> +
> +	ret = traceprobe_split_symbol_offset(sym_offs_str, &offset);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	sof = kzalloc(sizeof(struct symbol_offset) + strlen(sym_offs_str) + 1,
> +			GFP_KERNEL);
> +	if (!sof)
> +		return ERR_PTR(-ENOMEM);
> +
> +	sof->offset = offset;
> +	strcpy(sof->symbol, sym_offs_str);
> +	return sof;
> +}
> +
>  /* Recursive argument parser */
>  static int
>  parse_probe_arg(char *arg, const struct fetch_type *type,
> @@ -253,9 +273,9 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
>  
>  			/* Preserve symbol for updating */
>  			code->op = FETCH_NOP_SYMBOL;
> -			code->data = kstrdup(arg + 1, GFP_KERNEL);
> -			if (!code->data)
> -				return -ENOMEM;
> +			code->symoffs = allocate_symbol_offset(arg + 1);
> +			if (IS_ERR(code->symoffs))
> +				return PTR_ERR(code->symoffs);
>  			if (++code == end)
>  				return -E2BIG;
>  
> @@ -483,7 +503,7 @@ int traceprobe_parse_probe_arg(char *arg, ssize_t *size,
>  	if (ret) {
>  		for (code = tmp; code < tmp + FETCH_INSN_MAX; code++)
>  			if (code->op == FETCH_NOP_SYMBOL)
> -				kfree(code->data);
> +				kfree(code->symoffs);
>  	}
>  	kfree(tmp);
>  
> @@ -513,7 +533,7 @@ void traceprobe_free_probe_arg(struct probe_arg *arg)
>  
>  	while (code && code->op != FETCH_OP_END) {
>  		if (code->op == FETCH_NOP_SYMBOL)
> -			kfree(code->data);
> +			kfree(code->symoffs);
>  		code++;
>  	}
>  	kfree(arg->code);
> @@ -525,31 +545,18 @@ void traceprobe_free_probe_arg(struct probe_arg *arg)
>  int traceprobe_update_arg(struct probe_arg *arg)
>  {
>  	struct fetch_insn *code = arg->code;
> -	long offset;
> -	char *tmp;
> -	char c;
> -	int ret = 0;
>  
>  	while (code && code->op != FETCH_OP_END) {
>  		if (code->op == FETCH_NOP_SYMBOL) {
>  			if (code[1].op != FETCH_OP_IMM)
>  				return -EINVAL;
>  
> -			tmp = strpbrk(code->data, "+-");
> -			if (tmp)
> -				c = *tmp;
> -			ret = traceprobe_split_symbol_offset(code->data,
> -							     &offset);
> -			if (ret)
> -				return ret;
> -
>  			code[1].immediate =
> -				(unsigned long)kallsyms_lookup_name(code->data);
> -			if (tmp)
> -				*tmp = c;
> +				(unsigned long)kallsyms_lookup_name(
> +						code->symoffs->symbol);
>  			if (!code[1].immediate)
>  				return -ENOENT;
> -			code[1].immediate += offset;
> +			code[1].immediate += code->symoffs->offset;
>  		}
>  		code++;
>  	}
> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 974afc1a3e73..942424d05427 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -103,6 +103,11 @@ enum fetch_op {
>  	FETCH_NOP_SYMBOL,	/* Unresolved Symbol holder */
>  };
>  
> +struct symbol_offset {
> +	long offset;
> +	char symbol[];
> +};
> +
>  struct fetch_insn {
>  	enum fetch_op op;
>  	union {
> @@ -117,7 +122,7 @@ struct fetch_insn {
>  			unsigned char rshift;
>  		};
>  		unsigned long immediate;
> -		void *data;
> +		struct symbol_offset *symoffs;
>  	};
>  };
>  

Re: [RFC PATCH] tracing/kprobes: Avoid parsing symbol+offset when updating arguments

[Masami Hiramatsu]

On Sat, 3 Nov 2018 13:43:16 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sun,  4 Nov 2018 01:03:34 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Introduce symbol_offset data structure for avoiding symbol+offset
> > parsing when updating arguments.
> > 
> > For kprobe events, "@symbol+offset" is supported, that requires
> > to be updated when new module is loaded because @symbol address
> > can be solved at that point. Currently kprobe events saves
> > the "symbol+offset" string and parse it repeatedly, but that
> > is inefficient.
> 
> Do we care if it's inefficient? It's only done on module load and at
> the beginning. That's far from a fast path.
> 
> If anything, the original solution saves memory, which is a bigger
> concern than speed in this case.

OK, and original one saves lines too. Please drop it :)

Thank you,

> 
> -- Steve
> 
> 
> > This introduces symbol_offset data structure which can save the
> > offset and symbol separated, so that we don't need to parse it
> > anymore.
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  kernel/trace/trace_probe.c |   49 +++++++++++++++++++++++++-------------------
> >  kernel/trace/trace_probe.h |    7 +++++-
> >  2 files changed, 34 insertions(+), 22 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > index bd30e9398d2a..0a3af7d6e133 100644
> > --- a/kernel/trace/trace_probe.c
> > +++ b/kernel/trace/trace_probe.c
> > @@ -201,6 +201,26 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
> >  	return ret;
> >  }
> >  
> > +static struct symbol_offset *allocate_symbol_offset(char *sym_offs_str)
> > +{
> > +	int ret;
> > +	long offset = 0;
> > +	struct symbol_offset *sof;
> > +
> > +	ret = traceprobe_split_symbol_offset(sym_offs_str, &offset);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	sof = kzalloc(sizeof(struct symbol_offset) + strlen(sym_offs_str) + 1,
> > +			GFP_KERNEL);
> > +	if (!sof)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	sof->offset = offset;
> > +	strcpy(sof->symbol, sym_offs_str);
> > +	return sof;
> > +}
> > +
> >  /* Recursive argument parser */
> >  static int
> >  parse_probe_arg(char *arg, const struct fetch_type *type,
> > @@ -253,9 +273,9 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
> >  
> >  			/* Preserve symbol for updating */
> >  			code->op = FETCH_NOP_SYMBOL;
> > -			code->data = kstrdup(arg + 1, GFP_KERNEL);
> > -			if (!code->data)
> > -				return -ENOMEM;
> > +			code->symoffs = allocate_symbol_offset(arg + 1);
> > +			if (IS_ERR(code->symoffs))
> > +				return PTR_ERR(code->symoffs);
> >  			if (++code == end)
> >  				return -E2BIG;
> >  
> > @@ -483,7 +503,7 @@ int traceprobe_parse_probe_arg(char *arg, ssize_t *size,
> >  	if (ret) {
> >  		for (code = tmp; code < tmp + FETCH_INSN_MAX; code++)
> >  			if (code->op == FETCH_NOP_SYMBOL)
> > -				kfree(code->data);
> > +				kfree(code->symoffs);
> >  	}
> >  	kfree(tmp);
> >  
> > @@ -513,7 +533,7 @@ void traceprobe_free_probe_arg(struct probe_arg *arg)
> >  
> >  	while (code && code->op != FETCH_OP_END) {
> >  		if (code->op == FETCH_NOP_SYMBOL)
> > -			kfree(code->data);
> > +			kfree(code->symoffs);
> >  		code++;
> >  	}
> >  	kfree(arg->code);
> > @@ -525,31 +545,18 @@ void traceprobe_free_probe_arg(struct probe_arg *arg)
> >  int traceprobe_update_arg(struct probe_arg *arg)
> >  {
> >  	struct fetch_insn *code = arg->code;
> > -	long offset;
> > -	char *tmp;
> > -	char c;
> > -	int ret = 0;
> >  
> >  	while (code && code->op != FETCH_OP_END) {
> >  		if (code->op == FETCH_NOP_SYMBOL) {
> >  			if (code[1].op != FETCH_OP_IMM)
> >  				return -EINVAL;
> >  
> > -			tmp = strpbrk(code->data, "+-");
> > -			if (tmp)
> > -				c = *tmp;
> > -			ret = traceprobe_split_symbol_offset(code->data,
> > -							     &offset);
> > -			if (ret)
> > -				return ret;
> > -
> >  			code[1].immediate =
> > -				(unsigned long)kallsyms_lookup_name(code->data);
> > -			if (tmp)
> > -				*tmp = c;
> > +				(unsigned long)kallsyms_lookup_name(
> > +						code->symoffs->symbol);
> >  			if (!code[1].immediate)
> >  				return -ENOENT;
> > -			code[1].immediate += offset;
> > +			code[1].immediate += code->symoffs->offset;
> >  		}
> >  		code++;
> >  	}
> > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> > index 974afc1a3e73..942424d05427 100644
> > --- a/kernel/trace/trace_probe.h
> > +++ b/kernel/trace/trace_probe.h
> > @@ -103,6 +103,11 @@ enum fetch_op {
> >  	FETCH_NOP_SYMBOL,	/* Unresolved Symbol holder */
> >  };
> >  
> > +struct symbol_offset {
> > +	long offset;
> > +	char symbol[];
> > +};
> > +
> >  struct fetch_insn {
> >  	enum fetch_op op;
> >  	union {
> > @@ -117,7 +122,7 @@ struct fetch_insn {
> >  			unsigned char rshift;
> >  		};
> >  		unsigned long immediate;
> > -		void *data;
> > +		struct symbol_offset *symoffs;
> >  	};
> >  };
> >  
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>