linux-kernel - Re: [PATCH] tracing: use %ps format string to print symbols

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211015095704.49a99859@gandalf.local.home>
Date:   Fri, 15 Oct 2021 09:57:04 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Arnd Bergmann <arnd@...nel.org>
Cc:     Ingo Molnar <mingo@...hat.com>, Arnd Bergmann <arnd@...db.de>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Tom Zanussi <zanussi@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Jiapeng Chong <jiapeng.chong@...ux.alibaba.com>,
        linux-kernel@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH] tracing: use %ps format string to print symbols

On Fri, 15 Oct 2021 10:34:31 +0200
Arnd Bergmann <arnd@...nel.org> wrote:

> From: Arnd Bergmann <arnd@...db.de>
> 
> clang started warning about excessive stack usage in
> hist_trigger_print_key()
> 
> kernel/trace/trace_events_hist.c:4723:13: error: stack frame size (1336) exceeds limit (1024) in function 'hist_trigger_print_key' [-Werror,-Wframe-larger-than]
> 
> The problem is that there are two 512-byte arrays on the stack if
> hist_trigger_stacktrace_print() gets inlined. I don't think this has
> changed in the past five years, but something probably changed the
> inlining decisions made by the compiler, so the problem is now made
> more obvious.

Could we just add "noinline" attribute to that function?

> 
> Rather than printing the symbol names into separate buffers, it
> seems we can simply use the special %ps format string modifier
> to print the pointers symbolically and get rid of both buffers.
> 
> Fixes: 69a0200c2e25 ("tracing: Add hist trigger support for stacktraces as keys")
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
> I'm not sure if the %ps format string actually works as intended
> with the explicit length modifier, it would be good if someone
> could try this out before applying. Would it be ok to remove the
> length modifier otherwise?

I believe that shows a "table" format. So the length modifier is required.

> ---
>  kernel/trace/trace_events_hist.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index a6061a69aa84..640c79898b51 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -4706,7 +4706,6 @@ static void hist_trigger_stacktrace_print(struct seq_file *m,
>  					  unsigned long *stacktrace_entries,
>  					  unsigned int max_entries)
>  {
> -	char str[KSYM_SYMBOL_LEN];
>  	unsigned int spaces = 8;
>  	unsigned int i;
>  
> @@ -4715,8 +4714,7 @@ static void hist_trigger_stacktrace_print(struct seq_file *m,
>  			return;
>  
>  		seq_printf(m, "%*c", 1 + spaces, ' ');
> -		sprint_symbol(str, stacktrace_entries[i]);
> -		seq_printf(m, "%s\n", str);
> +		seq_printf(m, "%pS\n", stacktrace_entries[i]);
>  	}
>  }
>  
> @@ -4726,7 +4724,6 @@ static void hist_trigger_print_key(struct seq_file *m,
>  				   struct tracing_map_elt *elt)
>  {
>  	struct hist_field *key_field;
> -	char str[KSYM_SYMBOL_LEN];
>  	bool multiline = false;
>  	const char *field_name;
>  	unsigned int i;
> @@ -4747,14 +4744,12 @@ static void hist_trigger_print_key(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);
> -			seq_printf(m, "%s: [%llx] %-45s", field_name,
> -				   uval, str);
> +			seq_printf(m, "%s: [%llx] %-45ps", field_name,
> +				   uval, (void *)uval);
>  		} else if (key_field->flags & HIST_FIELD_FL_SYM_OFFSET) {
>  			uval = *(u64 *)(key + key_field->offset);
> -			sprint_symbol(str, uval);
> -			seq_printf(m, "%s: [%llx] %-55s", field_name,
> -				   uval, str);
> +			seq_printf(m, "%s: [%llx] %-55ps", field_name,
> +				   uval, (void *)uval);

The above requires "Ps" not "ps".

But other than that, I could test if this doesn't change the formatting and
functionality at all.

-- Steve


>  		} else if (key_field->flags & HIST_FIELD_FL_EXECNAME) {
>  			struct hist_elt_data *elt_data = elt->private_data;
>  			char *comm;