* [PATCH 0/2] tracing: Fix some bug about synth
@ 2022-11-08 8:31 Shang XiaoJing
2022-11-08 8:31 ` [PATCH 1/2] tracing: Fix memory leak in test_gen_synth_cmd() and test_empty_synth_event() Shang XiaoJing
2022-11-08 8:31 ` [PATCH 2/2] tracing: Fix wild-memory-access in register_synth_event() Shang XiaoJing
0 siblings, 2 replies; 7+ messages in thread
From: Shang XiaoJing @ 2022-11-08 8:31 UTC (permalink / raw)
To: rostedt, mhiramat, zanussi, fengguang.wu, linux-kernel; +Cc: shangxiaojing
Some bugs are found when insert synth_event_gen_test, and fixed by this
patch set.
Shang XiaoJing (2):
tracing: Fix memory leak in test_gen_synth_cmd() and
test_empty_synth_event()
tracing: Fix wild-memory-access in register_synth_event()
kernel/trace/synth_event_gen_test.c | 2 ++
kernel/trace/trace_events_synth.c | 2 ++
2 files changed, 4 insertions(+)
--
2.17.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/2] tracing: Fix memory leak in test_gen_synth_cmd() and test_empty_synth_event() 2022-11-08 8:31 [PATCH 0/2] tracing: Fix some bug about synth Shang XiaoJing @ 2022-11-08 8:31 ` Shang XiaoJing 2022-11-12 21:12 ` Tom Zanussi 2022-11-08 8:31 ` [PATCH 2/2] tracing: Fix wild-memory-access in register_synth_event() Shang XiaoJing 1 sibling, 1 reply; 7+ messages in thread From: Shang XiaoJing @ 2022-11-08 8:31 UTC (permalink / raw) To: rostedt, mhiramat, zanussi, fengguang.wu, linux-kernel; +Cc: shangxiaojing test_gen_synth_cmd() only free buf in fail path, hence buf will leak when there is no failure. Add kfree(buf) to prevent the memleak. The same reason and solution in test_empty_synth_event(). unreferenced object 0xffff8881127de000 (size 2048): comm "modprobe", pid 247, jiffies 4294972316 (age 78.756s) hex dump (first 32 bytes): 20 67 65 6e 5f 73 79 6e 74 68 5f 74 65 73 74 20 gen_synth_test 20 70 69 64 5f 74 20 6e 65 78 74 5f 70 69 64 5f pid_t next_pid_ backtrace: [<000000004254801a>] kmalloc_trace+0x26/0x100 [<0000000039eb1cf5>] 0xffffffffa00083cd [<000000000e8c3bc8>] 0xffffffffa00086ba [<00000000c293d1ea>] do_one_initcall+0xdb/0x480 [<00000000aa189e6d>] do_init_module+0x1cf/0x680 [<00000000d513222b>] load_module+0x6a50/0x70a0 [<000000001fd4d529>] __do_sys_finit_module+0x12f/0x1c0 [<00000000b36c4c0f>] do_syscall_64+0x3f/0x90 [<00000000bbf20cf3>] entry_SYSCALL_64_after_hwframe+0x63/0xcd unreferenced object 0xffff8881127df000 (size 2048): comm "modprobe", pid 247, jiffies 4294972324 (age 78.728s) hex dump (first 32 bytes): 20 65 6d 70 74 79 5f 73 79 6e 74 68 5f 74 65 73 empty_synth_tes 74 20 20 70 69 64 5f 74 20 6e 65 78 74 5f 70 69 t pid_t next_pi backtrace: [<000000004254801a>] kmalloc_trace+0x26/0x100 [<00000000d4db9a3d>] 0xffffffffa0008071 [<00000000c31354a5>] 0xffffffffa00086ce [<00000000c293d1ea>] do_one_initcall+0xdb/0x480 [<00000000aa189e6d>] do_init_module+0x1cf/0x680 [<00000000d513222b>] load_module+0x6a50/0x70a0 [<000000001fd4d529>] __do_sys_finit_module+0x12f/0x1c0 [<00000000b36c4c0f>] do_syscall_64+0x3f/0x90 [<00000000bbf20cf3>] entry_SYSCALL_64_after_hwframe+0x63/0xcd Fixes: 9fe41efaca08 ("tracing: Add synth event generation test module") Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> --- kernel/trace/synth_event_gen_test.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/kernel/trace/synth_event_gen_test.c b/kernel/trace/synth_event_gen_test.c index 0b15e975d2c2..db1ec4809ad1 100644 --- a/kernel/trace/synth_event_gen_test.c +++ b/kernel/trace/synth_event_gen_test.c @@ -120,6 +120,7 @@ static int __init test_gen_synth_cmd(void) /* Now generate a gen_synth_test event */ ret = synth_event_trace_array(gen_synth_test, vals, ARRAY_SIZE(vals)); + kfree(buf); out: return ret; delete: @@ -227,6 +228,7 @@ static int __init test_empty_synth_event(void) /* Now trace an empty_synth_test event */ ret = synth_event_trace_array(empty_synth_test, vals, ARRAY_SIZE(vals)); + kfree(buf); out: return ret; delete: -- 2.17.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] tracing: Fix memory leak in test_gen_synth_cmd() and test_empty_synth_event() 2022-11-08 8:31 ` [PATCH 1/2] tracing: Fix memory leak in test_gen_synth_cmd() and test_empty_synth_event() Shang XiaoJing @ 2022-11-12 21:12 ` Tom Zanussi 2022-11-13 6:48 ` shangxiaojing 0 siblings, 1 reply; 7+ messages in thread From: Tom Zanussi @ 2022-11-12 21:12 UTC (permalink / raw) To: Shang XiaoJing, rostedt, mhiramat, fengguang.wu, linux-kernel Hi Shang, Thanks for finding this bug, comment below... On Tue, 2022-11-08 at 16:31 +0800, Shang XiaoJing wrote: > test_gen_synth_cmd() only free buf in fail path, hence buf will leak > when there is no failure. Add kfree(buf) to prevent the memleak. The > same reason and solution in test_empty_synth_event(). > > unreferenced object 0xffff8881127de000 (size 2048): > comm "modprobe", pid 247, jiffies 4294972316 (age 78.756s) > hex dump (first 32 bytes): > 20 67 65 6e 5f 73 79 6e 74 68 5f 74 65 73 74 20 gen_synth_test > 20 70 69 64 5f 74 20 6e 65 78 74 5f 70 69 64 5f pid_t next_pid_ > backtrace: > [<000000004254801a>] kmalloc_trace+0x26/0x100 > [<0000000039eb1cf5>] 0xffffffffa00083cd > [<000000000e8c3bc8>] 0xffffffffa00086ba > [<00000000c293d1ea>] do_one_initcall+0xdb/0x480 > [<00000000aa189e6d>] do_init_module+0x1cf/0x680 > [<00000000d513222b>] load_module+0x6a50/0x70a0 > [<000000001fd4d529>] __do_sys_finit_module+0x12f/0x1c0 > [<00000000b36c4c0f>] do_syscall_64+0x3f/0x90 > [<00000000bbf20cf3>] entry_SYSCALL_64_after_hwframe+0x63/0xcd > unreferenced object 0xffff8881127df000 (size 2048): > comm "modprobe", pid 247, jiffies 4294972324 (age 78.728s) > hex dump (first 32 bytes): > 20 65 6d 70 74 79 5f 73 79 6e 74 68 5f 74 65 73 empty_synth_tes > 74 20 20 70 69 64 5f 74 20 6e 65 78 74 5f 70 69 t pid_t next_pi > backtrace: > [<000000004254801a>] kmalloc_trace+0x26/0x100 > [<00000000d4db9a3d>] 0xffffffffa0008071 > [<00000000c31354a5>] 0xffffffffa00086ce > [<00000000c293d1ea>] do_one_initcall+0xdb/0x480 > [<00000000aa189e6d>] do_init_module+0x1cf/0x680 > [<00000000d513222b>] load_module+0x6a50/0x70a0 > [<000000001fd4d529>] __do_sys_finit_module+0x12f/0x1c0 > [<00000000b36c4c0f>] do_syscall_64+0x3f/0x90 > [<00000000bbf20cf3>] entry_SYSCALL_64_after_hwframe+0x63/0xcd > > Fixes: 9fe41efaca08 ("tracing: Add synth event generation test > module") > Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> > --- > kernel/trace/synth_event_gen_test.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/kernel/trace/synth_event_gen_test.c > b/kernel/trace/synth_event_gen_test.c > index 0b15e975d2c2..db1ec4809ad1 100644 > --- a/kernel/trace/synth_event_gen_test.c > +++ b/kernel/trace/synth_event_gen_test.c > @@ -120,6 +120,7 @@ static int __init test_gen_synth_cmd(void) > > /* Now generate a gen_synth_test event */ > ret = synth_event_trace_array(gen_synth_test, vals, > ARRAY_SIZE(vals)); > + kfree(buf); > out: > return ret; > delete: > @@ -227,6 +228,7 @@ static int __init test_empty_synth_event(void) > > /* Now trace an empty_synth_test event */ > ret = synth_event_trace_array(empty_synth_test, vals, > ARRAY_SIZE(vals)); > + kfree(buf); > out: > return ret; > delete: Makes sense, and if you do this then you could probably remove the other kfree() at the bottom and clean the code up a bit, like change this: /* Now generate a gen_synth_test event */ ret = synth_event_trace_array(gen_synth_test, vals, ARRAY_SIZE(vals)); kfree(buf); out: return ret; delete: /* We got an error after creating the event, delete it */ synth_event_delete("gen_synth_test"); free: kfree(buf); goto out; } to this: /* Now generate a gen_synth_test event */ ret = synth_event_trace_array(gen_synth_test, vals, ARRAY_SIZE(vals)); free: kfree(buf); return ret; delete: /* We got an error after creating the event, delete it */ synth_event_delete("gen_synth_test"); goto free; } What do you think? Tom ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] tracing: Fix memory leak in test_gen_synth_cmd() and test_empty_synth_event() 2022-11-12 21:12 ` Tom Zanussi @ 2022-11-13 6:48 ` shangxiaojing 0 siblings, 0 replies; 7+ messages in thread From: shangxiaojing @ 2022-11-13 6:48 UTC (permalink / raw) To: Tom Zanussi, rostedt, mhiramat, fengguang.wu, linux-kernel On 2022/11/13 5:12, Tom Zanussi wrote: > Hi Shang, > > Thanks for finding this bug, comment below... > > On Tue, 2022-11-08 at 16:31 +0800, Shang XiaoJing wrote: >> test_gen_synth_cmd() only free buf in fail path, hence buf will leak >> when there is no failure. Add kfree(buf) to prevent the memleak. The >> same reason and solution in test_empty_synth_event(). >> >> unreferenced object 0xffff8881127de000 (size 2048): >> comm "modprobe", pid 247, jiffies 4294972316 (age 78.756s) >> hex dump (first 32 bytes): >> 20 67 65 6e 5f 73 79 6e 74 68 5f 74 65 73 74 20 gen_synth_test >> 20 70 69 64 5f 74 20 6e 65 78 74 5f 70 69 64 5f pid_t next_pid_ >> backtrace: >> [<000000004254801a>] kmalloc_trace+0x26/0x100 >> [<0000000039eb1cf5>] 0xffffffffa00083cd >> [<000000000e8c3bc8>] 0xffffffffa00086ba >> [<00000000c293d1ea>] do_one_initcall+0xdb/0x480 >> [<00000000aa189e6d>] do_init_module+0x1cf/0x680 >> [<00000000d513222b>] load_module+0x6a50/0x70a0 >> [<000000001fd4d529>] __do_sys_finit_module+0x12f/0x1c0 >> [<00000000b36c4c0f>] do_syscall_64+0x3f/0x90 >> [<00000000bbf20cf3>] entry_SYSCALL_64_after_hwframe+0x63/0xcd >> unreferenced object 0xffff8881127df000 (size 2048): >> comm "modprobe", pid 247, jiffies 4294972324 (age 78.728s) >> hex dump (first 32 bytes): >> 20 65 6d 70 74 79 5f 73 79 6e 74 68 5f 74 65 73 empty_synth_tes >> 74 20 20 70 69 64 5f 74 20 6e 65 78 74 5f 70 69 t pid_t next_pi >> backtrace: >> [<000000004254801a>] kmalloc_trace+0x26/0x100 >> [<00000000d4db9a3d>] 0xffffffffa0008071 >> [<00000000c31354a5>] 0xffffffffa00086ce >> [<00000000c293d1ea>] do_one_initcall+0xdb/0x480 >> [<00000000aa189e6d>] do_init_module+0x1cf/0x680 >> [<00000000d513222b>] load_module+0x6a50/0x70a0 >> [<000000001fd4d529>] __do_sys_finit_module+0x12f/0x1c0 >> [<00000000b36c4c0f>] do_syscall_64+0x3f/0x90 >> [<00000000bbf20cf3>] entry_SYSCALL_64_after_hwframe+0x63/0xcd >> >> Fixes: 9fe41efaca08 ("tracing: Add synth event generation test >> module") >> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> >> --- >> kernel/trace/synth_event_gen_test.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/kernel/trace/synth_event_gen_test.c >> b/kernel/trace/synth_event_gen_test.c >> index 0b15e975d2c2..db1ec4809ad1 100644 >> --- a/kernel/trace/synth_event_gen_test.c >> +++ b/kernel/trace/synth_event_gen_test.c >> @@ -120,6 +120,7 @@ static int __init test_gen_synth_cmd(void) >> >> /* Now generate a gen_synth_test event */ >> ret = synth_event_trace_array(gen_synth_test, vals, >> ARRAY_SIZE(vals)); >> + kfree(buf); >> out: >> return ret; >> delete: >> @@ -227,6 +228,7 @@ static int __init test_empty_synth_event(void) >> >> /* Now trace an empty_synth_test event */ >> ret = synth_event_trace_array(empty_synth_test, vals, >> ARRAY_SIZE(vals)); >> + kfree(buf); >> out: >> return ret; >> delete: > > Makes sense, and if you do this then you could probably remove the > other kfree() at the bottom and clean the code up a bit, like change > this: > > /* Now generate a gen_synth_test event */ > ret = synth_event_trace_array(gen_synth_test, vals, ARRAY_SIZE(vals)); > kfree(buf); > out: > return ret; > delete: > /* We got an error after creating the event, delete it */ > synth_event_delete("gen_synth_test"); > free: > kfree(buf); > > goto out; > } > > to this: > > /* Now generate a gen_synth_test event */ > ret = synth_event_trace_array(gen_synth_test, vals, ARRAY_SIZE(vals)); > free: > kfree(buf); > return ret; > delete: > /* We got an error after creating the event, delete it */ > synth_event_delete("gen_synth_test"); > goto free; > } > > What do you think? > Right, I thought maybe less change is better for the bugfix. It looks better to carry some clean up, thanks for the review, will fix in v2. Thanks, -- Shang XiaoJing ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] tracing: Fix wild-memory-access in register_synth_event() 2022-11-08 8:31 [PATCH 0/2] tracing: Fix some bug about synth Shang XiaoJing 2022-11-08 8:31 ` [PATCH 1/2] tracing: Fix memory leak in test_gen_synth_cmd() and test_empty_synth_event() Shang XiaoJing @ 2022-11-08 8:31 ` Shang XiaoJing 2022-11-12 21:24 ` Tom Zanussi 1 sibling, 1 reply; 7+ messages in thread From: Shang XiaoJing @ 2022-11-08 8:31 UTC (permalink / raw) To: rostedt, mhiramat, zanussi, fengguang.wu, linux-kernel; +Cc: shangxiaojing In register_synth_event(), if set_synth_event_print_fmt() failed, then both trace_remove_event_call() and unregister_trace_event() will be called. If call->event.funcs is not NULL, then the trace_event_call will call __unregister_trace_event() twice. As the result, the second __unregister_trace_event() will causes the wild-memory-access. register_synth_event set_synth_event_print_fmt failed trace_remove_event_call event_remove if call->event.funcs then __unregister_trace_event (first call) unregister_trace_event __unregister_trace_event (second call) Fix the bug by avoiding to call the second __unregister_trace_event() by checking if the first one is called. general protection fault, probably for non-canonical address 0xfbd59c0000000024: 0000 [#1] SMP KASAN PTI KASAN: maybe wild-memory-access in range [0xdead000000000120-0xdead000000000127] CPU: 0 PID: 3807 Comm: modprobe Not tainted 6.1.0-rc1-00186-g76f33a7eedb4 #299 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org 04/01/2014 RIP: 0010:unregister_trace_event+0x6e/0x280 Code: 00 fc ff df 4c 89 ea 48 c1 ea 03 80 3c 02 00 0f 85 0e 02 00 00 48 b8 00 00 00 00 00 fc ff df 4c 8b 63 08 4c 89 e2 48 c1 ea 03 <80> 3c 02 00 0f 85 e2 01 00 00 49 89 2c 24 48 85 ed 74 28 e8 7a 9b RSP: 0018:ffff88810413f370 EFLAGS: 00010a06 RAX: dffffc0000000000 RBX: ffff888105d050b0 RCX: 0000000000000000 RDX: 1bd5a00000000024 RSI: ffff888119e276e0 RDI: ffffffff835a8b20 RBP: dead000000000100 R08: 0000000000000000 R09: fffffbfff0913481 R10: ffffffff8489a407 R11: fffffbfff0913480 R12: dead000000000122 R13: ffff888105d050b8 R14: 0000000000000000 R15: ffff888105d05028 FS: 00007f7823e8d540(0000) GS:ffff888119e00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f7823e7ebec CR3: 000000010a058002 CR4: 0000000000330ef0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> __create_synth_event+0x1e37/0x1eb0 create_or_delete_synth_event+0x110/0x250 synth_event_run_command+0x2f/0x110 test_gen_synth_cmd+0x170/0x2eb [synth_event_gen_test] synth_event_gen_test_init+0x76/0x9bc [synth_event_gen_test] do_one_initcall+0xdb/0x480 do_init_module+0x1cf/0x680 load_module+0x6a50/0x70a0 __do_sys_finit_module+0x12f/0x1c0 do_syscall_64+0x3f/0x90 entry_SYSCALL_64_after_hwframe+0x63/0xcd Fixes: 4b147936fa50 ("tracing: Add support for 'synthetic' events") Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> Cc: stable@vger.kernel.org --- kernel/trace/trace_events_synth.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c index e310052dc83c..a51280a153e3 100644 --- a/kernel/trace/trace_events_synth.c +++ b/kernel/trace/trace_events_synth.c @@ -830,6 +830,8 @@ static int register_synth_event(struct synth_event *event) ret = set_synth_event_print_fmt(call); if (ret < 0) { trace_remove_event_call(call); + if (call->event.funcs) + return ret; goto err; } out: -- 2.17.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] tracing: Fix wild-memory-access in register_synth_event() 2022-11-08 8:31 ` [PATCH 2/2] tracing: Fix wild-memory-access in register_synth_event() Shang XiaoJing @ 2022-11-12 21:24 ` Tom Zanussi 2022-11-13 6:49 ` shangxiaojing 0 siblings, 1 reply; 7+ messages in thread From: Tom Zanussi @ 2022-11-12 21:24 UTC (permalink / raw) To: Shang XiaoJing, rostedt, mhiramat, fengguang.wu, linux-kernel Hi Shang, On Tue, 2022-11-08 at 16:31 +0800, Shang XiaoJing wrote: > In register_synth_event(), if set_synth_event_print_fmt() failed, then > both trace_remove_event_call() and unregister_trace_event() will be > called. If call->event.funcs is not NULL, then the trace_event_call will > call __unregister_trace_event() twice. As the result, the second > __unregister_trace_event() will causes the wild-memory-access. > > register_synth_event > set_synth_event_print_fmt failed > trace_remove_event_call > event_remove > if call->event.funcs then > __unregister_trace_event (first call) > unregister_trace_event > __unregister_trace_event (second call) > > Fix the bug by avoiding to call the second __unregister_trace_event() by > checking if the first one is called. > > general protection fault, probably for non-canonical address > 0xfbd59c0000000024: 0000 [#1] SMP KASAN PTI > KASAN: maybe wild-memory-access in range > [0xdead000000000120-0xdead000000000127] > CPU: 0 PID: 3807 Comm: modprobe Not tainted > 6.1.0-rc1-00186-g76f33a7eedb4 #299 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org 04/01/2014 > RIP: 0010:unregister_trace_event+0x6e/0x280 > Code: 00 fc ff df 4c 89 ea 48 c1 ea 03 80 3c 02 00 0f 85 0e 02 00 00 48 > b8 00 00 00 00 00 fc ff df 4c 8b 63 08 4c 89 e2 48 c1 ea 03 <80> 3c 02 > 00 0f 85 e2 01 00 00 49 89 2c 24 48 85 ed 74 28 e8 7a 9b > RSP: 0018:ffff88810413f370 EFLAGS: 00010a06 > RAX: dffffc0000000000 RBX: ffff888105d050b0 RCX: 0000000000000000 > RDX: 1bd5a00000000024 RSI: ffff888119e276e0 RDI: ffffffff835a8b20 > RBP: dead000000000100 R08: 0000000000000000 R09: fffffbfff0913481 > R10: ffffffff8489a407 R11: fffffbfff0913480 R12: dead000000000122 > R13: ffff888105d050b8 R14: 0000000000000000 R15: ffff888105d05028 > FS: 00007f7823e8d540(0000) GS:ffff888119e00000(0000) > knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00007f7823e7ebec CR3: 000000010a058002 CR4: 0000000000330ef0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > <TASK> > __create_synth_event+0x1e37/0x1eb0 > create_or_delete_synth_event+0x110/0x250 > synth_event_run_command+0x2f/0x110 > test_gen_synth_cmd+0x170/0x2eb [synth_event_gen_test] > synth_event_gen_test_init+0x76/0x9bc [synth_event_gen_test] > do_one_initcall+0xdb/0x480 > do_init_module+0x1cf/0x680 > load_module+0x6a50/0x70a0 > __do_sys_finit_module+0x12f/0x1c0 > do_syscall_64+0x3f/0x90 > entry_SYSCALL_64_after_hwframe+0x63/0xcd > > Fixes: 4b147936fa50 ("tracing: Add support for 'synthetic' events") > Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> > Cc: stable@vger.kernel.org > --- > kernel/trace/trace_events_synth.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c > index e310052dc83c..a51280a153e3 100644 > --- a/kernel/trace/trace_events_synth.c > +++ b/kernel/trace/trace_events_synth.c > @@ -830,6 +830,8 @@ static int register_synth_event(struct synth_event *event) > ret = set_synth_event_print_fmt(call); > if (ret < 0) { > trace_remove_event_call(call); > + if (call->event.funcs) > + return ret; > goto err; > } > out: Good catch, thanks for finding this bug! It looks like call->event.funcs will always be true here since it's set to &synth_event_funcs above. So it seems like you could just call trace_remove_event_call() and fall through for this (ret < 0) case? If so, it might be good to put a comment there noting that trace_remove_event_call() will call unregister_trace_event(), so it's ok to just return. Thanks, Tom ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] tracing: Fix wild-memory-access in register_synth_event() 2022-11-12 21:24 ` Tom Zanussi @ 2022-11-13 6:49 ` shangxiaojing 0 siblings, 0 replies; 7+ messages in thread From: shangxiaojing @ 2022-11-13 6:49 UTC (permalink / raw) To: Tom Zanussi, rostedt, mhiramat, fengguang.wu, linux-kernel On 2022/11/13 5:24, Tom Zanussi wrote: > Hi Shang, > > On Tue, 2022-11-08 at 16:31 +0800, Shang XiaoJing wrote: >> In register_synth_event(), if set_synth_event_print_fmt() failed, then >> both trace_remove_event_call() and unregister_trace_event() will be >> called. If call->event.funcs is not NULL, then the trace_event_call will >> call __unregister_trace_event() twice. As the result, the second >> __unregister_trace_event() will causes the wild-memory-access. >> >> register_synth_event >> set_synth_event_print_fmt failed >> trace_remove_event_call >> event_remove >> if call->event.funcs then >> __unregister_trace_event (first call) >> unregister_trace_event >> __unregister_trace_event (second call) >> >> Fix the bug by avoiding to call the second __unregister_trace_event() by >> checking if the first one is called. >> >> general protection fault, probably for non-canonical address >> 0xfbd59c0000000024: 0000 [#1] SMP KASAN PTI >> KASAN: maybe wild-memory-access in range >> [0xdead000000000120-0xdead000000000127] >> CPU: 0 PID: 3807 Comm: modprobe Not tainted >> 6.1.0-rc1-00186-g76f33a7eedb4 #299 >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS >> rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org 04/01/2014 >> RIP: 0010:unregister_trace_event+0x6e/0x280 >> Code: 00 fc ff df 4c 89 ea 48 c1 ea 03 80 3c 02 00 0f 85 0e 02 00 00 48 >> b8 00 00 00 00 00 fc ff df 4c 8b 63 08 4c 89 e2 48 c1 ea 03 <80> 3c 02 >> 00 0f 85 e2 01 00 00 49 89 2c 24 48 85 ed 74 28 e8 7a 9b >> RSP: 0018:ffff88810413f370 EFLAGS: 00010a06 >> RAX: dffffc0000000000 RBX: ffff888105d050b0 RCX: 0000000000000000 >> RDX: 1bd5a00000000024 RSI: ffff888119e276e0 RDI: ffffffff835a8b20 >> RBP: dead000000000100 R08: 0000000000000000 R09: fffffbfff0913481 >> R10: ffffffff8489a407 R11: fffffbfff0913480 R12: dead000000000122 >> R13: ffff888105d050b8 R14: 0000000000000000 R15: ffff888105d05028 >> FS: 00007f7823e8d540(0000) GS:ffff888119e00000(0000) >> knlGS:0000000000000000 >> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> CR2: 00007f7823e7ebec CR3: 000000010a058002 CR4: 0000000000330ef0 >> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 >> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 >> Call Trace: >> <TASK> >> __create_synth_event+0x1e37/0x1eb0 >> create_or_delete_synth_event+0x110/0x250 >> synth_event_run_command+0x2f/0x110 >> test_gen_synth_cmd+0x170/0x2eb [synth_event_gen_test] >> synth_event_gen_test_init+0x76/0x9bc [synth_event_gen_test] >> do_one_initcall+0xdb/0x480 >> do_init_module+0x1cf/0x680 >> load_module+0x6a50/0x70a0 >> __do_sys_finit_module+0x12f/0x1c0 >> do_syscall_64+0x3f/0x90 >> entry_SYSCALL_64_after_hwframe+0x63/0xcd >> >> Fixes: 4b147936fa50 ("tracing: Add support for 'synthetic' events") >> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> >> Cc: stable@vger.kernel.org >> --- >> kernel/trace/trace_events_synth.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c >> index e310052dc83c..a51280a153e3 100644 >> --- a/kernel/trace/trace_events_synth.c >> +++ b/kernel/trace/trace_events_synth.c >> @@ -830,6 +830,8 @@ static int register_synth_event(struct synth_event *event) >> ret = set_synth_event_print_fmt(call); >> if (ret < 0) { >> trace_remove_event_call(call); >> + if (call->event.funcs) >> + return ret; >> goto err; >> } >> out: > > Good catch, thanks for finding this bug! > > It looks like call->event.funcs will always be true here since it's set > to &synth_event_funcs above. > > So it seems like you could just call trace_remove_event_call() and fall > through for this (ret < 0) case? If so, it might be good to put a > comment there noting that trace_remove_event_call() will call > unregister_trace_event(), so it's ok to just return. > Ok, will fix in v2. Thanks, -- Shang XiaoJing ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-11-13 6:49 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-08 8:31 [PATCH 0/2] tracing: Fix some bug about synth Shang XiaoJing 2022-11-08 8:31 ` [PATCH 1/2] tracing: Fix memory leak in test_gen_synth_cmd() and test_empty_synth_event() Shang XiaoJing 2022-11-12 21:12 ` Tom Zanussi 2022-11-13 6:48 ` shangxiaojing 2022-11-08 8:31 ` [PATCH 2/2] tracing: Fix wild-memory-access in register_synth_event() Shang XiaoJing 2022-11-12 21:24 ` Tom Zanussi 2022-11-13 6:49 ` shangxiaojing
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®