linux-kernel - Re: [RFC/PATCH] lib/vsprintf: Add support to store cpumask

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: <878txpi066.fsf@rasmusvillemoes.dk>
Date:	Tue, 28 Jun 2016 21:27:29 +0200
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	Jiri Olsa <jolsa@...nel.org>, Lai Jiangshan <laijs@...fujitsu.com>,
	lkml <linux-kernel@...r.kernel.org>,
	Frederic Weisbecker <fweisbec@...il.com>
Subject: Re: [RFC/PATCH] lib/vsprintf: Add support to store cpumask

On Tue, Jun 28 2016, Steven Rostedt <rostedt@...dmis.org> wrote:

> On Tue, 28 Jun 2016 17:34:34 +0200
> Jiri Olsa <jolsa@...nel.org> wrote:
>
>> When using trace_printk for cpumask I've got wrong results,
>> some bitmaps were completely different from what I expected.
>> 
>> Currently you get wrong results when using trace_printk
>> on local cpumask, like:
>> 
>>   void test(void)
>>   {
>>       struct cpumask mask;
>>       ...
>>       trace_printk("mask '%*pbl'\n", cpumask_pr_args(&mask));
>>   }
>> 
>> The reason is that trace_printk stores the data into binary
>> buffer (pointer for cpumask), which is read after via read
>> handler of trace/trace_pipe files. At that time pointer for
>> local cpumask is no longer valid and you get wrong data.
>> 
>> Fixing this by storing complete cpumask into tracing buffer.
>
> The thing is, this is basically true with all pointer derivatives
> (just look at the list of options under pointer()). 

Yeah, back in December I asked what made this pointer stashing safe, but
I guess the answer is that it simply isn't, and we currently rely
on nobody using the more advanced %p extensions with trace_printk
(e.g. %pD that could easily end up not just following the pointer, but
also interpret the pointed-to memory as a pointer).

> I probably should make a trace_printk() that doesn't default to the
> binary print, to handle things like this.
>
>   trace_printk_ptr()?
>
> Or even just see if I can find a way that detects this in the fmt
> string. Hmm, that probably can't be done at compile time :-/

Well, not with gcc itself, but it wouldn't be too hard to make smatch
complain loudly if trace_printk is used on a format string with any %p
extension (directing people to use trace_printk_ptr()) - the format
parsing (and type checking) is already there.

>> Cc: Steven Rostedt <rostedt@...dmis.org>
>> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
>> ---
>>  lib/vsprintf.c | 41 ++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 36 insertions(+), 5 deletions(-)
>> 
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index 0967771d8f7f..f21d68e1b5fc 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -388,7 +388,8 @@ enum format_type {
>>  struct printf_spec {
>>  	unsigned int	type:8;		/* format_type enum */
>>  	signed int	field_width:24;	/* width of output field */
>> -	unsigned int	flags:8;	/* flags to number() */
>> +	unsigned int	flags:7;	/* flags to number() */
>> +	unsigned int	cpumask:1;	/* pointer to cpumask flag */
>
> Why not just add this as another flag? There's one left. I'm not sure
> gcc does nice things with bit fields not a multiple of 8.

I really don't think we should pollute the common printf code with this
stuff, partly because of the code generation issues, but also: what
should we do the next time someone decides to handle a %p extension more
correctly in vbin_printf?

Rasmus