tracing/kprobes: Fix strpbrk() argument order

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 <[email protected]>

---
 kernel/trace/trace_probe.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/trace/trace_probe.c +1 -1

index 3ef15a6683c0..bd30e9398d2a 100644
@@ -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,

Quoted 4 lines · On Thu, 1 Nov 2018 23:29:28 +0900 Masami Hiramatsu <[email protected]> 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.

Quoted 2 lines

Fixes a6682814f371 ("tracing/kprobes: Allow kprobe-events to record module symbol")

Signed-off-by Masami Hiramatsu <[email protected]>

Thanks Masami,

I'm pulling this and starting to test it.

-- Steve

Quoted 17 lines

---
 kernel/trace/trace_probe.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/trace/trace_probe.c +1 -1

index 3ef15a6683c0..bd30e9398d2a 100644
@@ -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,

Quoted 7 lines · On Thu, 1 Nov 2018 15:10:14 -0400 Steven Rostedt <[email protected]> wrote:

Quoted 4 lines · On Thu, 1 Nov 2018 23:29:28 +0900 Masami Hiramatsu <[email protected]> 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.

Quoted 6 lines

Quoted 2 lines

Fixes a6682814f371 ("tracing/kprobes: Allow kprobe-events to record module symbol")

Signed-off-by Masami Hiramatsu <[email protected]>

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

Quoted 13 lines · On Fri, 2 Nov 2018 16:14:59 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 10 lines · On Thu, 1 Nov 2018 15:10:14 -0400 Steven Rostedt <[email protected]> wrote:

Quoted 6 lines · On Thu, 1 Nov 2018 23:29:28 +0900 Masami Hiramatsu <[email protected]> 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 <[email protected]>

Quoted 5 lines

Quoted 3 lines

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:

kernel/trace/trace_probe.c +6 -0

index 3ef15a6683c0..4ddafddf1343 100644
@@ -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?

Quoted 18 lines · On Fri, 2 Nov 2018 09:54:24 -0400 Steven Rostedt <[email protected]> wrote:

Quoted 15 lines · On Fri, 2 Nov 2018 16:14:59 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 12 lines · On Thu, 1 Nov 2018 15:10:14 -0400 Steven Rostedt <[email protected]> wrote:

Quoted 6 lines · On Thu, 1 Nov 2018 23:29:28 +0900 Masami Hiramatsu <[email protected]> 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 <[email protected]>

Thanks Masami,

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

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.

Quoted 41 lines

Then I tested with this:

kernel/trace/trace_probe.c +6 -0

index 3ef15a6683c0..4ddafddf1343 100644
@@ -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;

Quoted 2 lines

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.

Quoted 39 lines

Quoted 39 lines

Then I tested with this:

kernel/trace/trace_probe.c +6 -0

index 3ef15a6683c0..4ddafddf1343 100644
@@ -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;

Quoted 6 lines

Quoted 2 lines

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

Quoted 8 lines

Hmm, maybe I can introduce a struct like

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

Quoted 1 line

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.

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 <[email protected]>

---
 kernel/trace/trace_probe.c |   49 +++++++++++++++++++++++++-------------------
 kernel/trace/trace_probe.h |    7 +++++-
 2 files changed, 34 insertions(+), 22 deletions(-)

kernel/trace/trace_probe.c +28 -21

index bd30e9398d2a..0a3af7d6e133 100644
@@ -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++;
 	}

kernel/trace/trace_probe.h +6 -1

index 974afc1a3e73..942424d05427 100644
@@ -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;
 	};
 };
 

Quoted 8 lines · On Sun, 4 Nov 2018 01:03:34 +0900 Masami Hiramatsu <[email protected]> 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

Quoted 132 lines

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 <[email protected]>

---
 kernel/trace/trace_probe.c |   49 +++++++++++++++++++++++++-------------------
 kernel/trace/trace_probe.h |    7 +++++-
 2 files changed, 34 insertions(+), 22 deletions(-)

kernel/trace/trace_probe.c +28 -21

index bd30e9398d2a..0a3af7d6e133 100644
@@ -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++;
 	}

kernel/trace/trace_probe.h +6 -1

index 974afc1a3e73..942424d05427 100644
@@ -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;
 	};
 };