Re: [PATCH 3/3] trace: print address if symbol not found
Hi Tobin,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.15-rc4 next-20171219]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Tobin-C-Harding/kallsyms-don-t-leak-address/20171220-062707
config: i386-randconfig-s1-201751 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
kernel/trace/trace_events_hist.c: In function
'hist_trigger_stacktrace_print':
>> kernel/trace/trace_events_hist.c:985:3: error: implicit declaration of
>> function 'trace_sprint_symbol_addr' [-Werror=implicit-function-declaration]
trace_sprint_symbol_addr(str, stacktrace_entries[i]);
^~~~
cc1: some warnings being treated as errors
vim +/trace_sprint_symbol_addr +985 kernel/trace/trace_events_hist.c
971
972 static void hist_trigger_stacktrace_print(struct seq_file *m,
973unsigned long
*stacktrace_entries,
974unsigned int max_entries)
975 {
976 char str[KSYM_SYMBOL_LEN];
977 unsigned int spaces = 8;
978 unsigned int i;
979
980 for (i = 0; i < max_entries; i++) {
981 if (stacktrace_entries[i] == ULONG_MAX)
982 return;
983
984 seq_printf(m, "%*c", 1 + spaces, ' ');
> 985 trace_sprint_symbol_addr(str, stacktrace_entries[i]);
986 seq_printf(m, "%s\n", str);
987 }
988 }
989
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH 3/3] trace: print address if symbol not found
Hi Tobin,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.15-rc4 next-20171219]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Tobin-C-Harding/kallsyms-don-t-leak-address/20171220-062707
config: i386-randconfig-x016-201751 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
kernel/trace/trace_events_hist.c: In function
'hist_trigger_stacktrace_print':
>> kernel/trace/trace_events_hist.c:985:3: error: implicit declaration of
>> function 'trace_sprint_symbol_addr'; did you mean 'trace_sprint_symbol'?
>> [-Werror=implicit-function-declaration]
trace_sprint_symbol_addr(str, stacktrace_entries[i]);
^~~~
trace_sprint_symbol
cc1: some warnings being treated as errors
vim +985 kernel/trace/trace_events_hist.c
971
972 static void hist_trigger_stacktrace_print(struct seq_file *m,
973unsigned long
*stacktrace_entries,
974unsigned int max_entries)
975 {
976 char str[KSYM_SYMBOL_LEN];
977 unsigned int spaces = 8;
978 unsigned int i;
979
980 for (i = 0; i < max_entries; i++) {
981 if (stacktrace_entries[i] == ULONG_MAX)
982 return;
983
984 seq_printf(m, "%*c", 1 + spaces, ' ');
> 985 trace_sprint_symbol_addr(str, stacktrace_entries[i]);
986 seq_printf(m, "%s\n", str);
987 }
988 }
989
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH 3/3] trace: print address if symbol not found
On Mon, Dec 18, 2017 at 10:37:38PM -0500, Steven Rostedt wrote: > On Tue, 19 Dec 2017 14:00:11 +1100 > "Tobin C. Harding" wrote: > > > I ran through these as outlined here for the new version (v4). This hits > > the modified code but doesn't test symbol look up failure. > > stacktrace shouldn't post non kernel values, unless there's a frame > pointer that isn't handled by kallsyms. > > As for the other two, we could probably force a failure, like: > > # echo 'hist:keys=hrtimer.sym' > \ > events/timer/hrtimer_start/trigger > # cat events/timer/hrtimer_start/hist > > And then just add sym-offset too. > > > I also configured kernel with 'Perform a startup test on ftrace' for > > good luck. > > > > Are you happy with this level of testing? > > Can you try the above. Did both and in both cases we get the addresses as hoped :) thanks, Tobin.
Re: [PATCH 3/3] trace: print address if symbol not found
On Tue, 19 Dec 2017 14:00:11 +1100 "Tobin C. Harding" wrote: > I ran through these as outlined here for the new version (v4). This hits > the modified code but doesn't test symbol look up failure. stacktrace shouldn't post non kernel values, unless there's a frame pointer that isn't handled by kallsyms. As for the other two, we could probably force a failure, like: # echo 'hist:keys=hrtimer.sym' > \ events/timer/hrtimer_start/trigger # cat events/timer/hrtimer_start/hist And then just add sym-offset too. > > I also configured kernel with 'Perform a startup test on ftrace' for > good luck. > > Are you happy with this level of testing? Can you try the above. -- Steve
Re: [PATCH 3/3] trace: print address if symbol not found
On Tue, Dec 19, 2017 at 02:00:11PM +1100, Tobin C. Harding wrote: > On Mon, Dec 18, 2017 at 06:51:43PM -0500, Steven Rostedt wrote: > > On Tue, 19 Dec 2017 08:16:14 +1100 > > "Tobin C. Harding" wrote: > > > > > > > #endif /* _LINUX_KERNEL_TRACE_H */ > > > > > diff --git a/kernel/trace/trace_events_hist.c > > > > > b/kernel/trace/trace_events_hist.c > > > > > index 1e1558c99d56..3e28522a76f4 100644 > > > > > --- a/kernel/trace/trace_events_hist.c > > > > > +++ b/kernel/trace/trace_events_hist.c > > > > > @@ -982,7 +982,7 @@ static void hist_trigger_stacktrace_print(struct > > > > > seq_file *m, > > > > > return; > > > > > > > > > > seq_printf(m, "%*c", 1 + spaces, ' '); > > > > > - sprint_symbol(str, stacktrace_entries[i]); > > > > > + trace_sprint_symbol_addr(str, stacktrace_entries[i]); > > > > > > > > > > > > If you have the time to give me some brief pointers on how I should go > > > about testing this I'd love to test it before the next version. I know > > > very little about ftrace. > > > > For hitting the histogram stacktrace trigger (this code path), make > > sure you have CONFIG_HIST_TRIGGERS enabled. And then do: > > > > # cd /sys/kernel/debug/tracing > > # echo 'hist:keys=common_pid.execname,stacktrace:vals=prev_state' > \ > > events/sched/sched_switch/trigger > > # cat events/sched/sched_switch/hist > > > > For the "sym" part, you can do (from the same directory): > > > > # echo 'hist:keys=call_site.sym:vals=bytes_req' > \ > > events/kmem/kmalloc/trigger > > # cat events/kmem/kmalloc/hist > > > > > > And for sym-offset: > > > > # echo 'hist:keys=call_site.sym-offset:vals=bytes_req' > \ > > events/kmem/kmalloc/trigger > > # cat events/kmem/kmalloc/hist > > I ran through these as outlined here for the new version (v4). This hits Should have been: v2 thanks, Tobin.
Re: [PATCH 3/3] trace: print address if symbol not found
On Mon, Dec 18, 2017 at 06:51:43PM -0500, Steven Rostedt wrote: > On Tue, 19 Dec 2017 08:16:14 +1100 > "Tobin C. Harding" wrote: > > > > > #endif /* _LINUX_KERNEL_TRACE_H */ > > > > diff --git a/kernel/trace/trace_events_hist.c > > > > b/kernel/trace/trace_events_hist.c > > > > index 1e1558c99d56..3e28522a76f4 100644 > > > > --- a/kernel/trace/trace_events_hist.c > > > > +++ b/kernel/trace/trace_events_hist.c > > > > @@ -982,7 +982,7 @@ static void hist_trigger_stacktrace_print(struct > > > > seq_file *m, > > > > return; > > > > > > > > seq_printf(m, "%*c", 1 + spaces, ' '); > > > > - sprint_symbol(str, stacktrace_entries[i]); > > > > + trace_sprint_symbol_addr(str, stacktrace_entries[i]); > > > > > > > > If you have the time to give me some brief pointers on how I should go > > about testing this I'd love to test it before the next version. I know > > very little about ftrace. > > For hitting the histogram stacktrace trigger (this code path), make > sure you have CONFIG_HIST_TRIGGERS enabled. And then do: > > # cd /sys/kernel/debug/tracing > # echo 'hist:keys=common_pid.execname,stacktrace:vals=prev_state' > \ > events/sched/sched_switch/trigger > # cat events/sched/sched_switch/hist > > For the "sym" part, you can do (from the same directory): > > # echo 'hist:keys=call_site.sym:vals=bytes_req' > \ > events/kmem/kmalloc/trigger > # cat events/kmem/kmalloc/hist > > > And for sym-offset: > > # echo 'hist:keys=call_site.sym-offset:vals=bytes_req' > \ > events/kmem/kmalloc/trigger > # cat events/kmem/kmalloc/hist I ran through these as outlined here for the new version (v4). This hits the modified code but doesn't test symbol look up failure. I also configured kernel with 'Perform a startup test on ftrace' for good luck. Are you happy with this level of testing? thanks, Tobin.
Re: [PATCH 3/3] trace: print address if symbol not found
On Mon, Dec 18, 2017 at 06:51:43PM -0500, Steven Rostedt wrote: > On Tue, 19 Dec 2017 08:16:14 +1100 > "Tobin C. Harding" wrote: > > > > > #endif /* _LINUX_KERNEL_TRACE_H */ > > > > diff --git a/kernel/trace/trace_events_hist.c > > > > b/kernel/trace/trace_events_hist.c > > > > index 1e1558c99d56..3e28522a76f4 100644 > > > > --- a/kernel/trace/trace_events_hist.c > > > > +++ b/kernel/trace/trace_events_hist.c > > > > @@ -982,7 +982,7 @@ static void hist_trigger_stacktrace_print(struct > > > > seq_file *m, > > > > return; > > > > > > > > seq_printf(m, "%*c", 1 + spaces, ' '); > > > > - sprint_symbol(str, stacktrace_entries[i]); > > > > + trace_sprint_symbol_addr(str, stacktrace_entries[i]); > > > > > > > > If you have the time to give me some brief pointers on how I should go > > about testing this I'd love to test it before the next version. I know > > very little about ftrace. > > For hitting the histogram stacktrace trigger (this code path), make > sure you have CONFIG_HIST_TRIGGERS enabled. And then do: > > # cd /sys/kernel/debug/tracing > # echo 'hist:keys=common_pid.execname,stacktrace:vals=prev_state' > \ > events/sched/sched_switch/trigger > # cat events/sched/sched_switch/hist > > For the "sym" part, you can do (from the same directory): > > # echo 'hist:keys=call_site.sym:vals=bytes_req' > \ > events/kmem/kmalloc/trigger > # cat events/kmem/kmalloc/hist > > > And for sym-offset: > > # echo 'hist:keys=call_site.sym-offset:vals=bytes_req' > \ > events/kmem/kmalloc/trigger > # cat events/kmem/kmalloc/hist > > -- Steve Thanks, you're the man
Re: [PATCH 3/3] trace: print address if symbol not found
On Tue, 19 Dec 2017 08:16:14 +1100 "Tobin C. Harding" wrote: > > > #endif /* _LINUX_KERNEL_TRACE_H */ > > > diff --git a/kernel/trace/trace_events_hist.c > > > b/kernel/trace/trace_events_hist.c > > > index 1e1558c99d56..3e28522a76f4 100644 > > > --- a/kernel/trace/trace_events_hist.c > > > +++ b/kernel/trace/trace_events_hist.c > > > @@ -982,7 +982,7 @@ static void hist_trigger_stacktrace_print(struct > > > seq_file *m, > > > return; > > > > > > seq_printf(m, "%*c", 1 + spaces, ' '); > > > - sprint_symbol(str, stacktrace_entries[i]); > > > + trace_sprint_symbol_addr(str, stacktrace_entries[i]); > > > > If you have the time to give me some brief pointers on how I should go > about testing this I'd love to test it before the next version. I know > very little about ftrace. For hitting the histogram stacktrace trigger (this code path), make sure you have CONFIG_HIST_TRIGGERS enabled. And then do: # cd /sys/kernel/debug/tracing # echo 'hist:keys=common_pid.execname,stacktrace:vals=prev_state' > \ events/sched/sched_switch/trigger # cat events/sched/sched_switch/hist For the "sym" part, you can do (from the same directory): # echo 'hist:keys=call_site.sym:vals=bytes_req' > \ events/kmem/kmalloc/trigger # cat events/kmem/kmalloc/hist And for sym-offset: # echo 'hist:keys=call_site.sym-offset:vals=bytes_req' > \ events/kmem/kmalloc/trigger # cat events/kmem/kmalloc/hist -- Steve
Re: [PATCH 3/3] trace: print address if symbol not found
On Mon, Dec 18, 2017 at 11:49:47AM -0500, Steven Rostedt wrote:
> On Mon, 18 Dec 2017 10:53:32 +1100
> "Tobin C. Harding" wrote:
>
> > Fixes behaviour modified by: commit bd6b239cdbb2 ("kallsyms: don't leak
> > address when symbol not found")
> >
> > Previous patch changed behaviour of kallsyms function sprint_symbol() to
> > return an error code instead of printing the address if a symbol was not
> > found. Ftrace relies on the original behaviour. We should not break
> > tracing when applying the previous patch. We can maintain the original
> > behaviour by checking the return code on calls to sprint_symbol() and
> > friends.
> >
> > Check return code and print actual address on error (i.e symbol not
> > found).
> >
> > Signed-off-by: Tobin C. Harding
> > ---
> > kernel/trace/trace.h | 24
> > kernel/trace/trace_events_hist.c | 6 +++---
> > 2 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index 2a6d0325a761..881b1a577d75 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -1814,4 +1814,28 @@ static inline void trace_event_eval_update(struct
> > trace_eval_map **map, int len)
> >
> > extern struct trace_iterator *tracepoint_print_iter;
> >
> > +static inline int
> > +trace_sprint_symbol(char *buffer, unsigned long address)
> > +{
> > + int ret;
> > +
> > + ret = sprint_symbol(buffer, address);
> > + if (ret == -1)
> > + ret = sprintf(buffer, "0x%lx", address);
> > +
> > + return ret;
> > +}
> > +
> > +static inline int
> > +trace_sprint_symbol_no_offset(char *buffer, unsigned long address)
> > +{
> > + int ret;
> > +
> > + ret = sprint_symbol_no_offset(buffer, address);
> > + if (ret == -1)
> > + ret = sprintf(buffer, "0x%lx", address);
> > +
> > + return ret;
> > +}
> > +
> > #endif /* _LINUX_KERNEL_TRACE_H */
> > diff --git a/kernel/trace/trace_events_hist.c
> > b/kernel/trace/trace_events_hist.c
> > index 1e1558c99d56..3e28522a76f4 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -982,7 +982,7 @@ static void hist_trigger_stacktrace_print(struct
> > seq_file *m,
> > return;
> >
> > seq_printf(m, "%*c", 1 + spaces, ' ');
> > - sprint_symbol(str, stacktrace_entries[i]);
> > + trace_sprint_symbol_addr(str, stacktrace_entries[i]);
>
> Hmm, where is trace_sprint_symbol_addr() defined?
>
> -- Steve
Also, I missed one in kernel/trace/trace_output.c
Added for next version.
thanks,
Tobin.
Re: [PATCH 3/3] trace: print address if symbol not found
On Mon, Dec 18, 2017 at 11:49:47AM -0500, Steven Rostedt wrote:
> On Mon, 18 Dec 2017 10:53:32 +1100
> "Tobin C. Harding" wrote:
>
> > Fixes behaviour modified by: commit bd6b239cdbb2 ("kallsyms: don't leak
> > address when symbol not found")
> >
> > Previous patch changed behaviour of kallsyms function sprint_symbol() to
> > return an error code instead of printing the address if a symbol was not
> > found. Ftrace relies on the original behaviour. We should not break
> > tracing when applying the previous patch. We can maintain the original
> > behaviour by checking the return code on calls to sprint_symbol() and
> > friends.
> >
> > Check return code and print actual address on error (i.e symbol not
> > found).
> >
> > Signed-off-by: Tobin C. Harding
> > ---
> > kernel/trace/trace.h | 24
> > kernel/trace/trace_events_hist.c | 6 +++---
> > 2 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index 2a6d0325a761..881b1a577d75 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -1814,4 +1814,28 @@ static inline void trace_event_eval_update(struct
> > trace_eval_map **map, int len)
> >
> > extern struct trace_iterator *tracepoint_print_iter;
> >
> > +static inline int
> > +trace_sprint_symbol(char *buffer, unsigned long address)
> > +{
> > + int ret;
> > +
> > + ret = sprint_symbol(buffer, address);
> > + if (ret == -1)
> > + ret = sprintf(buffer, "0x%lx", address);
> > +
> > + return ret;
> > +}
> > +
> > +static inline int
> > +trace_sprint_symbol_no_offset(char *buffer, unsigned long address)
> > +{
> > + int ret;
> > +
> > + ret = sprint_symbol_no_offset(buffer, address);
> > + if (ret == -1)
> > + ret = sprintf(buffer, "0x%lx", address);
> > +
> > + return ret;
> > +}
> > +
> > #endif /* _LINUX_KERNEL_TRACE_H */
> > diff --git a/kernel/trace/trace_events_hist.c
> > b/kernel/trace/trace_events_hist.c
> > index 1e1558c99d56..3e28522a76f4 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -982,7 +982,7 @@ static void hist_trigger_stacktrace_print(struct
> > seq_file *m,
> > return;
> >
> > seq_printf(m, "%*c", 1 + spaces, ' ');
> > - sprint_symbol(str, stacktrace_entries[i]);
> > + trace_sprint_symbol_addr(str, stacktrace_entries[i]);
>
> Hmm, where is trace_sprint_symbol_addr() defined?
Gee, seems I can't build a kernel and I can't read a diff - epic
fail. Sorry for wasting your time Steve I'll try to be more careful.
FTR This should have been
- sprint_symbol(str, stacktrace_entries[i]);
+ trace_sprint_symbol(str, stacktrace_entries[i]);
If you have the time to give me some brief pointers on how I should go
about testing this I'd love to test it before the next version. I know
very little about ftrace.
thanks,
Tobin.
Re: [PATCH 3/3] trace: print address if symbol not found
On Mon, 18 Dec 2017 10:53:32 +1100
"Tobin C. Harding" wrote:
> Fixes behaviour modified by: commit bd6b239cdbb2 ("kallsyms: don't leak
> address when symbol not found")
>
> Previous patch changed behaviour of kallsyms function sprint_symbol() to
> return an error code instead of printing the address if a symbol was not
> found. Ftrace relies on the original behaviour. We should not break
> tracing when applying the previous patch. We can maintain the original
> behaviour by checking the return code on calls to sprint_symbol() and
> friends.
>
> Check return code and print actual address on error (i.e symbol not
> found).
>
> Signed-off-by: Tobin C. Harding
> ---
> kernel/trace/trace.h | 24
> kernel/trace/trace_events_hist.c | 6 +++---
> 2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 2a6d0325a761..881b1a577d75 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1814,4 +1814,28 @@ static inline void trace_event_eval_update(struct
> trace_eval_map **map, int len)
>
> extern struct trace_iterator *tracepoint_print_iter;
>
> +static inline int
> +trace_sprint_symbol(char *buffer, unsigned long address)
> +{
> + int ret;
> +
> + ret = sprint_symbol(buffer, address);
> + if (ret == -1)
> + ret = sprintf(buffer, "0x%lx", address);
> +
> + return ret;
> +}
> +
> +static inline int
> +trace_sprint_symbol_no_offset(char *buffer, unsigned long address)
> +{
> + int ret;
> +
> + ret = sprint_symbol_no_offset(buffer, address);
> + if (ret == -1)
> + ret = sprintf(buffer, "0x%lx", address);
> +
> + return ret;
> +}
> +
> #endif /* _LINUX_KERNEL_TRACE_H */
> diff --git a/kernel/trace/trace_events_hist.c
> b/kernel/trace/trace_events_hist.c
> index 1e1558c99d56..3e28522a76f4 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -982,7 +982,7 @@ static void hist_trigger_stacktrace_print(struct seq_file
> *m,
> return;
>
> seq_printf(m, "%*c", 1 + spaces, ' ');
> - sprint_symbol(str, stacktrace_entries[i]);
> + trace_sprint_symbol_addr(str, stacktrace_entries[i]);
Hmm, where is trace_sprint_symbol_addr() defined?
-- Steve
> seq_printf(m, "%s\n", str);
> }
> }
> @@ -1014,12 +1014,12 @@ hist_trigger_entry_print(struct seq_file *m,
> seq_printf(m, "%s: %llx", field_name, uval);
> } else if (key_field->flags & HIST_FIELD_FL_SYM) {
> uval = *(u64 *)(key + key_field->offset);
> - sprint_symbol_no_offset(str, uval);
> + trace_sprint_symbol_no_offset(str, uval);
> seq_printf(m, "%s: [%llx] %-45s", field_name,
> uval, str);
> } else if (key_field->flags & HIST_FIELD_FL_SYM_OFFSET) {
> uval = *(u64 *)(key + key_field->offset);
> - sprint_symbol(str, uval);
> + trace_sprint_symbol(str, uval);
> seq_printf(m, "%s: [%llx] %-55s", field_name,
> uval, str);
> } else if (key_field->flags & HIST_FIELD_FL_EXECNAME) {
