[ftrace/kprobes PATCH 0/3] tracing: probeevent: Fix module symbol probing

[ftrace/kprobes PATCH 0/3] tracing: probeevent: Fix module symbol probing

[Masami Hiramatsu]

Hi,

This series is for fixing some bugs in Steve's ftrace/kprobes branch.

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git

Which is based on my fetcharg improvement series.

https://lkml.org/lkml/2018/4/25/601

This includes following fixes.

- Fix copy_from_user() misusing which Dan was reported.
- Fix to reject incorrect probeevent on loaded kernel module.
- Fix to update symbol-based argument on module.
  This also checks the symbol-based argument is correct or not when
  target module is loaded. If it is not correct, the event is kept
  unavailable.

Thank you,

---

Masami Hiramatsu (3):
      tracing/uprobes: Fix to return -EFAULT if copy_from_user failed
      tracing/kprobes: Check the probe on unloaded module correctly
      tracing/kprobes: Allow kprobe-events to record module symbol


 kernel/trace/trace_kprobe.c |   51 ++++++++++++++++++++++++++---------
 kernel/trace/trace_probe.c  |   62 +++++++++++++++++++++++++++++++++++++------
 kernel/trace/trace_probe.h  |    4 ++-
 kernel/trace/trace_uprobe.c |    2 +
 4 files changed, 95 insertions(+), 24 deletions(-)

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[ftrace/kprobes PATCH 1/3] tracing/uprobes: Fix to return -EFAULT if copy_from_user failed

[Masami Hiramatsu]

Fix probe_mem_read() to return -EFAULT if copy_from_user()
failed. The copy_from_user() returns remaining bytes
when it failed, but probe_mem_read() caller expects it
returns error code like as probe_kernel_read().

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/trace/trace_uprobe.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 12bdbdf772ed..63e99ee716fe 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -104,7 +104,7 @@ probe_mem_read(void *dest, void *src, size_t size)
 {
 	void __user *vaddr = (void __force __user *)src;
 
-	return copy_from_user(dest, vaddr, size);
+	return copy_from_user(dest, vaddr, size) ? -EFAULT : 0;
 }
 /*
  * Fetch a null-terminated string. Caller MUST set *(u32 *)dest with max

[ftrace/kprobes PATCH 2/3] tracing/kprobes: Check the probe on unloaded module correctly

[Masami Hiramatsu]

Current kprobe event doesn't checks correctly whether the
given event is on unloaded module or not. It just checks
the event has ":" in the name.

That is not enough because if we define a probe on non-exist
symbol on loaded module, it allows to define that (with
warning message)

To ensure it correctly, this searches the module name on
loaded module list and only if there is not, it allows to
define it. (this event will be available when the target
module is loaded)

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/trace/trace_kprobe.c |   39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index b006aaeceb92..fbf609cbeac2 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -62,9 +62,23 @@ static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
 	return strncmp(mod->name, name, len) == 0 && name[len] == ':';
 }
 
-static nokprobe_inline bool trace_kprobe_is_on_module(struct trace_kprobe *tk)
+static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
 {
-	return !!strchr(trace_kprobe_symbol(tk), ':');
+	char *p;
+	bool ret;
+
+	if (!tk->symbol)
+		return false;
+	p = strchr(tk->symbol, ':');
+	if (!p)
+		return true;
+	*p = '\0';
+	mutex_lock(&module_mutex);
+	ret = !!find_module(tk->symbol);
+	mutex_unlock(&module_mutex);
+	*p = ':';
+
+	return ret;
 }
 
 static nokprobe_inline unsigned long trace_kprobe_nhit(struct trace_kprobe *tk)
@@ -374,19 +388,13 @@ static int __register_trace_kprobe(struct trace_kprobe *tk)
 	else
 		ret = register_kprobe(&tk->rp.kp);
 
-	if (ret == 0)
+	if (ret == 0) {
 		tk->tp.flags |= TP_FLAG_REGISTERED;
-	else {
-		if (ret == -ENOENT && trace_kprobe_is_on_module(tk)) {
-			pr_warn("This probe might be able to register after target module is loaded. Continue.\n");
-			ret = 0;
-		} else if (ret == -EILSEQ) {
-			pr_warn("Probing address(0x%p) is not an instruction boundary.\n",
-				tk->rp.kp.addr);
-			ret = -EINVAL;
-		}
+	} else if (ret == -EILSEQ) {
+		pr_warn("Probing address(0x%p) is not an instruction boundary.\n",
+			tk->rp.kp.addr);
+		ret = -EINVAL;
 	}
-
 	return ret;
 }
 
@@ -449,6 +457,11 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
 
 	/* Register k*probe */
 	ret = __register_trace_kprobe(tk);
+	if (ret == -ENOENT && !trace_kprobe_module_exist(tk)) {
+		pr_warn("This probe might be able to register after target module is loaded. Continue.\n");
+		ret = 0;
+	}
+
 	if (ret < 0)
 		unregister_kprobe_event(tk);
 	else

[ftrace/kprobes PATCH 3/3] tracing/kprobes: Allow kprobe-events to record module symbol

[Masami Hiramatsu]

Allow kprobe-events to record module symbols.

Since data symbols in a non-loaded module doesn't exist, it fails to
define such symbol as an argument of kprobe-event. But if the kprobe
event is defined on that module, we can defer to resolve the symbol
address.

Note that if given symbol is not found, the event is kept unavailable.
User can enable it but the event is not recorded.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/trace/trace_kprobe.c |   12 ++++++++
 kernel/trace/trace_probe.c  |   62 +++++++++++++++++++++++++++++++++++++------
 kernel/trace/trace_probe.h  |    4 ++-
 3 files changed, 68 insertions(+), 10 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index fbf609cbeac2..920985fae8c0 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -366,7 +366,7 @@ static bool within_notrace_func(struct trace_kprobe *tk)
 /* Internal register function - just handle k*probes and flags */
 static int __register_trace_kprobe(struct trace_kprobe *tk)
 {
-	int ret;
+	int i, ret;
 
 	if (trace_probe_is_registered(&tk->tp))
 		return -EINVAL;
@@ -377,6 +377,12 @@ static int __register_trace_kprobe(struct trace_kprobe *tk)
 		return -EINVAL;
 	}
 
+	for (i = 0; i < tk->tp.nr_args; i++) {
+		ret = traceprobe_update_arg(&tk->tp.args[i]);
+		if (ret)
+			return ret;
+	}
+
 	/* Set/clear disabled flag according to tp->flag */
 	if (trace_probe_is_enabled(&tk->tp))
 		tk->rp.kp.flags &= ~KPROBE_FLAG_DISABLED;
@@ -925,6 +931,7 @@ process_fetch_insn(struct fetch_insn *code, struct pt_regs *regs, void *dest,
 {
 	unsigned long val;
 
+retry:
 	/* 1st stage: get value from context */
 	switch (code->op) {
 	case FETCH_OP_REG:
@@ -950,6 +957,9 @@ process_fetch_insn(struct fetch_insn *code, struct pt_regs *regs, void *dest,
 		val = regs_get_kernel_argument(regs, code->param);
 		break;
 #endif
+	case FETCH_NOP_SYMBOL:	/* Ignore a place holder */
+		code++;
+		goto retry;
 	default:
 		return -EILSEQ;
 	}
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 333cda6d2633..5b3d573b3dcf 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -251,16 +251,16 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
 			if (!(flags & TPARG_FL_KERNEL))
 				return -EINVAL;
 
-			ret = traceprobe_split_symbol_offset(arg + 1, &offset);
-			if (ret)
-				break;
+			/* Preserve symbol for updating */
+			code->op = FETCH_NOP_SYMBOL;
+			code->data = kstrdup(arg + 1, GFP_KERNEL);
+			if (!code->data)
+				return -ENOMEM;
+			if (++code == end)
+				return -E2BIG;
 
 			code->op = FETCH_OP_IMM;
-			code->immediate =
-				(unsigned long)kallsyms_lookup_name(arg + 1);
-			if (!code->immediate)
-				return -ENOENT;
-			code->immediate += offset;
+			code->immediate = 0;
 		}
 		/* These are fetching from memory */
 		if (++code == end)
@@ -480,6 +480,11 @@ int traceprobe_parse_probe_arg(char *arg, ssize_t *size,
 		memcpy(parg->code, tmp, sizeof(*code) * (code - tmp + 1));
 
 fail:
+	if (ret) {
+		for (code = tmp; code < tmp + FETCH_INSN_MAX; code++)
+			if (code->op == FETCH_NOP_SYMBOL)
+				kfree(code->data);
+	}
 	kfree(tmp);
 
 	return ret;
@@ -504,12 +509,53 @@ int traceprobe_conflict_field_name(const char *name,
 
 void traceprobe_free_probe_arg(struct probe_arg *arg)
 {
+	struct fetch_insn *code = arg->code;
+
+	while (code && code->op != FETCH_OP_END) {
+		if (code->op == FETCH_NOP_SYMBOL)
+			kfree(code->data);
+		code++;
+	}
 	kfree(arg->code);
 	kfree(arg->name);
 	kfree(arg->comm);
 	kfree(arg->fmt);
 }
 
+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;
+			if (!code[1].immediate)
+				return -ENOENT;
+			code[1].immediate += offset;
+		}
+		code++;
+	}
+	return 0;
+}
+
 /* When len=0, we just calculate the needed length */
 #define LEN_OR_ZERO (len ? len - pos : 0)
 static int __set_print_fmt(struct trace_probe *tp, char *buf, int len,
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 613e7b823452..052adce1388e 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -100,6 +100,7 @@ enum fetch_op {
 	// Stage 5 (loop) op
 	FETCH_OP_LP_ARRAY,	/* Array: .param = loop count */
 	FETCH_OP_END,
+	FETCH_NOP_SYMBOL,	/* Unresolved Symbol holder */
 };
 
 struct fetch_insn {
@@ -116,6 +117,7 @@ struct fetch_insn {
 			unsigned char rshift;
 		};
 		unsigned long immediate;
+		void *data;
 	};
 };
 
@@ -276,7 +278,7 @@ extern int traceprobe_parse_probe_arg(char *arg, ssize_t *size,
 extern int traceprobe_conflict_field_name(const char *name,
 			       struct probe_arg *args, int narg);
 
-extern void traceprobe_update_arg(struct probe_arg *arg);
+extern int traceprobe_update_arg(struct probe_arg *arg);
 extern void traceprobe_free_probe_arg(struct probe_arg *arg);
 
 extern int traceprobe_split_symbol_offset(char *symbol, long *offset);

Re: [ftrace/kprobes PATCH 3/3] tracing/kprobes: Allow kprobe-events to record module symbol

[Steven Rostedt]

On Wed, 29 Aug 2018 01:18:43 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Allow kprobe-events to record module symbols.
> 
> Since data symbols in a non-loaded module doesn't exist, it fails to
> define such symbol as an argument of kprobe-event. But if the kprobe
> event is defined on that module, we can defer to resolve the symbol
> address.
> 
> Note that if given symbol is not found, the event is kept unavailable.
> User can enable it but the event is not recorded.
> 
>

Hi Masami,

The first two patches look like they should be applied now and marked
for stable, as they are fixes. This patch looks like an added feature,
and will go into the next merge window.

Is that what you think too?

Thanks!

-- Steve

Re: [ftrace/kprobes PATCH 3/3] tracing/kprobes: Allow kprobe-events to record module symbol

[Steven Rostedt]

On Wed, 3 Oct 2018 15:12:27 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 29 Aug 2018 01:18:43 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Allow kprobe-events to record module symbols.
> > 
> > Since data symbols in a non-loaded module doesn't exist, it fails to
> > define such symbol as an argument of kprobe-event. But if the kprobe
> > event is defined on that module, we can defer to resolve the symbol
> > address.
> > 
> > Note that if given symbol is not found, the event is kept unavailable.
> > User can enable it but the event is not recorded.
> > 
> >  
> 
> Hi Masami,
> 
> The first two patches look like they should be applied now and marked
> for stable, as they are fixes. This patch looks like an added feature,
> and will go into the next merge window.
> 
> Is that what you think too?
>

Heh, I just realized that this is on my kprobes branch. I was looking
at it as if they were in mainline. Nevermind about the "stable"
comment ;-) 

-- Steve

Re: [ftrace/kprobes PATCH 3/3] tracing/kprobes: Allow kprobe-events to record module symbol

[Masami Hiramatsu]

On Wed, 3 Oct 2018 21:11:28 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 3 Oct 2018 15:12:27 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Wed, 29 Aug 2018 01:18:43 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> > > Allow kprobe-events to record module symbols.
> > > 
> > > Since data symbols in a non-loaded module doesn't exist, it fails to
> > > define such symbol as an argument of kprobe-event. But if the kprobe
> > > event is defined on that module, we can defer to resolve the symbol
> > > address.
> > > 
> > > Note that if given symbol is not found, the event is kept unavailable.
> > > User can enable it but the event is not recorded.
> > > 
> > >  
> > 
> > Hi Masami,
> > 
> > The first two patches look like they should be applied now and marked
> > for stable, as they are fixes. This patch looks like an added feature,
> > and will go into the next merge window.
> > 
> > Is that what you think too?
> >
> 
> Heh, I just realized that this is on my kprobes branch. I was looking
> at it as if they were in mainline. Nevermind about the "stable"
> comment ;-) 

Yes, ;) so I added [ftrace/kprobes] on the subject.

Anyway, these patches must be on top of that branch.

Thank you,

> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [ftrace/kprobes PATCH 0/3] tracing: probeevent: Fix module symbol probing

[Masami Hiramatsu]

Hello Steve,

Could you also include this series to the branch?

Thank you,

On Wed, 29 Aug 2018 01:17:18 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi,
> 
> This series is for fixing some bugs in Steve's ftrace/kprobes branch.
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
> 
> Which is based on my fetcharg improvement series.
> 
> https://lkml.org/lkml/2018/4/25/601
> 
> This includes following fixes.
> 
> - Fix copy_from_user() misusing which Dan was reported.
> - Fix to reject incorrect probeevent on loaded kernel module.
> - Fix to update symbol-based argument on module.
>   This also checks the symbol-based argument is correct or not when
>   target module is loaded. If it is not correct, the event is kept
>   unavailable.
> 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (3):
>       tracing/uprobes: Fix to return -EFAULT if copy_from_user failed
>       tracing/kprobes: Check the probe on unloaded module correctly
>       tracing/kprobes: Allow kprobe-events to record module symbol
> 
> 
>  kernel/trace/trace_kprobe.c |   51 ++++++++++++++++++++++++++---------
>  kernel/trace/trace_probe.c  |   62 +++++++++++++++++++++++++++++++++++++------
>  kernel/trace/trace_probe.h  |    4 ++-
>  kernel/trace/trace_uprobe.c |    2 +
>  4 files changed, 95 insertions(+), 24 deletions(-)
> 
> --
> Masami Hiramatsu (Linaro) <mhiramat@kernel.org>


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [ftrace/kprobes PATCH 0/3] tracing: probeevent: Fix module symbol probing

[Steven Rostedt]

On Thu, 27 Sep 2018 19:48:58 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hello Steve,
> 
> Could you also include this series to the branch?
> 

Hi Masami, 

I just came back from Embedded / Kernel Recipes, I'll look at these
today.

-- Steve

Re: [ftrace/kprobes PATCH 0/3] tracing: probeevent: Fix module symbol probing

[Steven Rostedt]

On Mon, 1 Oct 2018 11:48:55 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 27 Sep 2018 19:48:58 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Hello Steve,
> > 
> > Could you also include this series to the branch?
> >   
> 
> Hi Masami, 
> 
> I just came back from Embedded / Kernel Recipes, I'll look at these
> today.
> 

Unfortunately, I've been pulled off to other tasks :-(

I'll make it a priority tomorrow (Wednesday).

-- Steve

Re: [ftrace/kprobes PATCH 0/3] tracing: probeevent: Fix module symbol probing

[Masami Hiramatsu]

On Tue, 2 Oct 2018 22:47:14 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 1 Oct 2018 11:48:55 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Thu, 27 Sep 2018 19:48:58 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> > > Hello Steve,
> > > 
> > > Could you also include this series to the branch?
> > >   
> > 
> > Hi Masami, 
> > 
> > I just came back from Embedded / Kernel Recipes, I'll look at these
> > today.
> > 
> 
> Unfortunately, I've been pulled off to other tasks :-(
> 
> I'll make it a priority tomorrow (Wednesday).

No problem, thank you for reviewing :)


> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>