On Tue, Jun 28 2016, 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.
>
> 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 */
> 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
