Skip to contentSkip to conversation

tracing: Fixes for 5.15

PATCH New Steven Rostedt 1 participant 7 messages Oct 16, 2021

Subsystems (3)
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

cover tracing: Fixes for 5.15

Tracing fixes for 5.15:

- Fix defined but not use warning/error for osnoise function

- Fix memory leak in event probe

- Fix memblock leak in bootconfig

- Fix the API of event probes to be like kprobes

- Added test to check removal of event probe API

- Fix recordmcount.pl for nds32 failed build

Colin Ian King (1):
tracing: Fix missing * in comment block

Jackie Liu (1):
tracing: Fix missing osnoise tracer on max_latency

Masami Hiramatsu (1):
bootconfig: init: Fix memblock leak in xbc_make_cmdline()

Steven Rostedt (1):
nds32/ftrace: Fix Error: invalid operands (*UND* and *UND* sections) for `^'

Steven Rostedt (VMware) (2):
tracing: Fix event probe removal from dynamic events selftests/ftrace: Update test for more eprobe removal process

Vamshi K Sthambamkadi (1):
tracing: Fix memory leak in eprobe_register()

----

 init/main.c                                        |  1 +
 kernel/trace/trace.c                               | 11 ++--
 kernel/trace/trace_eprobe.c                        | 61 ++++++++++++++++++++--
 kernel/trace/trace_events_hist.c                   |  2 +-
 scripts/recordmcount.pl                            |  2 +-
 .../ftrace/test.d/dynevent/add_remove_eprobe.tc    | 54 ++++++++++++++++++-
 6 files changed, 117 insertions(+), 14 deletions(-)
author maintainer
#1 lore raw

PATCH 2/7 tracing: Fix memory leak in eprobe_register()

From: Vamshi K Sthambamkadi <[email protected]>

kmemleak report:
unreferenced object 0xffff900a70ec7ec0 (size 32):
comm "ftracetest", pid 2770, jiffies 4295042510 (age 311.464s) hex dump (first 32 bytes):

    c8 31 23 45 0a 90 ff ff 40 85 c7 6e 0a 90 ff ff  .1#[email protected]....
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<000000009d3751fd>] kmem_cache_alloc_trace+0x2a2/0x440
    [<0000000088b8124b>] eprobe_register+0x1e3/0x350
    [<000000002a9a0517>] __ftrace_event_enable_disable+0x7c/0x240
    [<0000000019109321>] event_enable_write+0x93/0xe0
    [<000000007d85b320>] vfs_write+0xb9/0x260
    [<00000000b94c5e41>] ksys_write+0x67/0xe0
    [<000000005a08c81d>] __x64_sys_write+0x1a/0x20
    [<00000000240bf576>] do_syscall_64+0x3b/0xc0
    [<0000000043d5d9f6>] entry_SYSCALL_64_after_hwframe+0x44/0xae

unreferenced object 0xffff900a56bbf280 (size 128):
comm "ftracetest", pid 2770, jiffies 4295042510 (age 311.464s) hex dump (first 32 bytes):

    ff ff ff ff ff ff ff ff 00 00 00 00 01 00 00 00  ................
    80 69 3b b2 ff ff ff ff 20 69 3b b2 ff ff ff ff  .i;..... i;.....
  backtrace:
    [<000000009d3751fd>] kmem_cache_alloc_trace+0x2a2/0x440
    [<00000000c4e90fad>] eprobe_register+0x1fc/0x350
    [<000000002a9a0517>] __ftrace_event_enable_disable+0x7c/0x240
    [<0000000019109321>] event_enable_write+0x93/0xe0
    [<000000007d85b320>] vfs_write+0xb9/0x260
    [<00000000b94c5e41>] ksys_write+0x67/0xe0
    [<000000005a08c81d>] __x64_sys_write+0x1a/0x20
    [<00000000240bf576>] do_syscall_64+0x3b/0xc0
    [<0000000043d5d9f6>] entry_SYSCALL_64_after_hwframe+0x44/0xae

In new_eprobe_trigger(), allocated edata and trigger variables are never freed.

To fix, free memory in disable_eprobe().

Fixes 7491e2c442781 ("tracing: Add a probe that attaches to trace events")
---
 kernel/trace/trace_eprobe.c | 7 +++++++
 1 file changed, 7 insertions(+)
kernel/trace/trace_eprobe.c +7 -0
index 3044b762cbd7..570d081929fb 100644
@@ -632,6 +632,13 @@ static int disable_eprobe(struct trace_eprobe *ep,
 
 	trace_event_trigger_enable_disable(file, 0);
 	update_cond_flag(file);
+
+	/* Make sure nothing is using the edata or trigger */
+	tracepoint_synchronize_unregister();
+
+	kfree(edata);
+	kfree(trigger);
+
 	return 0;
 }
 
Signature · 1 line
2.32.0
author maintainer
#2 lore raw

PATCH 3/7 bootconfig: init: Fix memblock leak in xbc_make_cmdline()

In reply to Steven Rostedt

From: Masami Hiramatsu <[email protected]>

Free unused memblock in a error case to fix memblock leak in xbc_make_cmdline().

Fixes 51887d03aca1 ("bootconfig: init: Allow admin to use bootconfig for kernel command line")
---
 init/main.c | 1 +
 1 file changed, 1 insertion(+)
init/main.c +1 -0
index 81a79a77db46..3c4054a95545 100644
@@ -382,6 +382,7 @@ static char * __init xbc_make_cmdline(const char *key)
 	ret = xbc_snprint_cmdline(new_cmdline, len + 1, root);
 	if (ret < 0 || ret > len) {
 		pr_err("Failed to print extra kernel cmdline.\n");
+		memblock_free_ptr(new_cmdline, len + 1);
 		return NULL;
 	}
 
Signature · 1 line
2.32.0
author maintainer
#3 lore raw

PATCH 4/7 tracing: Fix missing * in comment block

In reply to Steven Rostedt

From: Colin Ian King <[email protected]>

There is a missing * in a comment block, add it in.

---
 kernel/trace/trace_events_hist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/trace_events_hist.c +1 -1
index a6061a69aa84..f01e442716e2 100644
@@ -2506,7 +2506,7 @@ find_synthetic_field_var(struct hist_trigger_data *target_hist_data,
  * events.  However, for convenience, users are allowed to directly
  * specify an event field in an action, which will be automatically
  * converted into a variable on their behalf.
-
+ *
  * If a user specifies a field on an event that isn't the event the
  * histogram currently being defined (the target event histogram), the
  * only way that can be accomplished is if a new hist trigger is
Signature · 1 line
2.32.0
author maintainer
#4 lore raw

PATCH 5/7 tracing: Fix event probe removal from dynamic events

In reply to Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

When an event probe is to be removed via the API that created it via the dynamic events, an -ENOENT error is returned.

This is because the removal of the event probe does not expect to see the event system and name that the event probe is attached to, even though that's part of the API to create it. As the removal of probes is to use the same API as they are created.

In fact, the removal is not consistent with the kprobes and uprobes removal. Fix that by allowing various ways to remove the eprobe.

The eprobe is created with:

e:[GROUP/]NAME SYSTEM/EVENT [OPTIONS]

Have it get removed by echoing in the following into dynamic_events:

# Remove all eprobes with NAME
echo '-:NAME' >> dynamic_events

# Remove a specific eprobe
echo '-:GROUP/NAME' >> dynamic_events
echo '-:GROUP/NAME SYSTEM/EVENT' >> dynamic_events echo '-:NAME SYSTEM/EVENT' >> dynamic_events
echo '-:GROUP/NAME SYSTEM/EVENT OPTIONS' >> dynamic_events echo '-:NAME SYSTEM/EVENT OPTIONS' >> dynamic_events

Fixes 7491e2c442781 ("tracing: Add a probe that attaches to trace events")
---
 kernel/trace/trace_eprobe.c | 54 ++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 3 deletions(-)
kernel/trace/trace_eprobe.c +51 -3
index 570d081929fb..c4a15aef36af 100644
@@ -119,10 +119,58 @@ static bool eprobe_dyn_event_match(const char *system, const char *event,
 			int argc, const char **argv, struct dyn_event *ev)
 {
 	struct trace_eprobe *ep = to_trace_eprobe(ev);
+	const char *slash;
 
-	return strcmp(trace_probe_name(&ep->tp), event) == 0 &&
-	    (!system || strcmp(trace_probe_group_name(&ep->tp), system) == 0) &&
-	    trace_probe_match_command_args(&ep->tp, argc, argv);
+	/*
+	 * We match the following:
+	 *  event only			- match all eprobes with event name
+	 *  system and event only	- match all system/event probes
+	 *
+	 * The below has the above satisfied with more arguments:
+	 *
+	 *  attached system/event	- If the arg has the system and event
+	 *				  the probe is attached to, match
+	 *				  probes with the attachment.
+	 *
+	 *  If any more args are given, then it requires a full match.
+	 */
+
+	/*
+	 * If system exists, but this probe is not part of that system
+	 * do not match.
+	 */
+	if (system && strcmp(trace_probe_group_name(&ep->tp), system) != 0)
+		return false;
+
+	/* Must match the event name */
+	if (strcmp(trace_probe_name(&ep->tp), event) != 0)
+		return false;
+
+	/* No arguments match all */
+	if (argc < 1)
+		return true;
+
+	/* First argument is the system/event the probe is attached to */
+
+	slash = strchr(argv[0], '/');
+	if (!slash)
+		slash = strchr(argv[0], '.');
+	if (!slash)
+		return false;
+
+	if (strncmp(ep->event_system, argv[0], slash - argv[0]))
+		return false;
+	if (strcmp(ep->event_name, slash + 1))
+		return false;
+
+	argc--;
+	argv++;
+
+	/* If there are no other args, then match */
+	if (argc < 1)
+		return true;
+
+	return trace_probe_match_command_args(&ep->tp, argc, argv);
 }
 
 static struct dyn_event_operations eprobe_dyn_event_ops = {
Signature · 1 line
2.32.0
author maintainer
#5 lore raw

PATCH 6/7 selftests/ftrace: Update test for more eprobe removal process

In reply to Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

The removal of eprobes was broken and missed in testing. Add various ways to remove eprobes that are considered acceptable to the testing process to catch when/if they break again.

---
 .../test.d/dynevent/add_remove_eprobe.tc      | 54 ++++++++++++++++++-
 1 file changed, 52 insertions(+), 2 deletions(-)
tools/testing/selftests/ftrace/test.d/dynevent/add_remove_eprobe.tc +52 -2
index 5f5b2ba3e557..60c02b482be8 100644
@@ -11,8 +11,8 @@ SYSTEM="syscalls"
 EVENT="sys_enter_openat"
 FIELD="filename"
 EPROBE="eprobe_open"
-
-echo "e:$EPROBE $SYSTEM/$EVENT file=+0(\$filename):ustring" >> dynamic_events
+OPTIONS="file=+0(\$filename):ustring"
+echo "e:$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
 
 grep -q "$EPROBE" dynamic_events
 test -d events/eprobes/$EPROBE
@@ -37,4 +37,54 @@ echo "-:$EPROBE" >> dynamic_events
 ! grep -q "$EPROBE" dynamic_events
 ! test -d events/eprobes/$EPROBE
 
+# test various ways to remove the probe (already tested with just event name)
+
+# With group name
+echo "e:$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+grep -q "$EPROBE" dynamic_events
+test -d events/eprobes/$EPROBE
+echo "-:eprobes/$EPROBE" >> dynamic_events
+! grep -q "$EPROBE" dynamic_events
+! test -d events/eprobes/$EPROBE
+
+# With group name and system/event
+echo "e:$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+grep -q "$EPROBE" dynamic_events
+test -d events/eprobes/$EPROBE
+echo "-:eprobes/$EPROBE $SYSTEM/$EVENT" >> dynamic_events
+! grep -q "$EPROBE" dynamic_events
+! test -d events/eprobes/$EPROBE
+
+# With just event name and system/event
+echo "e:$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+grep -q "$EPROBE" dynamic_events
+test -d events/eprobes/$EPROBE
+echo "-:$EPROBE $SYSTEM/$EVENT" >> dynamic_events
+! grep -q "$EPROBE" dynamic_events
+! test -d events/eprobes/$EPROBE
+
+# With just event name and system/event and options
+echo "e:$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+grep -q "$EPROBE" dynamic_events
+test -d events/eprobes/$EPROBE
+echo "-:$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+! grep -q "$EPROBE" dynamic_events
+! test -d events/eprobes/$EPROBE
+
+# With group name and system/event and options
+echo "e:$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+grep -q "$EPROBE" dynamic_events
+test -d events/eprobes/$EPROBE
+echo "-:eprobes/$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+! grep -q "$EPROBE" dynamic_events
+! test -d events/eprobes/$EPROBE
+
+# Finally make sure what is in the dynamic_events file clears it too
+echo "e:$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+LINE=`sed -e '/$EPROBE/s/^e/-/' < dynamic_events`
+test -d events/eprobes/$EPROBE
+echo "-:eprobes/$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+! grep -q "$EPROBE" dynamic_events
+! test -d events/eprobes/$EPROBE
+
 clear_trace
Signature · 1 line
2.32.0
author maintainer
#6 lore raw

PATCH 7/7 nds32/ftrace: Fix Error: invalid operands (*UND* and *UND* sections) for `^

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

I received a build failure for a new patch I'm working on the nds32 architecture, and when I went to test it, I couldn't get to my build error, because it failed to build with a bunch of:

Error: invalid operands (*UND* and *UND* sections) for `^'

issues with various files. Those files were temporary asm files that looked
like: kernel/.tmp_mc_fork.s

I decided to look deeper, and found that the "mc" portion of that name stood for "mcount", and was created by the recordmcount.pl script. One that I wrote over a decade ago. Once I knew the source of the problem, I was able to investigate it further.

The way the recordmcount.pl script works (BTW, there's a C version that simply modifies the ELF object) is by doing an "objdump" on the object file. Looks for all the calls to "mcount", and creates an offset of those locations from some global variable it can use (usually a global function name, found with <.*>:). Creates a asm file that is a table of references to these locations, using the found variable/function. Compiles it and links it back into the original object file. This asm file is called ".tmp_mc_<object_base_name>.s".

The problem here is that the objdump produced by the nds32 object file, contains things that look like:

0000159a <.L3^B1>:

    159a:       c6 00           beqz38 $r6, 159a <.L3^B1>
                        159a: R_NDS32_9_PCREL_RELA      .text+0x159e
    159c:       84 d2           movi55 $r6, #-14
    159e:       80 06           mov55 $r0, $r6
    15a0:       ec 3c           addi10.sp #0x3c

Where ".L3^B1 is somehow selected as the "global" variable to index off of.

Then the assembly file that holds the mcount locations looks like this:

        .section __mcount_loc,"a",@progbits
        .align 2
        .long .L3^B1 + -5522
        .long .L3^B1 + -5384
        .long .L3^B1 + -5270
        .long .L3^B1 + -5098
        .long .L3^B1 + -4970
        .long .L3^B1 + -4758
        .long .L3^B1 + -4122
        [...]

And when it is compiled back to an object to link to the original object, the compile fails on the "^" symbol.

Simple solution for now, is to have the perl script ignore using function symbols that have an "^" in the name.

Fixes fbf58a52ac088 ("nds32/ftrace: Add RECORD_MCOUNT support")
---
 scripts/recordmcount.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
scripts/recordmcount.pl +1 -1
index 8f6b13ae46bf..7d631aaa0ae1 100755
@@ -189,7 +189,7 @@ if ($arch =~ /(x86(_64)?)|(i386)/) {
 $local_regex = "^[0-9a-fA-F]+\\s+t\\s+(\\S+)";
 $weak_regex = "^[0-9a-fA-F]+\\s+([wW])\\s+(\\S+)";
 $section_regex = "Disassembly of section\\s+(\\S+):";
-$function_regex = "^([0-9a-fA-F]+)\\s+<(.*?)>:";
+$function_regex = "^([0-9a-fA-F]+)\\s+<([^^]*?)>:";
 $mcount_regex = "^\\s*([0-9a-fA-F]+):.*\\s(mcount|__fentry__)\$";
 $section_type = '@progbits';
 $mcount_adjust = 0;
Signature · 1 line
2.32.0