Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1433067 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC/PATCH] lib/vsprintf: Add support to store cpumask Jiri Olsa <jolsa@kernel.org> - 2016-06-28 17:40 +0200
    Re: [RFC/PATCH] lib/vsprintf: Add support to store cpumask Steven Rostedt <rostedt@goodmis.org> - 2016-06-28 18:30 +0200
      Re: [RFC/PATCH] lib/vsprintf: Add support to store cpumask Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-06-28 21:30 +0200
        Re: [RFC/PATCH] lib/vsprintf: Add support to store cpumask Steven Rostedt <rostedt@goodmis.org> - 2016-06-28 22:00 +0200
          Re: [RFC/PATCH] lib/vsprintf: Add support to store cpumask Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-06-29 00:00 +0200
            Re: [RFC/PATCH] lib/vsprintf: Add support to store cpumask Steven Rostedt <rostedt@goodmis.org> - 2016-06-29 00:10 +0200
        Re: [RFC/PATCH] lib/vsprintf: Add support to store cpumask Steven Rostedt <rostedt@goodmis.org> - 2016-06-29 00:10 +0200
          Re: [RFC/PATCH] lib/vsprintf: Add support to store cpumask Jiri Olsa <jolsa@redhat.com> - 2016-06-29 08:50 +0200
            Re: [RFC/PATCH] lib/vsprintf: Add support to store cpumask Steven Rostedt <rostedt@goodmis.org> - 2016-06-29 17:50 +0200
    Re: [RFC/PATCH] lib/vsprintf: Add support to store cpumask Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-06-28 23:30 +0200
      Re: [RFC/PATCH] lib/vsprintf: Add support to store cpumask Jiri Olsa <jolsa@redhat.com> - 2016-06-29 09:20 +0200

#1433067 — [RFC/PATCH] lib/vsprintf: Add support to store cpumask

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.

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 */
 	unsigned int	base:8;		/* number base, 8, 10 or 16 only */
 	signed int	precision:16;	/* # of digits/chars */
 } __packed;
@@ -1864,6 +1865,7 @@ qualifier:
 
 	case 'p':
 		spec->type = FORMAT_TYPE_PTR;
+		spec->cpumask = fmt[1] == 'b';
 		return ++fmt - start;
 
 	case '%':
@@ -2338,7 +2340,23 @@ do {									\
 		}
 
 		case FORMAT_TYPE_PTR:
-			save_arg(void *);
+			if (spec.cpumask) {
+				/*
+				 * Store entire cpumask directly to buffer
+				 * instead of storing just a pointer.
+				 */
+				struct cpumask *mask = va_arg(args, void *);
+
+				str = PTR_ALIGN(str, sizeof(u32));
+
+				if (str + sizeof(*mask) <= end)
+					cpumask_copy((struct cpumask *) str, mask);
+
+				str += sizeof(*mask);
+			} else {
+				save_arg(void *);
+			}
+
 			/* skip all alphanumeric pointer suffixes */
 			while (isalnum(*fmt))
 				fmt++;
@@ -2490,12 +2508,25 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 			break;
 		}
 
-		case FORMAT_TYPE_PTR:
-			str = pointer(fmt, str, end, get_arg(void *), spec);
+		case FORMAT_TYPE_PTR: {
+			void *ptr;
+
+			if (spec.cpumask) {
+				/*
+				 * Load cpumask directly from buffer.
+				 */
+				args  = PTR_ALIGN(args, sizeof(u32));
+				ptr   = (void *) args;
+				args += sizeof(struct cpumask);
+			} else {
+				ptr = get_arg(void *);
+			}
+
+			str = pointer(fmt, str, end, ptr, spec);
 			while (isalnum(*fmt))
 				fmt++;
 			break;
-
+		}
 		case FORMAT_TYPE_PERCENT_CHAR:
 			if (str < end)
 				*str = '%';
-- 
2.4.11

[toc] | [next] | [standalone]

#1433099

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()). 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 :-/



> 
> 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.


-- Steve


>  	unsigned int	base:8;		/* number base, 8, 10 or 16 only */
>  	signed int	precision:16;	/* # of digits/chars */
>  } __packed;
> @@ -1864,6 +1865,7 @@ qualifier:
>  
>  	case 'p':
>  		spec->type = FORMAT_TYPE_PTR;
> +		spec->cpumask = fmt[1] == 'b';
>  		return ++fmt - start;
>  
>  	case '%':
> @@ -2338,7 +2340,23 @@ do {									\
>  		}
>  
>  		case FORMAT_TYPE_PTR:
> -			save_arg(void *);
> +			if (spec.cpumask) {
> +				/*
> +				 * Store entire cpumask directly to buffer
> +				 * instead of storing just a pointer.
> +				 */
> +				struct cpumask *mask = va_arg(args, void *);
> +
> +				str = PTR_ALIGN(str, sizeof(u32));
> +
> +				if (str + sizeof(*mask) <= end)
> +					cpumask_copy((struct cpumask *) str, mask);
> +
> +				str += sizeof(*mask);
> +			} else {
> +				save_arg(void *);
> +			}
> +
>  			/* skip all alphanumeric pointer suffixes */
>  			while (isalnum(*fmt))
>  				fmt++;
> @@ -2490,12 +2508,25 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
>  			break;
>  		}
>  
> -		case FORMAT_TYPE_PTR:
> -			str = pointer(fmt, str, end, get_arg(void *), spec);
> +		case FORMAT_TYPE_PTR: {
> +			void *ptr;
> +
> +			if (spec.cpumask) {
> +				/*
> +				 * Load cpumask directly from buffer.
> +				 */
> +				args  = PTR_ALIGN(args, sizeof(u32));
> +				ptr   = (void *) args;
> +				args += sizeof(struct cpumask);
> +			} else {
> +				ptr = get_arg(void *);
> +			}
> +
> +			str = pointer(fmt, str, end, ptr, spec);
>  			while (isalnum(*fmt))
>  				fmt++;
>  			break;
> -
> +		}
>  		case FORMAT_TYPE_PERCENT_CHAR:
>  			if (str < end)
>  				*str = '%';

[toc] | [prev] | [next] | [standalone]

#1433228

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

[toc] | [prev] | [next] | [standalone]

#1433255

On Tue, 28 Jun 2016 21:27:29 +0200
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:


> > 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.

But people don't usually run "make smatch" on debug kernels.
trace_printk() currently is not allowed to be used in the kernel. When
it is used, a big ugly banner is posted on boot up saying that the
kernel is in "debug mode" and is "unstable" (even though it isn't) just
to scare people enough to never compile with a trace_printk() in their
code. If they need a permanent trace_printk() then they need to use
tracepoints.

> 
> >> 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?


Perhaps we should add an WARN_ON is these are used in vbin_printf().

I'll go and make a trace_printk_ptr() patch.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1433343

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

> But people don't usually run "make smatch" on debug kernels.
> trace_printk() currently is not allowed to be used in the kernel. When
> it is used, a big ugly banner is posted on boot up saying that the
> kernel is in "debug mode" and is "unstable" (even though it isn't) just
> to scare people enough to never compile with a trace_printk() in their
> code. If they need a permanent trace_printk() then they need to use
> tracepoints.

But don't tracepoints also pass through vbin_printf? IOW, are there no
print-like function calls in the kernel where a static checker warning
for %pX would be worthwhile?

Rasmus

[toc] | [prev] | [next] | [standalone]

#1433346

On Tue, 28 Jun 2016 23:50:27 +0200
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On Tue, Jun 28 2016, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > But people don't usually run "make smatch" on debug kernels.
> > trace_printk() currently is not allowed to be used in the kernel. When
> > it is used, a big ugly banner is posted on boot up saying that the
> > kernel is in "debug mode" and is "unstable" (even though it isn't) just
> > to scare people enough to never compile with a trace_printk() in their
> > code. If they need a permanent trace_printk() then they need to use
> > tracepoints.  
> 
> But don't tracepoints also pass through vbin_printf? IOW, are there no
> print-like function calls in the kernel where a static checker warning
> for %pX would be worthwhile?


tracepoints don't use vbin_printf() but you are right, they do save the
data in binary format parse them out later.

Perhaps we should set up something in the static checker to test for
this in TP_printk() within the TRACE_EVENT() macros.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1433348

On Tue, 28 Jun 2016 21:27:29 +0200
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> > 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.

Well, actually gcc can (see below). Although, the more I think about
this, the more I'm thinking that the bin_printk() should be default
just copy the pointer content. As the whole point of bin_printk() is to
print the content at another time. And since pointers should not be
dereferenced later, it should be saved at the moment the bprintk() is
called and not dereferenced later.

-- Steve

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 94aa10ffe156..62693900cc4b 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -631,7 +631,24 @@ do {									\
 									\
 	__trace_printk_check_format(fmt, ##args);			\
 									\
-	if (__builtin_constant_p(fmt))					\
+	if (__builtin_constant_p(fmt) &&				\
+	    !__builtin_strstr(fmt, "%pF") &&				\
+	    !__builtin_strstr(fmt, "%pf") &&				\
+	    !__builtin_strstr(fmt, "%pR") &&				\
+	    !__builtin_strstr(fmt, "%pr") &&				\
+	    !__builtin_strstr(fmt, "%pb") &&				\
+	    !__builtin_strstr(fmt, "%pM") &&				\
+	    !__builtin_strstr(fmt, "%pI") &&				\
+	    !__builtin_strstr(fmt, "%pE") &&				\
+	    !__builtin_strstr(fmt, "%pU") &&				\
+	    !__builtin_strstr(fmt, "%pV") &&				\
+	    !__builtin_strstr(fmt, "%pN") &&				\
+	    !__builtin_strstr(fmt, "%pa") &&				\
+	    !__builtin_strstr(fmt, "%pd") &&				\
+	    !__builtin_strstr(fmt, "%pC") &&				\
+	    !__builtin_strstr(fmt, "%pD") &&				\
+	    !__builtin_strstr(fmt, "%pg") &&				\
+	    !__builtin_strstr(fmt, "%pG"))				\
 		__trace_bprintk(_THIS_IP_, trace_printk_fmt, ##args);	\
 	else								\
 		__trace_printk(_THIS_IP_, fmt, ##args);			\

[toc] | [prev] | [next] | [standalone]

#1433486

On Tue, Jun 28, 2016 at 06:06:47PM -0400, Steven Rostedt wrote:
> On Tue, 28 Jun 2016 21:27:29 +0200
> Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
> > > 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.
> 
> Well, actually gcc can (see below). Although, the more I think about
> this, the more I'm thinking that the bin_printk() should be default
> just copy the pointer content. As the whole point of bin_printk() is to
> print the content at another time. And since pointers should not be
> dereferenced later, it should be saved at the moment the bprintk() is
> called and not dereferenced later.
> 
> -- Steve
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 94aa10ffe156..62693900cc4b 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -631,7 +631,24 @@ do {									\
>  									\
>  	__trace_printk_check_format(fmt, ##args);			\
>  									\
> -	if (__builtin_constant_p(fmt))					\
> +	if (__builtin_constant_p(fmt) &&				\
> +	    !__builtin_strstr(fmt, "%pF") &&				\
> +	    !__builtin_strstr(fmt, "%pf") &&				\
> +	    !__builtin_strstr(fmt, "%pR") &&				\
> +	    !__builtin_strstr(fmt, "%pr") &&				\
> +	    !__builtin_strstr(fmt, "%pb") &&				\
> +	    !__builtin_strstr(fmt, "%pM") &&				\
> +	    !__builtin_strstr(fmt, "%pI") &&				\
> +	    !__builtin_strstr(fmt, "%pE") &&				\
> +	    !__builtin_strstr(fmt, "%pU") &&				\
> +	    !__builtin_strstr(fmt, "%pV") &&				\
> +	    !__builtin_strstr(fmt, "%pN") &&				\
> +	    !__builtin_strstr(fmt, "%pa") &&				\
> +	    !__builtin_strstr(fmt, "%pd") &&				\
> +	    !__builtin_strstr(fmt, "%pC") &&				\
> +	    !__builtin_strstr(fmt, "%pD") &&				\
> +	    !__builtin_strstr(fmt, "%pg") &&				\
> +	    !__builtin_strstr(fmt, "%pG"))				\
>  		__trace_bprintk(_THIS_IP_, trace_printk_fmt, ##args);	\
>  	else								\
>  		__trace_printk(_THIS_IP_, fmt, ##args);			\

is this one working? it seems better to me than adding new trace_printk_ptr


jirka

[toc] | [prev] | [next] | [standalone]

#1433872

On Wed, 29 Jun 2016 08:43:48 +0200
Jiri Olsa <jolsa@redhat.com> wrote:

> 
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index 94aa10ffe156..62693900cc4b 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -631,7 +631,24 @@ do {									\
> >  									\
> >  	__trace_printk_check_format(fmt, ##args);			\
> >  									\
> > -	if (__builtin_constant_p(fmt))					\
> > +	if (__builtin_constant_p(fmt) &&				\
> > +	    !__builtin_strstr(fmt, "%pF") &&				\
> > +	    !__builtin_strstr(fmt, "%pf") &&				\
> > +	    !__builtin_strstr(fmt, "%pR") &&				\
> > +	    !__builtin_strstr(fmt, "%pr") &&				\
> > +	    !__builtin_strstr(fmt, "%pb") &&				\
> > +	    !__builtin_strstr(fmt, "%pM") &&				\
> > +	    !__builtin_strstr(fmt, "%pI") &&				\
> > +	    !__builtin_strstr(fmt, "%pE") &&				\
> > +	    !__builtin_strstr(fmt, "%pU") &&				\
> > +	    !__builtin_strstr(fmt, "%pV") &&				\
> > +	    !__builtin_strstr(fmt, "%pN") &&				\
> > +	    !__builtin_strstr(fmt, "%pa") &&				\
> > +	    !__builtin_strstr(fmt, "%pd") &&				\
> > +	    !__builtin_strstr(fmt, "%pC") &&				\
> > +	    !__builtin_strstr(fmt, "%pD") &&				\
> > +	    !__builtin_strstr(fmt, "%pg") &&				\
> > +	    !__builtin_strstr(fmt, "%pG"))				\
> >  		__trace_bprintk(_THIS_IP_, trace_printk_fmt, ##args);	\
> >  	else								\
> >  		__trace_printk(_THIS_IP_, fmt, ##args);			\  
> 
> is this one working? it seems better to me than adding new trace_printk_ptr

The problem with this is that it would require a bunch more conditions
as you see it would not have helped your own use case (requiring the *p)

I'm actually looking into extending your patch to handle all cases that
vbin_printf() is an issue.

Thinking about it more, as vbin_printf() is a two part process, it
should by default not depend on any dereferencing, as I can't think of
any use case for it where it wouldn't be an issue.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1433337

On Tue, Jun 28 2016, 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.
>
> 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 */
>  	unsigned int	base:8;		/* number base, 8, 10 or 16 only */
>  	signed int	precision:16;	/* # of digits/chars */
>  } __packed;
> @@ -1864,6 +1865,7 @@ qualifier:
>  
>  	case 'p':
>  		spec->type = FORMAT_TYPE_PTR;
> +		spec->cpumask = fmt[1] == 'b';
>  		return ++fmt - start;
>  
>  	case '%':
> @@ -2338,7 +2340,23 @@ do {									\
>  		}
>  
>  		case FORMAT_TYPE_PTR:
> -			save_arg(void *);
> +			if (spec.cpumask) {

As I hinted in the other mail, I think it's better just to put the
fmt[1]=='b' here and not change struct printf_spec.

> +				/*
> +				 * Store entire cpumask directly to buffer
> +				 * instead of storing just a pointer.
> +				 */
> +				struct cpumask *mask = va_arg(args, void *);
> +
> +				str = PTR_ALIGN(str, sizeof(u32));
> +
> +				if (str + sizeof(*mask) <= end)
> +					cpumask_copy((struct cpumask *) str, mask);

A cpumask is an array of longs. Why is u32-alignment enough for that?
cpumask_copy may end up compiling to a simple "*dst = *src", and even if
this is a memcpy(), the same 4-but-possibly-not-8 byte aligned
pointer is created below in bstr_printf which is then passed on to
pointer() and then bitmap_* which certainly expects an unsigned long*. 

Rasmus

[toc] | [prev] | [next] | [standalone]

#1433497

On Tue, Jun 28, 2016 at 11:19:59PM +0200, Rasmus Villemoes wrote:

SNIP

> > -			save_arg(void *);
> > +			if (spec.cpumask) {
> 
> As I hinted in the other mail, I think it's better just to put the
> fmt[1]=='b' here and not change struct printf_spec.
> 
> > +				/*
> > +				 * Store entire cpumask directly to buffer
> > +				 * instead of storing just a pointer.
> > +				 */
> > +				struct cpumask *mask = va_arg(args, void *);
> > +
> > +				str = PTR_ALIGN(str, sizeof(u32));
> > +
> > +				if (str + sizeof(*mask) <= end)
> > +					cpumask_copy((struct cpumask *) str, mask);
> 
> A cpumask is an array of longs. Why is u32-alignment enough for that?
> cpumask_copy may end up compiling to a simple "*dst = *src", and even if
> this is a memcpy(), the same 4-but-possibly-not-8 byte aligned
> pointer is created below in bstr_printf which is then passed on to
> pointer() and then bitmap_* which certainly expects an unsigned long*. 

hum, the binary buffer is copied after to trace buffer which
has 32bit alignment I think, so bigger alignment breaks the
data.. also the save_arg macro does 32bit alignment for 8 bytes

anyway, I haven't checked deeply on this, was just hunting
wrong cpumask, now with Steven's changes I'm ok to drop it ;-)

thanks,
jirka

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web