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 <[email protected]>
---
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
…
> 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
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 <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
@@ -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 <[email protected]>
+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 <[email protected]>
L: [email protected]
-- Steve
On Mon, 15 Jun 2020 20:00:38 +0530
Vamshi K Sthambamkadi <[email protected]> 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 <[email protected]>
And,
Fixes: a42e3c4de964 ("tracing/probe: Add immediate string parameter support")
Cc: [email protected]
Thank you!
> Signed-off-by: Vamshi K Sthambamkadi <[email protected]>
> ---
> 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
Hi Steve,
On Mon, 15 Jun 2020 17:13:37 -0400
Steven Rostedt <[email protected]> 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> @@ -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 <[email protected]>
> +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 <[email protected]>
> L: [email protected]
>
>
> -- Steve
--
Masami Hiramatsu <[email protected]>
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.
* Oleg Nesterov <[email protected]> [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