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

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

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

> On Tue, 28 Jun 2016 17:34:34 +0200
> Jiri Olsa <jolsa@kernel.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@goodmis.org>
>> Signed-off-by: Jiri Olsa <jolsa@kernel.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