lib/vsprintf: Add support to store cpumask

RFC/PATCH Jiri Olsa 4 participants 11 messages Jun 28, 2016 – Jun 29, 2016

Subsystems (2)

Library code Vsprintf

lore

Share & tools

lore

Jiri Olsa

author

2016-06-28 15:34 UTC #0 lore raw

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 <[email protected]>

Signed-off-by Jiri Olsa <[email protected]>

---
 lib/vsprintf.c | 41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

lib/vsprintf.c +36 -5

index 0967771d8f7f..f21d68e1b5fc 100644
@@ -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 = '%';

Signature · 1 line

2.4.11

Steven Rostedt

maintainer

2016-06-28 16:26 UTC #1 lore raw

Quoted 13 lines · On Tue, 28 Jun 2016 17:34:34 +0200 Jiri Olsa <[email protected]> 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));
  }

Quoted 6 lines

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 :-/

Quoted 12 lines

Cc Steven Rostedt <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
 lib/vsprintf.c | 41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)
lib/vsprintf.c +0 -0
index 0967771d8f7f..f21d68e1b5fc 100644

lib/vsprintf.c enum format_type { · line 392

Quoted 6 lines

lib/vsprintf.c +2 -1

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

Quoted 65 lines

lib/vsprintf.c +34 -4

 	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 = '%';

Rasmus Villemoes

reviewer

2016-06-28 19:27 UTC #2 lore raw

Quoted 21 lines · Steven Rostedt

Quoted 18 lines · On Tue, 28 Jun 2016 17:34:34 +0200 Jiri Olsa <[email protected]> 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.

Quoted 4 lines

Quoted 1 line
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).

Quoted 7 lines

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.

Quoted 6 lines

Quoted 6 lines
Cc Steven Rostedt <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
 lib/vsprintf.c | 41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

Quoted 14 lines

lib/vsprintf.c enum format_type { · line 392
Quoted 11 lines
lib/vsprintf.c +2 -1
index 0967771d8f7f..f21d68e1b5fc 100644
@@ -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

Steven Rostedt

maintainer

2016-06-28 19:57 UTC #3 lore raw

Quoted 5 lines · On Tue, 28 Jun 2016 21:27:29 +0200 Rasmus Villemoes <[email protected]>…

Quoted 5 lines
I probably should make a trace_printk() that doesn't default to the binary print, to handle things like this.
trace_printk_ptr()?

Quoted 7 lines

Quoted 2 lines
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.

Quoted 22 lines

Quoted 7 lines
Quoted 7 lines
Cc Steven Rostedt <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
 lib/vsprintf.c | 41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)
Quoted 14 lines
lib/vsprintf.c enum format_type { · line 392
Quoted 11 lines
lib/vsprintf.c +2 -1
index 0967771d8f7f..f21d68e1b5fc 100644
@@ -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.

Quoted 4 lines

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

Rasmus Villemoes

reviewer

2016-06-28 21:50 UTC #4 lore raw

Quoted 7 lines · Steven Rostedt

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

Steven Rostedt

maintainer

2016-06-28 22:08 UTC #5 lore raw

Quoted 10 lines · On Tue, 28 Jun 2016 23:50:27 +0200 Rasmus Villemoes <[email protected]>…

Quoted 7 lines · Steven Rostedt
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.

Quoted 3 lines

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

Steven Rostedt

maintainer

2016-06-28 22:08 UTC #6 lore raw

In reply to Rasmus Villemoes

Quoted 5 lines · On Tue, 28 Jun 2016 21:27:29 +0200 Rasmus Villemoes <[email protected]>…

Quoted 5 lines
I probably should make a trace_printk() that doesn't default to the binary print, to handle things like this.
trace_printk_ptr()?

Quoted 7 lines

Quoted 2 lines
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

include/linux/kernel.h +18 -1

index 94aa10ffe156..62693900cc4b 100644
@@ -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);			\

Jiri Olsa

2016-06-29 06:43 UTC #7 lore raw

Quoted 45 lines · Steven Rostedt

Quoted 5 lines · On Tue, 28 Jun 2016 21:27:29 +0200 Rasmus Villemoes <[email protected]>…
Quoted 5 lines
I probably should make a trace_printk() that doesn't default to the binary print, to handle things like this.
trace_printk_ptr()?
Quoted 7 lines
Quoted 2 lines
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
include/linux/kernel.h +11 -1
index 94aa10ffe156..62693900cc4b 100644
@@ -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") &&				\

include/linux/kernel.h do { \ · line 654

Quoted 10 lines

include/linux/kernel.h +7 -0

+	    !__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

Steven Rostedt

maintainer

2016-06-29 15:42 UTC #8 lore raw

Quoted 25 lines · On Wed, 29 Jun 2016 08:43:48 +0200 Jiri Olsa <[email protected]> wrote:

Quoted 25 lines

include/linux/kernel.h +15 -1

index 94aa10ffe156..62693900cc4b 100644
@@ -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") &&				\

Quoted 8 lines

include/linux/kernel.h do { \ · line 654
Quoted 6 lines
include/linux/kernel.h +3 -0
+	    !__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

Rasmus Villemoes

reviewer

2016-06-28 21:20 UTC #9 lore raw

In reply to Jiri Olsa

Quoted 49 lines · Jiri Olsa

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 <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
 lib/vsprintf.c | 41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)
lib/vsprintf.c +3 -1
index 0967771d8f7f..f21d68e1b5fc 100644
@@ -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 '%':

lib/vsprintf.c do { \ · line 2343

Quoted 5 lines

lib/vsprintf.c +1 -1

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

lib/vsprintf.c do { \ · line 2353

Quoted 10 lines

lib/vsprintf.c +10 -0

+				/*
+				 * 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

Jiri Olsa

2016-06-29 07:12 UTC #10 lore raw

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

SNIP

Quoted 6 lines

lib/vsprintf.c do { \ · line 2343
Quoted 2 lines
lib/vsprintf.c +1 -1
-			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.

Quoted 16 lines

lib/vsprintf.c do { \ · line 2353
Quoted 10 lines
lib/vsprintf.c +10 -0
+				/*
+				 * 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