From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752431AbcF1T1f (ORCPT ); Tue, 28 Jun 2016 15:27:35 -0400 Received: from mail-wm0-f53.google.com ([74.125.82.53]:35246 "EHLO mail-wm0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752251AbcF1T1d (ORCPT ); Tue, 28 Jun 2016 15:27:33 -0400 From: Rasmus Villemoes To: Steven Rostedt Cc: Jiri Olsa , Lai Jiangshan , lkml , Frederic Weisbecker Subject: Re: [RFC/PATCH] lib/vsprintf: Add support to store cpumask Organization: D03 References: <1467128075-10841-1-git-send-email-jolsa@kernel.org> <20160628122608.0ea291b2@gandalf.local.home> X-Hashcash: 1:20:160628:linux-kernel@vger.kernel.org::EeYQ1bTQpLvNKcvs:00000000000000000000000000000000006lA X-Hashcash: 1:20:160628:fweisbec@gmail.com::M0jWy1FrlGTNI4an:00000000000000000000000000000000000000000000OWl X-Hashcash: 1:20:160628:rostedt@goodmis.org::xK/Hr3mOIAPg6pm3:0000000000000000000000000000000000000000002Vnw X-Hashcash: 1:20:160628:jolsa@kernel.org::0LXlkoqeE9pkK4j4:01v88 X-Hashcash: 1:20:160628:laijs@cn.fujitsu.com::sbUsb6jTFaOJ5c61:000000000000000000000000000000000000000003XBp Date: Tue, 28 Jun 2016 21:27:29 +0200 In-Reply-To: <20160628122608.0ea291b2@gandalf.local.home> (Steven Rostedt's message of "Tue, 28 Jun 2016 12:26:08 -0400") Message-ID: <878txpi066.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 28 2016, Steven Rostedt wrote: > On Tue, 28 Jun 2016 17:34:34 +0200 > Jiri Olsa 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 >> Signed-off-by: Jiri Olsa >> --- >> 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