[PATCH 0/3] tracing: Fix some selftest issues

[PATCH 0/3] tracing: Fix some selftest issues

[Masami Hiramatsu (Google)]

Hi,

Here is v2 of a series of some fixes/cleanups for the test modules and
boot time selftest of kprobe events. The previous version is here;

https://lore.kernel.org/all/171671825710.39694.6859036369216249956.stgit@devnote2/

In this version, I just update the description of the first patch to add
what bad things happen when the modules are built in.

I found a WARNING message with some boot time selftest configuration, which
came from the combination of embedded kprobe generate API tests module and
ftrace boot-time selftest. Since kprobe and synthetic event generation API
test modules add new events and lock it. Thus dynamic event remove-all
operation failes. This also causes all ftracetest failed because it tries
to cleanup all dynamic events before running test cases.

The main problem is that these modules should not be built-in. But I also
think this WARNING message is useless (because there are warning messages
already) and the cleanup code is redundant. This series fixes those issues.

Thank you,

---

Masami Hiramatsu (Google) (3):
      tracing: Build event generation tests only as modules
      tracing/kprobe: Remove unneeded WARN_ON_ONCE() in selftests
      tracing/kprobe: Remove cleanup code unrelated to selftest


 kernel/trace/Kconfig        |    4 ++--
 kernel/trace/trace_kprobe.c |   29 ++++++++++++-----------------
 2 files changed, 14 insertions(+), 19 deletions(-)

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

[PATCH 1/3] tracing: Build event generation tests only as modules

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

The kprobes and synth event generation test modules add events and lock
(get a reference) those event file reference in module init function,
and unlock and delete it in module exit function. This is because those
are designed for playing as modules.

If we make those modules as built-in, those events are left locked in the
kernel, and never be removed. This causes kprobe event self-test failure
as below.

[   97.349708] ------------[ cut here ]------------
[   97.353453] WARNING: CPU: 3 PID: 1 at kernel/trace/trace_kprobe.c:2133 kprobe_trace_self_tests_init+0x3f1/0x480
[   97.357106] Modules linked in:
[   97.358488] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 6.9.0-g699646734ab5-dirty #14
[   97.361556] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
[   97.363880] RIP: 0010:kprobe_trace_self_tests_init+0x3f1/0x480
[   97.365538] Code: a8 24 08 82 e9 ae fd ff ff 90 0f 0b 90 48 c7 c7 e5 aa 0b 82 e9 ee fc ff ff 90 0f 0b 90 48 c7 c7 2d 61 06 82 e9 8e fd ff ff 90 <0f> 0b 90 48 c7 c7 33 0b 0c 82 89 c6 e8 6e 03 1f ff 41 ff c7 e9 90
[   97.370429] RSP: 0000:ffffc90000013b50 EFLAGS: 00010286
[   97.371852] RAX: 00000000fffffff0 RBX: ffff888005919c00 RCX: 0000000000000000
[   97.373829] RDX: ffff888003f40000 RSI: ffffffff8236a598 RDI: ffff888003f40a68
[   97.375715] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
[   97.377675] R10: ffffffff811c9ae5 R11: ffffffff8120c4e0 R12: 0000000000000000
[   97.379591] R13: 0000000000000001 R14: 0000000000000015 R15: 0000000000000000
[   97.381536] FS:  0000000000000000(0000) GS:ffff88807dcc0000(0000) knlGS:0000000000000000
[   97.383813] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   97.385449] CR2: 0000000000000000 CR3: 0000000002244000 CR4: 00000000000006b0
[   97.387347] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   97.389277] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   97.391196] Call Trace:
[   97.391967]  <TASK>
[   97.392647]  ? __warn+0xcc/0x180
[   97.393640]  ? kprobe_trace_self_tests_init+0x3f1/0x480
[   97.395181]  ? report_bug+0xbd/0x150
[   97.396234]  ? handle_bug+0x3e/0x60
[   97.397311]  ? exc_invalid_op+0x1a/0x50
[   97.398434]  ? asm_exc_invalid_op+0x1a/0x20
[   97.399652]  ? trace_kprobe_is_busy+0x20/0x20
[   97.400904]  ? tracing_reset_all_online_cpus+0x15/0x90
[   97.402304]  ? kprobe_trace_self_tests_init+0x3f1/0x480
[   97.403773]  ? init_kprobe_trace+0x50/0x50
[   97.404972]  do_one_initcall+0x112/0x240
[   97.406113]  do_initcall_level+0x95/0xb0
[   97.407286]  ? kernel_init+0x1a/0x1a0
[   97.408401]  do_initcalls+0x3f/0x70
[   97.409452]  kernel_init_freeable+0x16f/0x1e0
[   97.410662]  ? rest_init+0x1f0/0x1f0
[   97.411738]  kernel_init+0x1a/0x1a0
[   97.412788]  ret_from_fork+0x39/0x50
[   97.413817]  ? rest_init+0x1f0/0x1f0
[   97.414844]  ret_from_fork_asm+0x11/0x20
[   97.416285]  </TASK>
[   97.417134] irq event stamp: 13437323
[   97.418376] hardirqs last  enabled at (13437337): [<ffffffff8110bc0c>] console_unlock+0x11c/0x150
[   97.421285] hardirqs last disabled at (13437370): [<ffffffff8110bbf1>] console_unlock+0x101/0x150
[   97.423838] softirqs last  enabled at (13437366): [<ffffffff8108e17f>] handle_softirqs+0x23f/0x2a0
[   97.426450] softirqs last disabled at (13437393): [<ffffffff8108e346>] __irq_exit_rcu+0x66/0xd0
[   97.428850] ---[ end trace 0000000000000000 ]---

And also, since we can not cleanup dynamic_event file, ftracetest are
failed too.

To avoid these issues, build these tests only as modules.

Fixes: 9fe41efaca08 ("tracing: Add synth event generation test module")
Fixes: 64836248dda2 ("tracing: Add kprobe event command generation test module")
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/Kconfig |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 166ad5444eea..721c3b221048 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -1136,7 +1136,7 @@ config PREEMPTIRQ_DELAY_TEST
 
 config SYNTH_EVENT_GEN_TEST
 	tristate "Test module for in-kernel synthetic event generation"
-	depends on SYNTH_EVENTS
+	depends on SYNTH_EVENTS && m
 	help
           This option creates a test module to check the base
           functionality of in-kernel synthetic event definition and
@@ -1149,7 +1149,7 @@ config SYNTH_EVENT_GEN_TEST
 
 config KPROBE_EVENT_GEN_TEST
 	tristate "Test module for in-kernel kprobe event generation"
-	depends on KPROBE_EVENTS
+	depends on KPROBE_EVENTS && m
 	help
           This option creates a test module to check the base
           functionality of in-kernel kprobe event definition.

Re: [PATCH 1/3] tracing: Build event generation tests only as modules

[Steven Rostedt]

On Tue, 11 Jun 2024 06:26:34 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> The kprobes and synth event generation test modules add events and lock
> (get a reference) those event file reference in module init function,
> and unlock and delete it in module exit function. This is because those
> are designed for playing as modules.
> 
> If we make those modules as built-in, those events are left locked in the
> kernel, and never be removed. This causes kprobe event self-test failure
> as below.

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve

[PATCH 2/3] tracing/kprobe: Remove unneeded WARN_ON_ONCE() in selftests

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Since the kprobe-events selftest shows OK or NG with the reason, the
WARN_ON_ONCE()s for each place are redundant. Let's remove it.

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

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 16383247bdbf..4abed36544d0 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -2023,18 +2023,18 @@ static __init int kprobe_trace_self_tests_init(void)
 	pr_info("Testing kprobe tracing: ");
 
 	ret = create_or_delete_trace_kprobe("p:testprobe kprobe_trace_selftest_target $stack $stack0 +0($stack)");
-	if (WARN_ON_ONCE(ret)) {
+	if (ret) {
 		pr_warn("error on probing function entry.\n");
 		warn++;
 	} else {
 		/* Enable trace point */
 		tk = find_trace_kprobe("testprobe", KPROBE_EVENT_SYSTEM);
-		if (WARN_ON_ONCE(tk == NULL)) {
+		if (tk == NULL) {
 			pr_warn("error on getting new probe.\n");
 			warn++;
 		} else {
 			file = find_trace_probe_file(tk, top_trace_array());
-			if (WARN_ON_ONCE(file == NULL)) {
+			if (file == NULL) {
 				pr_warn("error on getting probe file.\n");
 				warn++;
 			} else
@@ -2044,18 +2044,18 @@ static __init int kprobe_trace_self_tests_init(void)
 	}
 
 	ret = create_or_delete_trace_kprobe("r:testprobe2 kprobe_trace_selftest_target $retval");
-	if (WARN_ON_ONCE(ret)) {
+	if (ret) {
 		pr_warn("error on probing function return.\n");
 		warn++;
 	} else {
 		/* Enable trace point */
 		tk = find_trace_kprobe("testprobe2", KPROBE_EVENT_SYSTEM);
-		if (WARN_ON_ONCE(tk == NULL)) {
+		if (tk == NULL) {
 			pr_warn("error on getting 2nd new probe.\n");
 			warn++;
 		} else {
 			file = find_trace_probe_file(tk, top_trace_array());
-			if (WARN_ON_ONCE(file == NULL)) {
+			if (file == NULL) {
 				pr_warn("error on getting probe file.\n");
 				warn++;
 			} else
@@ -2079,7 +2079,7 @@ static __init int kprobe_trace_self_tests_init(void)
 
 	/* Disable trace points before removing it */
 	tk = find_trace_kprobe("testprobe", KPROBE_EVENT_SYSTEM);
-	if (WARN_ON_ONCE(tk == NULL)) {
+	if (tk == NULL) {
 		pr_warn("error on getting test probe.\n");
 		warn++;
 	} else {
@@ -2089,7 +2089,7 @@ static __init int kprobe_trace_self_tests_init(void)
 		}
 
 		file = find_trace_probe_file(tk, top_trace_array());
-		if (WARN_ON_ONCE(file == NULL)) {
+		if (file == NULL) {
 			pr_warn("error on getting probe file.\n");
 			warn++;
 		} else
@@ -2098,7 +2098,7 @@ static __init int kprobe_trace_self_tests_init(void)
 	}
 
 	tk = find_trace_kprobe("testprobe2", KPROBE_EVENT_SYSTEM);
-	if (WARN_ON_ONCE(tk == NULL)) {
+	if (tk == NULL) {
 		pr_warn("error on getting 2nd test probe.\n");
 		warn++;
 	} else {
@@ -2108,7 +2108,7 @@ static __init int kprobe_trace_self_tests_init(void)
 		}
 
 		file = find_trace_probe_file(tk, top_trace_array());
-		if (WARN_ON_ONCE(file == NULL)) {
+		if (file == NULL) {
 			pr_warn("error on getting probe file.\n");
 			warn++;
 		} else
@@ -2117,20 +2117,20 @@ static __init int kprobe_trace_self_tests_init(void)
 	}
 
 	ret = create_or_delete_trace_kprobe("-:testprobe");
-	if (WARN_ON_ONCE(ret)) {
+	if (ret) {
 		pr_warn("error on deleting a probe.\n");
 		warn++;
 	}
 
 	ret = create_or_delete_trace_kprobe("-:testprobe2");
-	if (WARN_ON_ONCE(ret)) {
+	if (ret) {
 		pr_warn("error on deleting a probe.\n");
 		warn++;
 	}
 
 end:
 	ret = dyn_events_release_all(&trace_kprobe_ops);
-	if (WARN_ON_ONCE(ret)) {
+	if (ret) {
 		pr_warn("error on cleaning up probes.\n");
 		warn++;
 	}

Re: [PATCH 2/3] tracing/kprobe: Remove unneeded WARN_ON_ONCE() in selftests

[Steven Rostedt]

On Tue, 11 Jun 2024 06:26:44 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Since the kprobe-events selftest shows OK or NG with the reason, the
> WARN_ON_ONCE()s for each place are redundant. Let's remove it.

Note, the ktests we run to validate commits, fail when it detects a WARN()
triggered.

If this fails in any configuration, ktest will not detect it failed.

-- Steve


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

Re: [PATCH 2/3] tracing/kprobe: Remove unneeded WARN_ON_ONCE() in selftests

[Masami Hiramatsu]

On Mon, 10 Jun 2024 17:40:52 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 11 Jun 2024 06:26:44 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Since the kprobe-events selftest shows OK or NG with the reason, the
> > WARN_ON_ONCE()s for each place are redundant. Let's remove it.
> 
> Note, the ktests we run to validate commits, fail when it detects a WARN()
> triggered.
> 
> If this fails in any configuration, ktest will not detect it failed.

Hmm, I think there are 2 options,
 - remove pr_warn() instead. (WARN_ON_ONCE + pr_warn is redundant)
 - Or, remove WARN_ON_ONCE() from each place, but add WARN_ON_ONCE() when
   `warn` is not zero.

Thank you,

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


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

Re: [PATCH 2/3] tracing/kprobe: Remove unneeded WARN_ON_ONCE() in selftests

[Steven Rostedt]

On Tue, 11 Jun 2024 06:26:44 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Since the kprobe-events selftest shows OK or NG with the reason, the
> WARN_ON_ONCE()s for each place are redundant. Let's remove it.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_kprobe.c |   26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 16383247bdbf..4abed36544d0 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -2023,18 +2023,18 @@ static __init int kprobe_trace_self_tests_init(void)
>  	pr_info("Testing kprobe tracing: ");
>  
>  	ret = create_or_delete_trace_kprobe("p:testprobe kprobe_trace_selftest_target $stack $stack0 +0($stack)");
> -	if (WARN_ON_ONCE(ret)) {
> +	if (ret) {
>  		pr_warn("error on probing function entry.\n");

Actually, you can consolidate this to:

	if (WARN_ONCE(ret, "error on probing function entry."))

>  		warn++;
>  	} else {
>  		/* Enable trace point */
>  		tk = find_trace_kprobe("testprobe", KPROBE_EVENT_SYSTEM);
> -		if (WARN_ON_ONCE(tk == NULL)) {
> +		if (tk == NULL) {
>  			pr_warn("error on getting new probe.\n");

And this to:

		if (WARN_ONCE(tk == NULL, "error on getting new probe."))

end so on.

-- Steve

>  			warn++;
>  		} else {
>  			file = find_trace_probe_file(tk, top_trace_array());
> -			if (WARN_ON_ONCE(file == NULL)) {
> +			if (file == NULL) {
>  				pr_warn("error on getting probe file.\n");
>  				warn++;
>  			} else
> @@ -2044,18 +2044,18 @@ static __init int kprobe_trace_self_tests_init(void)
>  	}
>  
>  	ret = create_or_delete_trace_kprobe("r:testprobe2 kprobe_trace_selftest_target $retval");
> -	if (WARN_ON_ONCE(ret)) {
> +	if (ret) {
>  		pr_warn("error on probing function return.\n");
>  		warn++;
>  	} else {
>  		/* Enable trace point */
>  		tk = find_trace_kprobe("testprobe2", KPROBE_EVENT_SYSTEM);
> -		if (WARN_ON_ONCE(tk == NULL)) {
> +		if (tk == NULL) {
>  			pr_warn("error on getting 2nd new probe.\n");
>  			warn++;
>  		} else {
>  			file = find_trace_probe_file(tk, top_trace_array());
> -			if (WARN_ON_ONCE(file == NULL)) {
> +			if (file == NULL) {
>  				pr_warn("error on getting probe file.\n");
>  				warn++;
>  			} else
> @@ -2079,7 +2079,7 @@ static __init int kprobe_trace_self_tests_init(void)
>  
>  	/* Disable trace points before removing it */
>  	tk = find_trace_kprobe("testprobe", KPROBE_EVENT_SYSTEM);
> -	if (WARN_ON_ONCE(tk == NULL)) {
> +	if (tk == NULL) {
>  		pr_warn("error on getting test probe.\n");
>  		warn++;
>  	} else {
> @@ -2089,7 +2089,7 @@ static __init int kprobe_trace_self_tests_init(void)
>  		}
>  
>  		file = find_trace_probe_file(tk, top_trace_array());
> -		if (WARN_ON_ONCE(file == NULL)) {
> +		if (file == NULL) {
>  			pr_warn("error on getting probe file.\n");
>  			warn++;
>  		} else
> @@ -2098,7 +2098,7 @@ static __init int kprobe_trace_self_tests_init(void)
>  	}
>  
>  	tk = find_trace_kprobe("testprobe2", KPROBE_EVENT_SYSTEM);
> -	if (WARN_ON_ONCE(tk == NULL)) {
> +	if (tk == NULL) {
>  		pr_warn("error on getting 2nd test probe.\n");
>  		warn++;
>  	} else {
> @@ -2108,7 +2108,7 @@ static __init int kprobe_trace_self_tests_init(void)
>  		}
>  
>  		file = find_trace_probe_file(tk, top_trace_array());
> -		if (WARN_ON_ONCE(file == NULL)) {
> +		if (file == NULL) {
>  			pr_warn("error on getting probe file.\n");
>  			warn++;
>  		} else
> @@ -2117,20 +2117,20 @@ static __init int kprobe_trace_self_tests_init(void)
>  	}
>  
>  	ret = create_or_delete_trace_kprobe("-:testprobe");
> -	if (WARN_ON_ONCE(ret)) {
> +	if (ret) {
>  		pr_warn("error on deleting a probe.\n");
>  		warn++;
>  	}
>  
>  	ret = create_or_delete_trace_kprobe("-:testprobe2");
> -	if (WARN_ON_ONCE(ret)) {
> +	if (ret) {
>  		pr_warn("error on deleting a probe.\n");
>  		warn++;
>  	}
>  
>  end:
>  	ret = dyn_events_release_all(&trace_kprobe_ops);
> -	if (WARN_ON_ONCE(ret)) {
> +	if (ret) {
>  		pr_warn("error on cleaning up probes.\n");
>  		warn++;
>  	}

Re: [PATCH 2/3] tracing/kprobe: Remove unneeded WARN_ON_ONCE() in selftests

[Masami Hiramatsu]

On Mon, 10 Jun 2024 20:18:13 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 11 Jun 2024 06:26:44 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Since the kprobe-events selftest shows OK or NG with the reason, the
> > WARN_ON_ONCE()s for each place are redundant. Let's remove it.
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  kernel/trace/trace_kprobe.c |   26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 16383247bdbf..4abed36544d0 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -2023,18 +2023,18 @@ static __init int kprobe_trace_self_tests_init(void)
> >  	pr_info("Testing kprobe tracing: ");
> >  
> >  	ret = create_or_delete_trace_kprobe("p:testprobe kprobe_trace_selftest_target $stack $stack0 +0($stack)");
> > -	if (WARN_ON_ONCE(ret)) {
> > +	if (ret) {
> >  		pr_warn("error on probing function entry.\n");
> 
> Actually, you can consolidate this to:
> 
> 	if (WARN_ONCE(ret, "error on probing function entry."))

Ahh, OK, let me update it.

> 
> >  		warn++;
> >  	} else {
> >  		/* Enable trace point */
> >  		tk = find_trace_kprobe("testprobe", KPROBE_EVENT_SYSTEM);
> > -		if (WARN_ON_ONCE(tk == NULL)) {
> > +		if (tk == NULL) {
> >  			pr_warn("error on getting new probe.\n");
> 
> And this to:
> 
> 		if (WARN_ONCE(tk == NULL, "error on getting new probe."))

Thank you!

> 
> end so on.
> 
> -- Steve
> 
> >  			warn++;
> >  		} else {
> >  			file = find_trace_probe_file(tk, top_trace_array());
> > -			if (WARN_ON_ONCE(file == NULL)) {
> > +			if (file == NULL) {
> >  				pr_warn("error on getting probe file.\n");
> >  				warn++;
> >  			} else
> > @@ -2044,18 +2044,18 @@ static __init int kprobe_trace_self_tests_init(void)
> >  	}
> >  
> >  	ret = create_or_delete_trace_kprobe("r:testprobe2 kprobe_trace_selftest_target $retval");
> > -	if (WARN_ON_ONCE(ret)) {
> > +	if (ret) {
> >  		pr_warn("error on probing function return.\n");
> >  		warn++;
> >  	} else {
> >  		/* Enable trace point */
> >  		tk = find_trace_kprobe("testprobe2", KPROBE_EVENT_SYSTEM);
> > -		if (WARN_ON_ONCE(tk == NULL)) {
> > +		if (tk == NULL) {
> >  			pr_warn("error on getting 2nd new probe.\n");
> >  			warn++;
> >  		} else {
> >  			file = find_trace_probe_file(tk, top_trace_array());
> > -			if (WARN_ON_ONCE(file == NULL)) {
> > +			if (file == NULL) {
> >  				pr_warn("error on getting probe file.\n");
> >  				warn++;
> >  			} else
> > @@ -2079,7 +2079,7 @@ static __init int kprobe_trace_self_tests_init(void)
> >  
> >  	/* Disable trace points before removing it */
> >  	tk = find_trace_kprobe("testprobe", KPROBE_EVENT_SYSTEM);
> > -	if (WARN_ON_ONCE(tk == NULL)) {
> > +	if (tk == NULL) {
> >  		pr_warn("error on getting test probe.\n");
> >  		warn++;
> >  	} else {
> > @@ -2089,7 +2089,7 @@ static __init int kprobe_trace_self_tests_init(void)
> >  		}
> >  
> >  		file = find_trace_probe_file(tk, top_trace_array());
> > -		if (WARN_ON_ONCE(file == NULL)) {
> > +		if (file == NULL) {
> >  			pr_warn("error on getting probe file.\n");
> >  			warn++;
> >  		} else
> > @@ -2098,7 +2098,7 @@ static __init int kprobe_trace_self_tests_init(void)
> >  	}
> >  
> >  	tk = find_trace_kprobe("testprobe2", KPROBE_EVENT_SYSTEM);
> > -	if (WARN_ON_ONCE(tk == NULL)) {
> > +	if (tk == NULL) {
> >  		pr_warn("error on getting 2nd test probe.\n");
> >  		warn++;
> >  	} else {
> > @@ -2108,7 +2108,7 @@ static __init int kprobe_trace_self_tests_init(void)
> >  		}
> >  
> >  		file = find_trace_probe_file(tk, top_trace_array());
> > -		if (WARN_ON_ONCE(file == NULL)) {
> > +		if (file == NULL) {
> >  			pr_warn("error on getting probe file.\n");
> >  			warn++;
> >  		} else
> > @@ -2117,20 +2117,20 @@ static __init int kprobe_trace_self_tests_init(void)
> >  	}
> >  
> >  	ret = create_or_delete_trace_kprobe("-:testprobe");
> > -	if (WARN_ON_ONCE(ret)) {
> > +	if (ret) {
> >  		pr_warn("error on deleting a probe.\n");
> >  		warn++;
> >  	}
> >  
> >  	ret = create_or_delete_trace_kprobe("-:testprobe2");
> > -	if (WARN_ON_ONCE(ret)) {
> > +	if (ret) {
> >  		pr_warn("error on deleting a probe.\n");
> >  		warn++;
> >  	}
> >  
> >  end:
> >  	ret = dyn_events_release_all(&trace_kprobe_ops);
> > -	if (WARN_ON_ONCE(ret)) {
> > +	if (ret) {
> >  		pr_warn("error on cleaning up probes.\n");
> >  		warn++;
> >  	}
> 


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

[PATCH 3/3] tracing/kprobe: Remove cleanup code unrelated to selftest

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

This cleanup all kprobe events code is not related to the selftest
itself, and it can fail by the reason unrelated to this test.
If the test is successful, the generated events are cleaned up.
And if not, we cannot guarantee that the kprobe events will work
correctly. So, anyway, there is no need to clean it up.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_kprobe.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 4abed36544d0..f94628c15c14 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -2129,11 +2129,6 @@ static __init int kprobe_trace_self_tests_init(void)
 	}
 
 end:
-	ret = dyn_events_release_all(&trace_kprobe_ops);
-	if (ret) {
-		pr_warn("error on cleaning up probes.\n");
-		warn++;
-	}
 	/*
 	 * Wait for the optimizer work to finish. Otherwise it might fiddle
 	 * with probes in already freed __init text.

Re: [PATCH 3/3] tracing/kprobe: Remove cleanup code unrelated to selftest

[Steven Rostedt]

On Tue, 11 Jun 2024 06:26:53 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> This cleanup all kprobe events code is not related to the selftest
> itself, and it can fail by the reason unrelated to this test.
> If the test is successful, the generated events are cleaned up.
> And if not, we cannot guarantee that the kprobe events will work
> correctly. So, anyway, there is no need to clean it up.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve

[PATCH 0/3] tracing: Fix some selftest issues

[Masami Hiramatsu (Google)]

Hi,

Here is a series of some fixes/improvements for the test modules and boot
time selftest of kprobe events. I found a WARNING message with some boot 
time selftest configuration, which came from the combination of embedded
kprobe generate API tests module and ftrace boot-time selftest. So the main
problem is that the test module should not be built-in. But I also think
this WARNING message is useless (because there are warning messages already)
and the cleanup code is redundant. This series fixes those issues.

Thank you,

---

Masami Hiramatsu (Google) (3):
      tracing: Build event generation tests only as modules
      tracing/kprobe: Remove unneeded WARN_ON_ONCE() in selftests
      tracing/kprobe: Remove cleanup code unrelated to selftest


 kernel/trace/Kconfig        |    4 ++--
 kernel/trace/trace_kprobe.c |   29 ++++++++++++-----------------
 2 files changed, 14 insertions(+), 19 deletions(-)

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

[PATCH 1/3] tracing: Build event generation tests only as modules

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Since the kprobes and synth event generation tests adds and enable
generated events in init_module() and delete it in exit_module(),
if we make it as built-in, those events are left in kernel and cause
kprobe event self-test failure.

[   97.349708] ------------[ cut here ]------------
[   97.353453] WARNING: CPU: 3 PID: 1 at kernel/trace/trace_kprobe.c:2133 kprobe_trace_self_tests_init+0x3f1/0x480
[   97.357106] Modules linked in:
[   97.358488] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 6.9.0-g699646734ab5-dirty #14
[   97.361556] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
[   97.363880] RIP: 0010:kprobe_trace_self_tests_init+0x3f1/0x480
[   97.365538] Code: a8 24 08 82 e9 ae fd ff ff 90 0f 0b 90 48 c7 c7 e5 aa 0b 82 e9 ee fc ff ff 90 0f 0b 90 48 c7 c7 2d 61 06 82 e9 8e fd ff ff 90 <0f> 0b 90 48 c7 c7 33 0b 0c 82 89 c6 e8 6e 03 1f ff 41 ff c7 e9 90
[   97.370429] RSP: 0000:ffffc90000013b50 EFLAGS: 00010286
[   97.371852] RAX: 00000000fffffff0 RBX: ffff888005919c00 RCX: 0000000000000000
[   97.373829] RDX: ffff888003f40000 RSI: ffffffff8236a598 RDI: ffff888003f40a68
[   97.375715] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
[   97.377675] R10: ffffffff811c9ae5 R11: ffffffff8120c4e0 R12: 0000000000000000
[   97.379591] R13: 0000000000000001 R14: 0000000000000015 R15: 0000000000000000
[   97.381536] FS:  0000000000000000(0000) GS:ffff88807dcc0000(0000) knlGS:0000000000000000
[   97.383813] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   97.385449] CR2: 0000000000000000 CR3: 0000000002244000 CR4: 00000000000006b0
[   97.387347] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   97.389277] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   97.391196] Call Trace:
[   97.391967]  <TASK>
[   97.392647]  ? __warn+0xcc/0x180
[   97.393640]  ? kprobe_trace_self_tests_init+0x3f1/0x480
[   97.395181]  ? report_bug+0xbd/0x150
[   97.396234]  ? handle_bug+0x3e/0x60
[   97.397311]  ? exc_invalid_op+0x1a/0x50
[   97.398434]  ? asm_exc_invalid_op+0x1a/0x20
[   97.399652]  ? trace_kprobe_is_busy+0x20/0x20
[   97.400904]  ? tracing_reset_all_online_cpus+0x15/0x90
[   97.402304]  ? kprobe_trace_self_tests_init+0x3f1/0x480
[   97.403773]  ? init_kprobe_trace+0x50/0x50
[   97.404972]  do_one_initcall+0x112/0x240
[   97.406113]  do_initcall_level+0x95/0xb0
[   97.407286]  ? kernel_init+0x1a/0x1a0
[   97.408401]  do_initcalls+0x3f/0x70
[   97.409452]  kernel_init_freeable+0x16f/0x1e0
[   97.410662]  ? rest_init+0x1f0/0x1f0
[   97.411738]  kernel_init+0x1a/0x1a0
[   97.412788]  ret_from_fork+0x39/0x50
[   97.413817]  ? rest_init+0x1f0/0x1f0
[   97.414844]  ret_from_fork_asm+0x11/0x20
[   97.416285]  </TASK>
[   97.417134] irq event stamp: 13437323
[   97.418376] hardirqs last  enabled at (13437337): [<ffffffff8110bc0c>] console_unlock+0x11c/0x150
[   97.421285] hardirqs last disabled at (13437370): [<ffffffff8110bbf1>] console_unlock+0x101/0x150
[   97.423838] softirqs last  enabled at (13437366): [<ffffffff8108e17f>] handle_softirqs+0x23f/0x2a0
[   97.426450] softirqs last disabled at (13437393): [<ffffffff8108e346>] __irq_exit_rcu+0x66/0xd0
[   97.428850] ---[ end trace 0000000000000000 ]---

To avoid this issue, build these tests only as modules.

Fixes: 9fe41efaca08 ("tracing: Add synth event generation test module")
Fixes: 64836248dda2 ("tracing: Add kprobe event command generation test module")
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/Kconfig |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 166ad5444eea..721c3b221048 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -1136,7 +1136,7 @@ config PREEMPTIRQ_DELAY_TEST
 
 config SYNTH_EVENT_GEN_TEST
 	tristate "Test module for in-kernel synthetic event generation"
-	depends on SYNTH_EVENTS
+	depends on SYNTH_EVENTS && m
 	help
           This option creates a test module to check the base
           functionality of in-kernel synthetic event definition and
@@ -1149,7 +1149,7 @@ config SYNTH_EVENT_GEN_TEST
 
 config KPROBE_EVENT_GEN_TEST
 	tristate "Test module for in-kernel kprobe event generation"
-	depends on KPROBE_EVENTS
+	depends on KPROBE_EVENTS && m
 	help
           This option creates a test module to check the base
           functionality of in-kernel kprobe event definition.