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