LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] tracing/probe: fix memleak in fetch_op_data operations
@ 2020-06-15 14:30 Vamshi K Sthambamkadi
  2020-06-15 21:13 ` Steven Rostedt
  2020-06-16  8:41 ` Masami Hiramatsu
  0 siblings, 2 replies; 7+ messages in thread
From: Vamshi K Sthambamkadi @ 2020-06-15 14:30 UTC (permalink / raw)
  To: rostedt; +Cc: mingo, linux-kernel

kmemleak report:
    [<57dcc2ca>] __kmalloc_track_caller+0x139/0x2b0
    [<f1c45d0f>] kstrndup+0x37/0x80
    [<f9761eb0>] parse_probe_arg.isra.7+0x3cc/0x630
    [<055bf2ba>] traceprobe_parse_probe_arg+0x2f5/0x810
    [<655a7766>] trace_kprobe_create+0x2ca/0x950
    [<4fc6a02a>] create_or_delete_trace_kprobe+0xf/0x30
    [<6d1c8a52>] trace_run_command+0x67/0x80
    [<be812cc0>] trace_parse_run_command+0xa7/0x140
    [<aecfe401>] probes_write+0x10/0x20
    [<2027641c>] __vfs_write+0x30/0x1e0
    [<6a4aeee1>] vfs_write+0x96/0x1b0
    [<3517fb7d>] ksys_write+0x53/0xc0
    [<dad91db7>] __ia32_sys_write+0x15/0x20
    [<da347f64>] do_syscall_32_irqs_on+0x3d/0x260
    [<fd0b7e7d>] do_fast_syscall_32+0x39/0xb0
    [<ea5ae810>] entry_SYSENTER_32+0xaf/0x102

Post parse_probe_arg(), the FETCH_OP_DATA operation type is overwritten
to FETCH_OP_ST_STRING, as a result memory is never freed since
traceprobe_free_probe_arg() iterates only over SYMBOL and DATA op types

Setup fetch string operation correctly after fetch_op_data operation.

Signed-off-by: Vamshi K Sthambamkadi <vamshi.k.sthambamkadi@gmail.com>
---
 kernel/trace/trace_probe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index b8a928e..d2867cc 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -639,8 +639,8 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
 			ret = -EINVAL;
 			goto fail;
 		}
-		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM) ||
-		     parg->count) {
+		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM ||
+		     code->op == FETCH_OP_DATA) || parg->count) {
 			/*
 			 * IMM, DATA and COMM is pointing actual address, those
 			 * must be kept, and if parg->count != 0, this is an
-- 
2.7.4


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] tracing/probe: fix memleak in fetch_op_data operations
  2020-06-15 14:30 [PATCH] tracing/probe: fix memleak in fetch_op_data operations Vamshi K Sthambamkadi
@ 2020-06-15 21:13 ` Steven Rostedt
  2020-06-16  8:46   ` Masami Hiramatsu
  2020-06-16  8:41 ` Masami Hiramatsu
  1 sibling, 1 reply; 7+ messages in thread
From: Steven Rostedt @ 2020-06-15 21:13 UTC (permalink / raw)
  To: Vamshi K Sthambamkadi
  Cc: mingo, linux-kernel, Masami Hiramatsu, Srikar Dronamraju


Masami or Srikar would you like to look at this patch.

And wondering why you were not on the Cc to this patch in the first
place, please take a look at the patch I want to add at the bottom ;-)


On Mon, 15 Jun 2020 20:00:38 +0530
Vamshi K Sthambamkadi <vamshi.k.sthambamkadi@gmail.com> wrote:

> kmemleak report:
>     [<57dcc2ca>] __kmalloc_track_caller+0x139/0x2b0
>     [<f1c45d0f>] kstrndup+0x37/0x80
>     [<f9761eb0>] parse_probe_arg.isra.7+0x3cc/0x630
>     [<055bf2ba>] traceprobe_parse_probe_arg+0x2f5/0x810
>     [<655a7766>] trace_kprobe_create+0x2ca/0x950
>     [<4fc6a02a>] create_or_delete_trace_kprobe+0xf/0x30
>     [<6d1c8a52>] trace_run_command+0x67/0x80
>     [<be812cc0>] trace_parse_run_command+0xa7/0x140
>     [<aecfe401>] probes_write+0x10/0x20
>     [<2027641c>] __vfs_write+0x30/0x1e0
>     [<6a4aeee1>] vfs_write+0x96/0x1b0
>     [<3517fb7d>] ksys_write+0x53/0xc0
>     [<dad91db7>] __ia32_sys_write+0x15/0x20
>     [<da347f64>] do_syscall_32_irqs_on+0x3d/0x260
>     [<fd0b7e7d>] do_fast_syscall_32+0x39/0xb0
>     [<ea5ae810>] entry_SYSENTER_32+0xaf/0x102
> 
> Post parse_probe_arg(), the FETCH_OP_DATA operation type is overwritten
> to FETCH_OP_ST_STRING, as a result memory is never freed since
> traceprobe_free_probe_arg() iterates only over SYMBOL and DATA op types
> 
> Setup fetch string operation correctly after fetch_op_data operation.
> 
> Signed-off-by: Vamshi K Sthambamkadi <vamshi.k.sthambamkadi@gmail.com>
> ---
>  kernel/trace/trace_probe.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index b8a928e..d2867cc 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -639,8 +639,8 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
>  			ret = -EINVAL;
>  			goto fail;
>  		}
> -		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM) ||
> -		     parg->count) {
> +		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM ||
> +		     code->op == FETCH_OP_DATA) || parg->count) {
>  			/*
>  			 * IMM, DATA and COMM is pointing actual address, those
>  			 * must be kept, and if parg->count != 0, this is an


diff --git a/MAINTAINERS b/MAINTAINERS
index 47873f2e6696..116e5cc7ef95 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9229,6 +9229,8 @@ F:	Documentation/kprobes.txt
 F:	include/linux/kprobes.h
 F:	include/asm-generic/kprobes.h
 F:	kernel/kprobes.c
+F:	kernel/trace/trace_kprobe.c
+F:	kernel/trace/trace_probe.c
 
 KS0108 LCD CONTROLLER DRIVER
 M:	Miguel Ojeda Sandonis <miguel.ojeda.sandonis@gmail.com>
@@ -16996,6 +16998,16 @@ F:	drivers/mtd/ubi/
 F:	include/linux/mtd/ubi.h
 F:	include/uapi/mtd/ubi-user.h
 
+UPROBES
+M:	Srikar Dronamraju <srikar@linux.vnet.ibm.com>
+S:	Maintained
+F:	Documentation/trace/uprobetracer.rst
+F:	Documentation/features/debug/uprobes
+F:	include/linux/uprobes.h
+F:	kernel/events/uprobes.c
+F:	kernel/trace/trace_uprobe.c
+F:	kernel/trace/trace_probe.c
+
 USB "USBNET" DRIVER FRAMEWORK
 M:	Oliver Neukum <oneukum@suse.com>
 L:	netdev@vger.kernel.org


-- Steve

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] tracing/probe: fix memleak in fetch_op_data operations
  2020-06-15 14:30 [PATCH] tracing/probe: fix memleak in fetch_op_data operations Vamshi K Sthambamkadi
  2020-06-15 21:13 ` Steven Rostedt
@ 2020-06-16  8:41 ` Masami Hiramatsu
  1 sibling, 0 replies; 7+ messages in thread
From: Masami Hiramatsu @ 2020-06-16  8:41 UTC (permalink / raw)
  To: Vamshi K Sthambamkadi; +Cc: rostedt, mingo, linux-kernel

On Mon, 15 Jun 2020 20:00:38 +0530
Vamshi K Sthambamkadi <vamshi.k.sthambamkadi@gmail.com> wrote:

> kmemleak report:
>     [<57dcc2ca>] __kmalloc_track_caller+0x139/0x2b0
>     [<f1c45d0f>] kstrndup+0x37/0x80
>     [<f9761eb0>] parse_probe_arg.isra.7+0x3cc/0x630
>     [<055bf2ba>] traceprobe_parse_probe_arg+0x2f5/0x810
>     [<655a7766>] trace_kprobe_create+0x2ca/0x950
>     [<4fc6a02a>] create_or_delete_trace_kprobe+0xf/0x30
>     [<6d1c8a52>] trace_run_command+0x67/0x80
>     [<be812cc0>] trace_parse_run_command+0xa7/0x140
>     [<aecfe401>] probes_write+0x10/0x20
>     [<2027641c>] __vfs_write+0x30/0x1e0
>     [<6a4aeee1>] vfs_write+0x96/0x1b0
>     [<3517fb7d>] ksys_write+0x53/0xc0
>     [<dad91db7>] __ia32_sys_write+0x15/0x20
>     [<da347f64>] do_syscall_32_irqs_on+0x3d/0x260
>     [<fd0b7e7d>] do_fast_syscall_32+0x39/0xb0
>     [<ea5ae810>] entry_SYSENTER_32+0xaf/0x102
> 
> Post parse_probe_arg(), the FETCH_OP_DATA operation type is overwritten
> to FETCH_OP_ST_STRING, as a result memory is never freed since
> traceprobe_free_probe_arg() iterates only over SYMBOL and DATA op types
> 
> Setup fetch string operation correctly after fetch_op_data operation.
> 

Oops, Good catch! (I've commented it but not coded...)

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

And,

Fixes: a42e3c4de964 ("tracing/probe: Add immediate string parameter support")
Cc: stable@vger.kernel.org

Thank you!

> Signed-off-by: Vamshi K Sthambamkadi <vamshi.k.sthambamkadi@gmail.com>
> ---
>  kernel/trace/trace_probe.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index b8a928e..d2867cc 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -639,8 +639,8 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
>  			ret = -EINVAL;
>  			goto fail;
>  		}
> -		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM) ||
> -		     parg->count) {
> +		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM ||
> +		     code->op == FETCH_OP_DATA) || parg->count) {
>  			/*
>  			 * IMM, DATA and COMM is pointing actual address, those
>  			 * must be kept, and if parg->count != 0, this is an
> -- 
> 2.7.4
> 


-- 
Masami Hiramatsu

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] tracing/probe: fix memleak in fetch_op_data operations
  2020-06-15 21:13 ` Steven Rostedt
@ 2020-06-16  8:46   ` Masami Hiramatsu
  2020-06-16 14:19     ` Oleg Nesterov
  0 siblings, 1 reply; 7+ messages in thread
From: Masami Hiramatsu @ 2020-06-16  8:46 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Vamshi K Sthambamkadi, mingo, linux-kernel, Masami Hiramatsu,
	Srikar Dronamraju, oleg

Hi Steve,

On Mon, 15 Jun 2020 17:13:37 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> Masami or Srikar would you like to look at this patch.

Thanks for letting me know!

> 
> And wondering why you were not on the Cc to this patch in the first
> place, please take a look at the patch I want to add at the bottom ;-)

Agreed to expand MAINTAINERS to clarify that. For the kprobes part, 

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

BTW, for uprobes, I think Oleg is mainly reviewed changes on that in
these days. Maybe better to assign him?

Thanks,


> 
> 
> On Mon, 15 Jun 2020 20:00:38 +0530
> Vamshi K Sthambamkadi <vamshi.k.sthambamkadi@gmail.com> wrote:
> 
> > kmemleak report:
> >     [<57dcc2ca>] __kmalloc_track_caller+0x139/0x2b0
> >     [<f1c45d0f>] kstrndup+0x37/0x80
> >     [<f9761eb0>] parse_probe_arg.isra.7+0x3cc/0x630
> >     [<055bf2ba>] traceprobe_parse_probe_arg+0x2f5/0x810
> >     [<655a7766>] trace_kprobe_create+0x2ca/0x950
> >     [<4fc6a02a>] create_or_delete_trace_kprobe+0xf/0x30
> >     [<6d1c8a52>] trace_run_command+0x67/0x80
> >     [<be812cc0>] trace_parse_run_command+0xa7/0x140
> >     [<aecfe401>] probes_write+0x10/0x20
> >     [<2027641c>] __vfs_write+0x30/0x1e0
> >     [<6a4aeee1>] vfs_write+0x96/0x1b0
> >     [<3517fb7d>] ksys_write+0x53/0xc0
> >     [<dad91db7>] __ia32_sys_write+0x15/0x20
> >     [<da347f64>] do_syscall_32_irqs_on+0x3d/0x260
> >     [<fd0b7e7d>] do_fast_syscall_32+0x39/0xb0
> >     [<ea5ae810>] entry_SYSENTER_32+0xaf/0x102
> > 
> > Post parse_probe_arg(), the FETCH_OP_DATA operation type is overwritten
> > to FETCH_OP_ST_STRING, as a result memory is never freed since
> > traceprobe_free_probe_arg() iterates only over SYMBOL and DATA op types
> > 
> > Setup fetch string operation correctly after fetch_op_data operation.
> > 
> > Signed-off-by: Vamshi K Sthambamkadi <vamshi.k.sthambamkadi@gmail.com>
> > ---
> >  kernel/trace/trace_probe.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > index b8a928e..d2867cc 100644
> > --- a/kernel/trace/trace_probe.c
> > +++ b/kernel/trace/trace_probe.c
> > @@ -639,8 +639,8 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
> >  			ret = -EINVAL;
> >  			goto fail;
> >  		}
> > -		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM) ||
> > -		     parg->count) {
> > +		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM ||
> > +		     code->op == FETCH_OP_DATA) || parg->count) {
> >  			/*
> >  			 * IMM, DATA and COMM is pointing actual address, those
> >  			 * must be kept, and if parg->count != 0, this is an
> 
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 47873f2e6696..116e5cc7ef95 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9229,6 +9229,8 @@ F:	Documentation/kprobes.txt
>  F:	include/linux/kprobes.h
>  F:	include/asm-generic/kprobes.h
>  F:	kernel/kprobes.c
> +F:	kernel/trace/trace_kprobe.c
> +F:	kernel/trace/trace_probe.c
>  
>  KS0108 LCD CONTROLLER DRIVER
>  M:	Miguel Ojeda Sandonis <miguel.ojeda.sandonis@gmail.com>
> @@ -16996,6 +16998,16 @@ F:	drivers/mtd/ubi/
>  F:	include/linux/mtd/ubi.h
>  F:	include/uapi/mtd/ubi-user.h
>  
> +UPROBES
> +M:	Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> +S:	Maintained
> +F:	Documentation/trace/uprobetracer.rst
> +F:	Documentation/features/debug/uprobes
> +F:	include/linux/uprobes.h
> +F:	kernel/events/uprobes.c
> +F:	kernel/trace/trace_uprobe.c
> +F:	kernel/trace/trace_probe.c
> +
>  USB "USBNET" DRIVER FRAMEWORK
>  M:	Oliver Neukum <oneukum@suse.com>
>  L:	netdev@vger.kernel.org
> 
> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] tracing/probe: fix memleak in fetch_op_data operations
  2020-06-16  8:46   ` Masami Hiramatsu
@ 2020-06-16 14:19     ` Oleg Nesterov
  2020-06-16 16:21       ` Srikar Dronamraju
  0 siblings, 1 reply; 7+ messages in thread
From: Oleg Nesterov @ 2020-06-16 14:19 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Steven Rostedt, Vamshi K Sthambamkadi, mingo, linux-kernel,
	Srikar Dronamraju

On 06/16, Masami Hiramatsu wrote:
>
> BTW, for uprobes, I think Oleg is mainly reviewed changes on that in
> these days. Maybe better to assign him?

Well, then I'd suggest both me and Srikar.

Oleg.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] tracing/probe: fix memleak in fetch_op_data operations
  2020-06-16 14:19     ` Oleg Nesterov
@ 2020-06-16 16:21       ` Srikar Dronamraju
  0 siblings, 0 replies; 7+ messages in thread
From: Srikar Dronamraju @ 2020-06-16 16:21 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Masami Hiramatsu, Steven Rostedt, Vamshi K Sthambamkadi, mingo,
	linux-kernel

* Oleg Nesterov <oleg@redhat.com> [2020-06-16 16:19:22]:

> On 06/16, Masami Hiramatsu wrote:
> >
> > BTW, for uprobes, I think Oleg is mainly reviewed changes on that in
> > these days. Maybe better to assign him?
> 
> Well, then I'd suggest both me and Srikar.
> 

Agree with Oleg. Actually he is more upto date with uprobes code.

-- 
Thanks and Regards
Srikar Dronamraju

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] tracing/probe: fix memleak in fetch_op_data operations
@ 2020-06-15 15:10 Markus Elfring
  0 siblings, 0 replies; 7+ messages in thread
From: Markus Elfring @ 2020-06-15 15:10 UTC (permalink / raw)
  To: Vamshi K Sthambamkadi, Ingo Molnar, Steven Rostedt
  Cc: linux-kernel, kernel-janitors

…
> Post parse_probe_arg(), the FETCH_OP_DATA operation type is overwritten
> to FETCH_OP_ST_STRING, as a result memory is never freed …

How do you think about to use the term “memory leak” in the patch subject?

Will the tag “Fixes” become helpful for the commit message?

Regards,
Markus

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-15 14:30 [PATCH] tracing/probe: fix memleak in fetch_op_data operations Vamshi K Sthambamkadi
2020-06-15 21:13 ` Steven Rostedt
2020-06-16  8:46   ` Masami Hiramatsu
2020-06-16 14:19     ` Oleg Nesterov
2020-06-16 16:21       ` Srikar Dronamraju
2020-06-16  8:41 ` Masami Hiramatsu
2020-06-15 15:10 Markus Elfring

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git