tracing: Last minute fixes and updates for 5.19
Subsystems (9)
Steven Rostedt
maintainercover tracing: Last minute fixes and updates for 5.19
Actually, a lot of these changes were in my queue, I just haven't tested them. But they are mostly fixes and clean ups. No real functional features.
My tests on these have not completely finished, so I may have to rebase. But since this is half way into the merge window, I want these in linux-next for a little bit before sending my pull request.
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
for-next-core
Head SHA1: 8e929948ac1f0bd2655b4776fbdc6b529f123350
Congyu Liu (1):
tracing: Disable kcov on trace_preemptirq.c
Daniel Bristot de Oliveira (3):
tracing/timerlat: Notify IRQ new max latency only if stop tracing is set tracing/timerlat: Print stacktrace in the IRQ handler if needed tracing/timerlat: Do not wakeup the thread if the trace stops at the IRQ
Gautam Menghani (1):
tracing: Initialize integer variable to prevent garbage return value
Julia Lawall (1):
ftrace: Fix typo in comment
Keita Suzuki (1):
tracing: Fix potential double free in create_var_ref()
Li Huafei (1):
tracing: Reset the function filter after completing trampoline/graph selftest
Li kunyu (1):
ftrace: Remove return value of ftrace_arch_modify_*()
Masami Hiramatsu (1):
kprobes: Fix build errors with CONFIG_KRETPROBES=n
Song Liu (1):
ftrace: Clean up hash direct_functions on register failures
Steven Rostedt (Google) (2):
tracing: Have event format check not flag %p* on __get_dynamic_array()
ftrace: Add FTRACE_MCOUNT_MAX_OFFSET to avoid adding weak function
Wonhyuk Yang (1):
tracing: Fix return value of trace_pid_write()
Yuntao Wang (3):
tracing: Use trace_create_file() to simplify creation of tracefs entries
ftrace: Deal with error return code of the ftrace_process_locs() function
tracing: Use strim() to remove whitespace instead of doing it manually
liqiong (2):
tracing: Change "char *" string form to "char []"
tracing: Cleanup code by removing init "char *name"
sunliming (4):
tracing: Fix comments of create_filter() x86,tracing: Remove unused headers x86/traceponit: Fix comment about irq vector tracepoints
tracing: Fix comments for event_trigger_separate_filter()
----
Documentation/trace/timerlat-tracer.rst | 5 +- arch/arm/kernel/ftrace.c | 6 +- arch/riscv/kernel/ftrace.c | 6 +- arch/s390/kernel/ftrace.c | 3 +- arch/x86/include/asm/ftrace.h | 7 ++ arch/x86/kernel/ftrace.c | 6 +- arch/x86/kernel/tracepoint.c | 6 -- include/linux/ftrace.h | 4 +- include/linux/kprobes.h | 2 +- kernel/kprobes.c | 144 ++++++++++++------------- kernel/trace/Makefile | 4 + kernel/trace/ftrace.c | 186 +++++++++++++++++++++++++++----- kernel/trace/trace.c | 20 ++-- kernel/trace/trace_boot.c | 2 +- kernel/trace/trace_dynevent.c | 9 +- kernel/trace/trace_events.c | 42 +++----- kernel/trace/trace_events_filter.c | 2 +- kernel/trace/trace_events_hist.c | 5 +- kernel/trace/trace_events_trigger.c | 8 +- kernel/trace/trace_kprobe.c | 15 +-- kernel/trace/trace_osnoise.c | 22 +++- kernel/trace/trace_recursion_record.c | 7 +- kernel/trace/trace_selftest.c | 3 + 23 files changed, 319 insertions(+), 195 deletions(-)
Steven Rostedt
maintainerPATCH 1/23 tracing: Have event format check not flag %p* on __get_dynamic_array()
From: "Steven Rostedt (Google)" <[email protected]>
The print fmt check against trace events to make sure that the format does not use pointers that may be freed from the time of the trace to the time the event is read, gives a false positive on %pISpc when reading data that was saved in __get_dynamic_array() when it is perfectly fine to do so, as the data being read is on the ring buffer.
--- kernel/trace/trace_events.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
kernel/trace/trace_events.c +7 -6
@@ -392,12 +392,6 @@ static void test_event_printk(struct trace_event_call *call)if (!(dereference_flags & (1ULL << arg))) goto next_arg; -/* Check for __get_sockaddr */ ; -if (str_has_prefix (fmt + i,"__get_sockaddr(" )) { - dereference_flags &= ~(1ULL << arg); - goto next_arg; - } -/* Find the REC-> in the argument */ c =strchr (fmt + i,',' ); r =strstr (fmt + i,"REC->" );
@@ -413,7 +407,14 @@ static void test_event_printk(struct trace_event_call *call) a =strchr (fmt + i,'&' );if ((a && (a < r)) ||test_field (r, call)) dereference_flags &= ~(1ULL << arg); + }else if ((r =strstr (fmt + i,"__get_dynamic_array(" )) && + (!c || r < c)) { + dereference_flags &= ~(1ULL << arg); + }else if ((r =strstr (fmt + i,"__get_sockaddr(" )) && + (!c || r < c)) { + dereference_flags &= ~(1ULL << arg); } + next_arg: i--; arg++;
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 2/23 tracing: Reset the function filter after completing trampoline/graph selftest
In reply to Steven RostedtFrom: Li Huafei <[email protected]>
The direct trampoline and graph coexistence test sets global_ops to trace only 'trace_selftest_dynamic_test_func', but does not reset it after the test is completed, resulting in the function filter being set already after the system starts. Although it can be reset through the tracefs interface, it is more or less confusing to the user, and we should reset it to trace all functions after the trampoline/graph test completes.
--- kernel/trace/trace_selftest.c | 3 +++ 1 file changed, 3 insertions(+)
kernel/trace/trace_selftest.c +3 -0
@@ -895,6 +895,9 @@ trace_selftest_startup_function_graph(struct tracer *trace, ret =-1 ; goto out; } + +/* Enable tracing on all functions again */ +ftrace_set_global_filter (NULL ,0 ,1 );#endif /* Don't test dynamic tracing, the function tracer already did */
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 3/23 tracing: Use trace_create_file() to simplify creation of tracefs entries
In reply to Steven RostedtFrom: Yuntao Wang <[email protected]>
Creating tracefs entries with tracefs_create_file() followed by pr_warn() is tedious and repetitive, we can use trace_create_file() to simplify this process and make the code more readable.
--- kernel/trace/ftrace.c | 9 +++------ kernel/trace/trace_dynevent.c | 9 ++------- kernel/trace/trace_events.c | 29 +++++++++------------------ kernel/trace/trace_kprobe.c | 15 ++++---------- kernel/trace/trace_recursion_record.c | 7 ++----- 5 files changed, 21 insertions(+), 48 deletions(-)
kernel/trace/ftrace.c +3 -6
@@ -952,7 +952,6 @@ static struct tracer_stat function_stats __initdata = {static __init void ftrace_profile_tracefs (struct dentry *d_tracer) {struct ftrace_profile_stat *stat; -struct dentry *entry;char *name;int ret;int cpu;
@@ -983,11 +982,9 @@ static __init void ftrace_profile_tracefs(struct dentry *d_tracer) } } - entry =tracefs_create_file ("function_profile_enabled" , -TRACE_MODE_WRITE , d_tracer,NULL , - &ftrace_profile_fops); -if (!entry) -pr_warn ("Could not create tracefs 'function_profile_enabled' entry\n" ); +trace_create_file ("function_profile_enabled" , +TRACE_MODE_WRITE , d_tracer,NULL , + &ftrace_profile_fops); }#else /* CONFIG_FUNCTION_PROFILER */
kernel/trace/trace_dynevent.c +2 -7
@@ -255,19 +255,14 @@ static const struct file_operations dynamic_events_ops = {/* Make a tracefs interface for controlling dynamic events */ static __init int init_dynamic_event (void ) { -struct dentry *entry;int ret; ret =tracing_init_dentry ();if (ret)return 0 ; - entry =tracefs_create_file ("dynamic_events" ,TRACE_MODE_WRITE ,NULL , -NULL , &dynamic_events_ops); - -/* Event list interface */ -if (!entry) -pr_warn ("Could not create tracefs 'dynamic_events' entry\n" ); +trace_create_file ("dynamic_events" ,TRACE_MODE_WRITE ,NULL , +NULL , &dynamic_events_ops);return 0 ; }
kernel/trace/trace_events.c +10 -19
@@ -3546,12 +3546,10 @@ create_event_toplevel_files(struct dentry *parent, struct trace_array *tr)struct dentry *d_events;struct dentry *entry; - entry =tracefs_create_file ("set_event" ,TRACE_MODE_WRITE , parent, - tr, &ftrace_set_event_fops); -if (!entry) { -pr_warn ("Could not create tracefs 'set_event' entry\n" ); + entry =trace_create_file ("set_event" ,TRACE_MODE_WRITE , parent, + tr, &ftrace_set_event_fops); +if (!entry)return -ENOMEM ; - } d_events =tracefs_create_dir ("events" , parent);if (!d_events) {
@@ -3566,16 +3564,12 @@ create_event_toplevel_files(struct dentry *parent, struct trace_array *tr)/* There are not as crucial, just warn if they are not created */ - entry =tracefs_create_file ("set_event_pid" ,TRACE_MODE_WRITE , parent, - tr, &ftrace_set_event_pid_fops); -if (!entry) -pr_warn ("Could not create tracefs 'set_event_pid' entry\n" ); +trace_create_file ("set_event_pid" ,TRACE_MODE_WRITE , parent, + tr, &ftrace_set_event_pid_fops); - entry =tracefs_create_file ("set_event_notrace_pid" , -TRACE_MODE_WRITE , parent, tr, - &ftrace_set_event_notrace_pid_fops); -if (!entry) -pr_warn ("Could not create tracefs 'set_event_notrace_pid' entry\n" ); +trace_create_file ("set_event_notrace_pid" , +TRACE_MODE_WRITE , parent, tr, + &ftrace_set_event_notrace_pid_fops);/* ring buffer internal formats */ trace_create_file("header_page", TRACE_MODE_READ, d_events,
@@ -3790,17 +3784,14 @@ static __init int event_trace_init_fields(void)__init int event_trace_init (void ) {struct trace_array *tr; -struct dentry *entry;int ret; tr =top_trace_array ();if (!tr)return -ENODEV ; - entry =tracefs_create_file ("available_events" ,TRACE_MODE_READ , -NULL , tr, &ftrace_avail_fops); -if (!entry) -pr_warn ("Could not create tracefs 'available_events' entry\n" ); +trace_create_file ("available_events" ,TRACE_MODE_READ , +NULL , tr, &ftrace_avail_fops); ret =early_event_add_tracer (NULL , tr);if (ret)
kernel/trace/trace_kprobe.c +4 -11
@@ -1907,25 +1907,18 @@ core_initcall(init_kprobe_trace_early);static __init int init_kprobe_trace (void ) {int ret; -struct dentry *entry; ret =tracing_init_dentry ();if (ret)return 0 ; - entry =tracefs_create_file ("kprobe_events" ,TRACE_MODE_WRITE , -NULL ,NULL , &kprobe_events_ops); -/* Event list interface */ -if (!entry) -pr_warn ("Could not create tracefs 'kprobe_events' entry\n" ); +trace_create_file ("kprobe_events" ,TRACE_MODE_WRITE , +NULL ,NULL , &kprobe_events_ops);/* Profile interface */ - entry =tracefs_create_file ("kprobe_profile" ,TRACE_MODE_READ , -NULL ,NULL , &kprobe_profile_ops); - -if (!entry) -pr_warn ("Could not create tracefs 'kprobe_profile' entry\n" ); +trace_create_file ("kprobe_profile" ,TRACE_MODE_READ , +NULL ,NULL , &kprobe_profile_ops);setup_boot_kprobe_events ();
kernel/trace/trace_recursion_record.c +2 -5
@@ -224,12 +224,9 @@ static const struct file_operations recursed_functions_fops = {__init static int create_recursed_functions (void ) { -struct dentry *dentry; - dentry =trace_create_file ("recursed_functions" ,TRACE_MODE_WRITE , -NULL ,NULL , &recursed_functions_fops); -if (!dentry) -pr_warn ("WARNING: Failed to create recursed_functions\n" ); +trace_create_file ("recursed_functions" ,TRACE_MODE_WRITE , +NULL ,NULL , &recursed_functions_fops);return 0 ; }
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 4/23 ftrace: Deal with error return code of the ftrace_process_locs() function
In reply to Steven RostedtFrom: Yuntao Wang <[email protected]>
The ftrace_process_locs() function may return -ENOMEM error code, which should be handled by the callers.
--- kernel/trace/ftrace.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
kernel/trace/ftrace.c +13 -4
@@ -6864,11 +6864,16 @@ void ftrace_module_enable(struct module *mod)void ftrace_module_init (struct module *mod) { +int ret; +if (ftrace_disabled || !mod->num_ftrace_callsites)return ; -ftrace_process_locs (mod, mod->ftrace_callsites, - mod->ftrace_callsites + mod->num_ftrace_callsites); + ret =ftrace_process_locs (mod, mod->ftrace_callsites, + mod->ftrace_callsites + mod->num_ftrace_callsites); +if (ret) +pr_warn ("ftrace: failed to allocate entries for module '%s' functions\n" , + mod->name); } static void save_ftrace_mod_rec(struct ftrace_mod_map *mod_map,
@@ -7201,15 +7206,19 @@ void __init ftrace_init(void)pr_info ("ftrace: allocating %ld entries in %ld pages\n" , count, count /ENTRIES_PER_PAGE +1 ); - last_ftrace_enabled = ftrace_enabled =1 ; - ret =ftrace_process_locs (NULL , __start_mcount_loc, __stop_mcount_loc); +if (ret) { +pr_warn ("ftrace: failed to allocate entries for functions\n" ); + goto failed; + }pr_info ("ftrace: allocated %ld pages with %ld groups\n" , ftrace_number_of_pages, ftrace_number_of_groups); + last_ftrace_enabled = ftrace_enabled =1 ; +set_ftrace_early_filters ();return ;
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 5/23 tracing: Use strim() to remove whitespace instead of doing it manually
In reply to Steven RostedtFrom: Yuntao Wang <[email protected]>
The tracing_set_trace_write() function just removes the trailing whitespace from the user supplied tracer name, but the leading whitespace should also be removed.
In addition, if the user supplied tracer name contains only a few whitespace characters, the first one will not be removed using the current method, which results it a single whitespace character left in the buf.
To fix all of these issues, we use strim() to correctly remove both the leading and trailing whitespace.
--- kernel/trace/trace.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
kernel/trace/trace.c +3 -5
@@ -6461,7 +6461,7 @@ tracing_set_trace_write(struct file *filp, const char __user *ubuf, {struct trace_array *tr = filp->private_data;char buf[MAX_TRACER_SIZE +1 ]; -int i; +char *name;size_t ret;int err;
@@ -6475,11 +6475,9 @@ tracing_set_trace_write(struct file *filp, const char __user *ubuf, buf[cnt] =0 ; -/* strip ending whitespace. */ -for (i = cnt -1 ; i >0 &&isspace (buf[i]); i--) - buf[i] =0 ; + name =strim (buf); - err =tracing_set_tracer (tr, buf); + err =tracing_set_tracer (tr, name);if (err)return err;
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 6/23 tracing: Fix potential double free in create_var_ref()
In reply to Steven RostedtFrom: Keita Suzuki <[email protected]>
In create_var_ref(), init_var_ref() is called to initialize the fields of variable ref_field, which is allocated in the previous function call to create_hist_field(). Function init_var_ref() allocates the corresponding fields such as ref_field->system, but frees these fields when the function encounters an error. The caller later calls destroy_hist_field() to conduct error handling, which frees the fields and the variable itself. This results in double free of the fields which are already freed in the previous function.
Fix this by storing NULL to the corresponding fields when they are freed in init_var_ref().
--- kernel/trace/trace_events_hist.c | 3 +++ 1 file changed, 3 insertions(+)
kernel/trace/trace_events_hist.c +3 -0
@@ -2093,8 +2093,11 @@ static int init_var_ref(struct hist_field *ref_field,return err; free:kfree (ref_field->system); + ref_field->system =NULL ;kfree (ref_field->event_name); + ref_field->event_name =NULL ;kfree (ref_field->name); + ref_field->name =NULL ; goto out; }
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 7/23 tracing: Fix return value of trace_pid_write()
In reply to Steven RostedtFrom: Wonhyuk Yang <[email protected]>
Setting set_event_pid with trailing whitespace lead to endless write system calls like below.
$ strace echo "123 " > /sys/kernel/debug/tracing/set_event_pid
execve("/usr/bin/echo", ["echo", "123 "], ...) = 0
...
write(1, "123 \n", 5) = 4
write(1, "\n", 1) = 0
write(1, "\n", 1) = 0
write(1, "\n", 1) = 0
write(1, "\n", 1) = 0
write(1, "\n", 1) = 0
....This is because, the result of trace_get_user's are not returned when it read at least one pid. To fix it, update read variable even if parser->idx == 0.
The result of applied patch is below.
$ strace echo "123 " > /sys/kernel/debug/tracing/set_event_pid
execve("/usr/bin/echo", ["echo", "123 "], ...) = 0
...
write(1, "123 \n", 5) = 5
close(1) = 0--- kernel/trace/trace.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
kernel/trace/trace.c +4 -2
@@ -721,13 +721,16 @@ int trace_pid_write(struct trace_pid_list *filtered_pids, pos =0 ; ret =trace_get_user (&parser, ubuf, cnt, &pos); -if (ret <0 || !trace_parser_loaded (&parser)) +if (ret <0 )break ; read += ret; ubuf += ret; cnt -= ret; +if (!trace_parser_loaded (&parser)) +break ; + ret = -EINVAL ;if (kstrtoul (parser.buffer,0 , &val))break ;
@@ -753,7 +756,6 @@ int trace_pid_write(struct trace_pid_list *filtered_pids,if (!nr_pids) {/* Cleared the list of pids */ trace_pid_list_free (pid_list); - read = ret; pid_list =NULL ; }
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 9/23 tracing/timerlat: Notify IRQ new max latency only if stop tracing is set
In reply to Steven RostedtFrom: Daniel Bristot de Oliveira <[email protected]>
Currently, the notification of a new max latency is sent from timerlat's IRQ handler anytime a new max latency is found.
While this behavior is not wrong, the send IPI overhead itself will increase the thread latency and that is not the desired effect (tracing overhead).
Moreover, the thread will notify a new max latency again because the thread latency as it is always higher than the IRQ latency that woke it up.
The only case in which it is helpful to notify a new max latency from IRQ is when stop tracing (for the IRQ) is set, as in this case, the thread will not be dispatched.
Notify a new max latency from the IRQ handler only if stop tracing is set for the IRQ handler.
--- kernel/trace/trace_osnoise.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
kernel/trace/trace_osnoise.c +5 -4
@@ -1578,11 +1578,12 @@ static enum hrtimer_restart timerlat_irq(struct hrtimer *timer)trace_timerlat_sample (&s); -notify_new_max_latency (diff); - -if (osnoise_data.stop_tracing) -if (time_to_us (diff) >= osnoise_data.stop_tracing) +if (osnoise_data.stop_tracing) { +if (time_to_us (diff) >= osnoise_data.stop_tracing) {osnoise_stop_tracing (); +notify_new_max_latency (diff); + } + }wake_up_process (tlat->kthread);
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 10/23 tracing/timerlat: Print stacktrace in the IRQ handler if needed
In reply to Steven RostedtFrom: Daniel Bristot de Oliveira <[email protected]>
If print_stack and stop_tracing_us are set, and stop_tracing_us is hit with latency higher than or equal to print_stack, print the stack at the IRQ handler as it is useful to define the root cause for the IRQ latency.
--- Documentation/trace/timerlat-tracer.rst | 5 +++-- kernel/trace/trace_osnoise.c | 13 +++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-)
Documentation/trace/timerlat-tracer.rst +3 -2
@@ -74,8 +74,9 @@ directory. The timerlat configs are: - stop_tracing_total_us: stop the system tracing if a timer latency at the *thread* context is higher than the configured value happens. Writing 0 disables this option. - - print_stack: save the stack of the IRQ occurrence, and print - it after the *thread context* event". + - print_stack: save the stack of the IRQ occurrence. The stack is printed + after the *thread context* event, or at the IRQ handler if *stop_tracing_us* + is hit. timerlat and osnoise ----------------------------
kernel/trace/trace_osnoise.c +13 -0
@@ -1580,6 +1580,19 @@ static enum hrtimer_restart timerlat_irq(struct hrtimer *timer)if (osnoise_data.stop_tracing) {if (time_to_us (diff) >= osnoise_data.stop_tracing) { + +/* +* At this point, if stop_tracing is set and <= print_stack, +* print_stack is set and would be printed in the thread handler. +* +* Thus, print the stack trace as it is helpful to define the +* root cause of an IRQ latency. +*/ +if (osnoise_data.stop_tracing <= osnoise_data.print_stack) { +timerlat_save_stack (0 ); +timerlat_dump_stack (time_to_us (diff)); + } +osnoise_stop_tracing ();notify_new_max_latency (diff); }
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 11/23 tracing/timerlat: Do not wakeup the thread if the trace stops at the IRQ
In reply to Steven RostedtFrom: Daniel Bristot de Oliveira <[email protected]>
There is no need to wakeup the timerlat/ thread if stop tracing is hit at the timerlat's IRQ handler.
Return before waking up timerlat's thread.
--- kernel/trace/trace_osnoise.c | 2 ++ 1 file changed, 2 insertions(+)
kernel/trace/trace_osnoise.c +2 -0
@@ -1595,6 +1595,8 @@ static enum hrtimer_restart timerlat_irq(struct hrtimer *timer)osnoise_stop_tracing ();notify_new_max_latency (diff); + +return HRTIMER_NORESTART ; } }
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 12/23 tracing: Change "char *" string form to "char []"
In reply to Steven RostedtFrom: liqiong <[email protected]>
The "char []" string form declares a single variable. It is better than "char *" which creates two variables in the final assembly.
--- kernel/trace/trace.c | 2 +- kernel/trace/trace_events_hist.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
kernel/trace/trace.c +1 -1
@@ -4252,7 +4252,7 @@ static void print_func_help_header_irq(struct array_buffer *buf, struct seq_file unsigned int flags) {bool tgid = flags &TRACE_ITER_RECORD_TGID ; -const char *space =" " ; +static const char space[] =" " ;int prec = tgid ?12 :2 ;print_event_info (buf, m);
kernel/trace/trace_events_hist.c +1 -1
@@ -4165,7 +4165,7 @@ static int create_val_field(struct hist_trigger_data *hist_data,return __create_val_field (hist_data, val_idx, file,NULL , field_str,0 ); } -static const char *no_comm ="(no comm)" ; +static const char no_comm[] ="(no comm)" ; static u64 hist_field_execname(struct hist_field *hist_field, struct tracing_map_elt *elt,
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 13/23 tracing: Cleanup code by removing init "char *name"
In reply to Steven RostedtFrom: liqiong <[email protected]>
The pointer is assigned to "type->name" anyway. no need to initialize with "preemption".
--- kernel/trace/trace.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
kernel/trace/trace.c +1 -3
@@ -4276,9 +4276,7 @@ print_trace_header(struct seq_file *m, struct trace_iterator *iter)struct tracer *type = iter->trace;unsigned long entries;unsigned long total; -const char *name ="preemption" ; - - name = type->name; +const char *name = type->name;get_total_entries (buf, &total, &entries);
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 14/23 ftrace: Remove return value of ftrace_arch_modify_*()
In reply to Steven RostedtFrom: Li kunyu <[email protected]>
All instances of the function ftrace_arch_modify_prepare() and
ftrace_arch_modify_post_process() return zero. There's no point in checking their return value. Just have them be void functions.
--- arch/arm/kernel/ftrace.c | 6 ++---- arch/riscv/kernel/ftrace.c | 6 ++---- arch/s390/kernel/ftrace.c | 3 +-- arch/x86/kernel/ftrace.c | 6 ++---- include/linux/ftrace.h | 4 ++-- kernel/trace/ftrace.c | 16 ++++------------ 6 files changed, 13 insertions(+), 28 deletions(-)
arch/arm/kernel/ftrace.c +2 -4
@@ -79,16 +79,14 @@ static unsigned long __ref adjust_address(struct dyn_ftrace *rec,return (unsigned long )&ftrace_regs_caller_from_init; } -int ftrace_arch_code_modify_prepare (void ) +void ftrace_arch_code_modify_prepare (void ) { -return 0 ; } -int ftrace_arch_code_modify_post_process (void ) +void ftrace_arch_code_modify_post_process (void ) {/* Make sure any TLB misses during machine stop are cleared. */ flush_tlb_all (); -return 0 ; } static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr,
arch/riscv/kernel/ftrace.c +2 -4
@@ -12,16 +12,14 @@#include <asm/patch.h> #ifdef CONFIG_DYNAMIC_FTRACE -int ftrace_arch_code_modify_prepare(void) __acquires(&text_mutex) +void ftrace_arch_code_modify_prepare(void) __acquires(&text_mutex) {mutex_lock (&text_mutex); -return 0 ; } -int ftrace_arch_code_modify_post_process(void) __releases(&text_mutex) +void ftrace_arch_code_modify_post_process(void) __releases(&text_mutex) {mutex_unlock (&text_mutex); -return 0 ; } static int ftrace_check_current_call(unsigned long hook_pos,
arch/s390/kernel/ftrace.c +1 -2
@@ -225,14 +225,13 @@ void arch_ftrace_update_code(int command)ftrace_modify_all_code (command); } -int ftrace_arch_code_modify_post_process (void ) +void ftrace_arch_code_modify_post_process (void ) {/* * Flush any pre-fetched instructions on all * CPUs to make the new code visible. */ text_poke_sync_lock (); -return 0 ; }#ifdef CONFIG_MODULES
arch/x86/kernel/ftrace.c +2 -4
@@ -37,7 +37,7 @@static int ftrace_poke_late =0 ; -int ftrace_arch_code_modify_prepare (void ) +void ftrace_arch_code_modify_prepare (void ) __acquires(&text_mutex) {/*
@@ -47,10 +47,9 @@ int ftrace_arch_code_modify_prepare(void)*/ mutex_lock (&text_mutex); ftrace_poke_late =1 ; -return 0 ; } -int ftrace_arch_code_modify_post_process (void ) +void ftrace_arch_code_modify_post_process (void ) __releases(&text_mutex) {/*
@@ -61,7 +60,6 @@ int ftrace_arch_code_modify_post_process(void)text_poke_finish (); ftrace_poke_late =0 ;mutex_unlock (&text_mutex); -return 0 ; }static const char *ftrace_nop_replace (void )
include/linux/ftrace.h +2 -2
@@ -449,8 +449,8 @@ static inline void stack_tracer_enable(void) { }#ifdef CONFIG_DYNAMIC_FTRACE -int ftrace_arch_code_modify_prepare (void ); -int ftrace_arch_code_modify_post_process (void ); +void ftrace_arch_code_modify_prepare (void ); +void ftrace_arch_code_modify_post_process (void );enum ftrace_bug_type { FTRACE_BUG_UNKNOWN,
kernel/trace/ftrace.c +4 -12
@@ -2704,18 +2704,16 @@ ftrace_nop_initialize(struct module *mod, struct dyn_ftrace *rec)* archs can override this function if they must do something * before the modifying code is performed. */ -int __weak ftrace_arch_code_modify_prepare (void ) +void __weak ftrace_arch_code_modify_prepare (void ) { -return 0 ; }/* * archs can override this function if they must do something * after the modifying code is performed. */ -int __weak ftrace_arch_code_modify_post_process (void ) +void __weak ftrace_arch_code_modify_post_process (void ) { -return 0 ; }void ftrace_modify_all_code (int command)
@@ -2801,12 +2799,7 @@ void __weak arch_ftrace_update_code(int command)static void ftrace_run_update_code (int command) { -int ret; - - ret =ftrace_arch_code_modify_prepare (); -FTRACE_WARN_ON (ret); -if (ret) -return ; +ftrace_arch_code_modify_prepare ();/* * By default we use stop_machine() to modify the code.
@@ -2816,8 +2809,7 @@ static void ftrace_run_update_code(int command)*/ arch_ftrace_update_code (command); - ret =ftrace_arch_code_modify_post_process (); -FTRACE_WARN_ON (ret); +ftrace_arch_code_modify_post_process (); } static void ftrace_run_modify_code(struct ftrace_ops *ops, int command,
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 15/23 ftrace: Fix typo in comment
In reply to Steven RostedtFrom: Julia Lawall <[email protected]>
Spelling mistake (triple letters) in comment.
Detected with the help of Coccinelle.
--- kernel/trace/ftrace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/ftrace.c +1 -1
@@ -119,7 +119,7 @@ struct ftrace_ops __rcu *ftrace_ops_list __read_mostly = &ftrace_list_end;ftrace_func_t ftrace_trace_function__read_mostly = ftrace_stub;struct ftrace_ops global_ops; -/* Defined by vmlinux.lds.h see the commment above arch_ftrace_ops_list_func for details */ +/* Defined by vmlinux.lds.h see the comment above arch_ftrace_ops_list_func for details */ void ftrace_ops_list_func (unsigned long ip,unsigned long parent_ip,struct ftrace_ops *op,struct ftrace_regs *fregs);
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 16/23 tracing: Initialize integer variable to prevent garbage return value
In reply to Steven RostedtFrom: Gautam Menghani <[email protected]>
Initialize the integer variable to 0 to fix the clang scan warning:
Undefined or garbage value returned to caller [core.uninitialized.UndefReturn]
return ret;
--- kernel/trace/trace_boot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/trace_boot.c +1 -1
@@ -300,7 +300,7 @@ trace_boot_hist_add_handlers(struct xbc_node *hnode, char **bufp, {struct xbc_node *node;const char *p, *handler; -int ret; +int ret =0 ; handler =xbc_node_get_data (hnode);
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 17/23 tracing: Disable kcov on trace_preemptirq.c
In reply to Steven RostedtFrom: Congyu Liu <[email protected]>
Functions in trace_preemptirq.c could be invoked from early interrupt code that bypasses kcov trace function's in_task() check. Disable kcov on this file to reduce random code coverage.
--- kernel/trace/Makefile | 4 ++++ 1 file changed, 4 insertions(+)
kernel/trace/Makefile +4 -0
@@ -31,6 +31,10 @@ ifdef CONFIG_GCOV_PROFILE_FTRACE GCOV_PROFILE := y endif +# Functions in this file could be invoked from early interrupt +# code and produce random code coverage. +KCOV_INSTRUMENT_trace_preemptirq.o := n + CFLAGS_bpf_trace.o := -I$(src) CFLAGS_trace_benchmark.o := -I$(src)
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 18/23 tracing: Fix comments of create_filter()
In reply to Steven RostedtFrom: sunliming <[email protected]>
The name in comments of parameter "filter_string" in function create_filter is annotated as "filter_str", just fix it.
--- kernel/trace/trace_events_filter.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/trace_events_filter.c +1 -1
@@ -1816,7 +1816,7 @@ static void create_filter_finish(struct filter_parse_error *pe)* create_filter - create a filter for a trace_event_call * @tr: the trace array associated with these events * @call: trace_event_call to create a filter for -* @filter_str: filter string +* @filter_string: filter string * @set_str: remember @filter_str and enable detailed error in filter * @filterp: out param for created filter (always updated on return) * Must be a pointer that references a NULL pointer.
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 19/23 ftrace: Clean up hash direct_functions on register failures
In reply to Steven RostedtFrom: Song Liu <[email protected]>
We see the following GPF when register_ftrace_direct fails:
[ ] general protection fault, probably for non-canonical address \
0x200000000000010: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC PTI
[...]
[ ] RIP: 0010:ftrace_find_rec_direct+0x53/0x70
[ ] Code: 48 c1 e0 03 48 03 42 08 48 8b 10 31 c0 48 85 d2 74 [...] [ ] RSP: 0018:ffffc9000138bc10 EFLAGS: 00010206
[ ] RAX: 0000000000000000 RBX: ffffffff813e0df0 RCX: 000000000000003b [ ] RDX: 0200000000000000 RSI: 000000000000000c RDI: ffffffff813e0df0 [ ] RBP: ffffffffa00a3000 R08: ffffffff81180ce0 R09: 0000000000000001 [ ] R10: ffffc9000138bc18 R11: 0000000000000001 R12: ffffffff813e0df0 [ ] R13: ffffffff813e0df0 R14: ffff888171b56400 R15: 0000000000000000 [ ] FS: 00007fa9420c7780(0000) GS:ffff888ff6a00000(0000) knlGS:000000000 [ ] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ ] CR2: 000000000770d000 CR3: 0000000107d50003 CR4: 0000000000370ee0 [ ] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ ] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ ] Call Trace:
[ ] <TASK> [ ] register_ftrace_direct+0x54/0x290 [ ] ? render_sigset_t+0xa0/0xa0 [ ] bpf_trampoline_update+0x3f5/0x4a0 [ ] ? 0xffffffffa00a3000 [ ] bpf_trampoline_link_prog+0xa9/0x140 [ ] bpf_tracing_prog_attach+0x1dc/0x450 [ ] bpf_raw_tracepoint_open+0x9a/0x1e0 [ ] ? find_held_lock+0x2d/0x90 [ ] ? lock_release+0x150/0x430 [ ] __sys_bpf+0xbd6/0x2700 [ ] ? lock_is_held_type+0xd8/0x130 [ ] __x64_sys_bpf+0x1c/0x20 [ ] do_syscall_64+0x3a/0x80 [ ] entry_SYSCALL_64_after_hwframe+0x44/0xae
[ ] RIP: 0033:0x7fa9421defa9
[ ] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 9 f8 [...] [ ] RSP: 002b:00007ffed743bd78 EFLAGS: 00000246 ORIG_RAX: 0000000000000141 [ ] RAX: ffffffffffffffda RBX: 00000000069d2480 RCX: 00007fa9421defa9 [ ] RDX: 0000000000000078 RSI: 00007ffed743bd80 RDI: 0000000000000011 [ ] RBP: 00007ffed743be00 R08: 0000000000bb7270 R09: 0000000000000000 [ ] R10: 00000000069da210 R11: 0000000000000246 R12: 0000000000000001 [ ] R13: 00007ffed743c4b0 R14: 00000000069d2480 R15: 0000000000000001 [ ] </TASK>
[ ] Modules linked in: klp_vm(OK)
[ ] ---[ end trace 0000000000000000 ]---
One way to trigger this is:
1. load a livepatch that patches kernel function xxx;
2. run bpftrace -e 'kfunc:xxx {}', this will fail (expected for now);
3. repeat #2 => gpf.
This is because the entry is added to direct_functions, but not removed. Fix this by remove the entry from direct_functions when register_ftrace_direct fails.
Also remove the last trailing space from ftrace.c, so we don't have to worry about it anymore.
--- kernel/trace/ftrace.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
kernel/trace/ftrace.c +2 -3
@@ -4454,7 +4454,7 @@ int ftrace_func_mapper_add_ip(struct ftrace_func_mapper *mapper,* @ip: The instruction pointer address to remove the data from * * Returns the data if it is found, otherwise NULL. -* Note, if the data pointer is used as the data itself, (see +* Note, if the data pointer is used as the data itself, (see * ftrace_func_mapper_find_ip(), then the return value may be meaningless, * if the data pointer was set to zero. */
@@ -5188,8 +5188,6 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr) goto out_unlock; ret =ftrace_set_filter_ip (&direct_ops, ip,0 ,0 ); -if (ret) -remove_hash_entry (direct_functions, entry);if (!ret && !(direct_ops.flags &FTRACE_OPS_FL_ENABLED )) { ret =register_ftrace_function (&direct_ops);
@@ -5198,6 +5196,7 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr) }if (ret) { +remove_hash_entry (direct_functions, entry);kfree (entry);if (!direct->count) {list_del_rcu (&direct->next);
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 20/23 x86,tracing: Remove unused headers
In reply to Steven RostedtFrom: sunliming <[email protected]>
Commit 4b9a8dca0e58 ("x86/idt: Remove the tracing IDT completely") removed the tracing IDT from the file arch/x86/kernel/tracepoint.c, but left the related headers unused, remove it.
--- arch/x86/kernel/tracepoint.c | 3 --- 1 file changed, 3 deletions(-)
arch/x86/kernel/tracepoint.c +0 -3
@@ -8,10 +8,7 @@#include <linux/jump_label.h> #include <linux/atomic.h> -#include <asm/hw_irq.h> -#include <asm/desc.h> #include <asm/trace/exceptions.h> -#include <asm/trace/irq_vectors.h> DEFINE_STATIC_KEY_FALSE (trace_pagefault_key);
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 21/23 x86/traceponit: Fix comment about irq vector tracepoints
In reply to Steven RostedtFrom: sunliming <[email protected]>
Commit:
4b9a8dca0e58 ("x86/idt: Remove the tracing IDT completely")
removed the 'tracing IDT' from arch/x86/kernel/tracepoint.c, but left related comment. So that the comment become anachronistic. Just remove the comment.
--- arch/x86/kernel/tracepoint.c | 3 --- 1 file changed, 3 deletions(-)
arch/x86/kernel/tracepoint.c +0 -3
@@ -1,9 +1,6 @@// SPDX-License-Identifier: GPL-2.0 /* -* Code for supporting irq vector tracepoints. -* * Copyright (C) 2013 Seiji Aguchi <[email protected]> -* */ #include <linux/jump_label.h> #include <linux/atomic.h>
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 22/23 tracing: Fix comments for event_trigger_separate_filter()
In reply to Steven RostedtFrom: sunliming <[email protected]>
The parameter name in comments of event_trigger_separate_filter() is inconsistent with actual parameter name, fix it.
--- kernel/trace/trace_events_trigger.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
kernel/trace/trace_events_trigger.c +4 -4
@@ -738,15 +738,15 @@ bool event_trigger_empty_param(const char *param)/** * event_trigger_separate_filter - separate an event trigger from a filter -* @param: The param string containing trigger and possibly filter -* @trigger: outparam, will be filled with a pointer to the trigger +* @param_and_filter: String containing trigger and possibly filter +* @param: outparam, will be filled with a pointer to the trigger * @filter: outparam, will be filled with a pointer to the filter * @param_required: Specifies whether or not the param string is required * * Given a param string of the form '[trigger] [if filter]', this * function separates the filter from the trigger and returns the -* trigger in *trigger and the filter in *filter. Either the *trigger -* or the *filter may be set to NULL by this function - if not set to +* trigger in @param and the filter in @filter. Either the @param +* or the @filter may be set to NULL by this function - if not set to * NULL, they will contain strings corresponding to the trigger and * filter. *
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 23/23 ftrace: Add FTRACE_MCOUNT_MAX_OFFSET to avoid adding weak function
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
If an unused weak function was traced, it's call to fentry will still exist, which gets added into the __mcount_loc table. Ftrace will use kallsyms to retrieve the name for each location in __mcount_loc to display it in the available_filter_functions and used to enable functions via the name matching in set_ftrace_filter/notrace. Enabling these functions do nothing but enable an unused call to ftrace_caller. If a traced weak function is overridden, the symbol of the function would be used for it, which will either created duplicate names, or if the previous function was not traced, it would be incorrectly be listed in available_filter_functions as a function that can be traced.
This became an issue with BPF[1] as there are tooling that enables the direct callers via ftrace but then checks to see if the functions were actually enabled. The case of one function that was marked notrace, but was followed by an unused weak function that was traced. The unused function's call to fentry was added to the __mcount_loc section, and kallsyms retrieved the untraced function's symbol as the weak function was overridden. Since the untraced function would not get traced, the BPF check would detect this and fail.
The real fix would be to fix kallsyms to not show addresses of weak functions as the function before it. But that would require adding code in the build to add function size to kallsyms so that it can know when the function ends instead of just using the start of the next known symbol.
In the mean time, this is a work around. Add a FTRACE_MCOUNT_MAX_OFFSET macro that if defined, ftrace will ignore any function that has its call to fentry/mcount that has an offset from the symbol that is greater than FTRACE_MCOUNT_MAX_OFFSET.
If CONFIG_HAVE_FENTRY is defined for x86, define FTRACE_MCOUNT_MAX_OFFSET to zero (unless IBT is enabled), which will have ftrace ignore all locations that are not at the start of the function (or one after the ENDBR instruction).
A worker thread is added at boot up to scan all the ftrace record entries, and will mark any that fail the FTRACE_MCOUNT_MAX_OFFSET test as disabled. They will still appear in the available_filter_functions file as:
__ftrace_invalid_address___<invalid-offset>
(showing the offset that caused it to be invalid).
This is required for tools that use libtracefs (like trace-cmd does) that scan the available_filter_functions and enable set_ftrace_filter and set_ftrace_notrace using indexes of the function listed in the file (this is a speedup, as enabling thousands of files via names is an O(n^2) operation and can take minutes to complete, where the indexing takes less than a second).
The invalid functions cannot be removed from available_filter_functions as the names there correspond to the ftrace records in the array that manages them (and the indexing depends on this).
[1] https://lore.kernel.org/all/[email protected]/
--- arch/x86/include/asm/ftrace.h | 7 ++ kernel/trace/ftrace.c | 137 +++++++++++++++++++++++++++++++++- 2 files changed, 142 insertions(+), 2 deletions(-)
arch/x86/include/asm/ftrace.h +7 -0
@@ -9,6 +9,13 @@# define MCOUNT_ADDR ((unsigned long)(__fentry__))#define MCOUNT_INSN_SIZE 5/* sizeof mcount call */ +/* Ignore unused weak functions which will have non zero offsets */ +#ifdef CONFIG_HAVE_FENTRY +# include <asm/ibt.h> +/* Add offset for endbr64 if IBT enabled */ +# define FTRACE_MCOUNT_MAX_OFFSET ENDBR_INSN_SIZE +#endif +#ifdef CONFIG_DYNAMIC_FTRACE #define ARCH_SUPPORTS_FTRACE_OPS 1#endif
kernel/trace/ftrace.c +135 -2
173 lines not on this page · raw message
Signature · 1 line
2.35.1
Steven Rostedt
maintainerQuoted 11 lines · On Fri, 27 May 2022 22:50:28 -0400 Steven Rostedt <[email protected]> wrote:
Actually, a lot of these changes were in my queue, I just haven't tested them. But they are mostly fixes and clean ups. No real functional features.
My tests on these have not completely finished, so I may have to rebase. But since this is half way into the merge window, I want these in linux-next for a little bit before sending my pull request.
Steven Rostedt (Google) (2):
ftrace: AddFTRACE_MCOUNT_MAX_OFFSETto avoid adding weak function
And my test just failed on this one :-p (Something stupid and minor)
I guess I'll send out a v6 and try again.
-- Steve