[PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces

[PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces

[Kapileshwar Singh]

When a trace recorded on a 32-bit device is processed with a 64-bit
binary, the higher 32-bits of the address need to be masked.

The lack of this results in the output of the 64-bit pointer
value to the trace as the 32-bit address lookup fails in find_printk.

Before:
burn-1778  [003]   548.600305: bputs:   0xc0046db2s: 2cec5c058d98c

After:
burn-1778  [003]   548.600305: bputs:	0xc0046db2s: RT throttling activated

The problem occurs in PRINT_FEILD when the field is recognized as a pointer
to a string (of the type const char *)

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Javi Merino <javi.merino@arm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Reported-by: Juri-Lelli <juri.lelli@arm.com>
Signed-off-by: Kapileshwar Singh <kapileshwar.singh@arm.com>
---
 tools/lib/traceevent/event-parse.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 4d885934b919..39163ea4a048 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -3829,6 +3829,17 @@ static void print_str_arg(struct trace_seq *s, void *data, int size,
 		if (!(field->flags & FIELD_IS_ARRAY) &&
 		    field->size == pevent->long_size) {
 			addr = *(unsigned long *)(data + field->offset);
+
+			/* In case the long_size is 4. The higher 32bits
+			 * need to be masked for a successful lookup in
+			 * in the printk table. As the pointers are 32-bit
+			 * long. This could happen if a trace recorded on
+			 * 32-bit platform is processed using a 64-bit
+			 * binary
+			 */
+			if (pevent->long_size == 4)
+				addr = addr & 0xffffffff;
+
 			/* Check if it matches a print format */
 			printk = find_printk(pevent, addr);
 			if (printk)
-- 
1.9.1

Re: [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces

[Steven Rostedt]

On Thu, 17 Sep 2015 12:14:36 +0100
Kapileshwar Singh <kapileshwar.singh@arm.com> wrote:

> When a trace recorded on a 32-bit device is processed with a 64-bit
> binary, the higher 32-bits of the address need to be masked.
> 
> The lack of this results in the output of the 64-bit pointer
> value to the trace as the 32-bit address lookup fails in find_printk.
> 
> Before:
> burn-1778  [003]   548.600305: bputs:   0xc0046db2s: 2cec5c058d98c
> 
> After:
> burn-1778  [003]   548.600305: bputs:	0xc0046db2s: RT throttling activated
> 
> The problem occurs in PRINT_FEILD when the field is recognized as a pointer
> to a string (of the type const char *)

Actually, there's two bugs here. You only fixed one of them.

> 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Javi Merino <javi.merino@arm.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Reported-by: Juri-Lelli <juri.lelli@arm.com>
> Signed-off-by: Kapileshwar Singh <kapileshwar.singh@arm.com>
> ---
>  tools/lib/traceevent/event-parse.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> index 4d885934b919..39163ea4a048 100644
> --- a/tools/lib/traceevent/event-parse.c
> +++ b/tools/lib/traceevent/event-parse.c
> @@ -3829,6 +3829,17 @@ static void print_str_arg(struct trace_seq *s, void *data, int size,
>  		if (!(field->flags & FIELD_IS_ARRAY) &&
>  		    field->size == pevent->long_size) {
>  			addr = *(unsigned long *)(data + field->offset);

addr is of type unsigned long. That means if we read a 64 bit record on
a 32 bit machine (which is supported), this will be truncated.

Perhaps we need to make addr into a unsigned long long, and then add:

	addr = (pevent->long_size == 8) ?
		*(unsigned long long *)(data + field->offset) :
		(unsigned long long )*(unsigned int *)(data + field->offset);



-- Steve


> +
> +			/* In case the long_size is 4. The higher 32bits
> +			 * need to be masked for a successful lookup in
> +			 * in the printk table. As the pointers are 32-bit
> +			 * long. This could happen if a trace recorded on
> +			 * 32-bit platform is processed using a 64-bit
> +			 * binary
> +			 */
> +			if (pevent->long_size == 4)
> +				addr = addr & 0xffffffff;
> +
>  			/* Check if it matches a print format */
>  			printk = find_printk(pevent, addr);
>  			if (printk)

Re: [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces

[Kapileshwar Singh]

Hi Steve,

Thanks for looking into this!

On 17/09/15 14:11, Steven Rostedt wrote:
> On Thu, 17 Sep 2015 12:14:36 +0100
> Kapileshwar Singh <kapileshwar.singh@arm.com> wrote:
> 
>> When a trace recorded on a 32-bit device is processed with a 64-bit
>> binary, the higher 32-bits of the address need to be masked.
>>
>> The lack of this results in the output of the 64-bit pointer
>> value to the trace as the 32-bit address lookup fails in find_printk.
>>
>> Before:
>> burn-1778  [003]   548.600305: bputs:   0xc0046db2s: 2cec5c058d98c
>>
>> After:
>> burn-1778  [003]   548.600305: bputs:	0xc0046db2s: RT throttling activated
>>
>> The problem occurs in PRINT_FEILD when the field is recognized as a pointer
>> to a string (of the type const char *)
> 
> Actually, there's two bugs here. You only fixed one of them.
> 
>>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Javi Merino <javi.merino@arm.com>
>> Cc: David Ahern <dsahern@gmail.com>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Reported-by: Juri-Lelli <juri.lelli@arm.com>
>> Signed-off-by: Kapileshwar Singh <kapileshwar.singh@arm.com>
>> ---
>>  tools/lib/traceevent/event-parse.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
>> index 4d885934b919..39163ea4a048 100644
>> --- a/tools/lib/traceevent/event-parse.c
>> +++ b/tools/lib/traceevent/event-parse.c
>> @@ -3829,6 +3829,17 @@ static void print_str_arg(struct trace_seq *s, void *data, int size,
>>  		if (!(field->flags & FIELD_IS_ARRAY) &&
>>  		    field->size == pevent->long_size) {
>>  			addr = *(unsigned long *)(data + field->offset);
> 
> addr is of type unsigned long. That means if we read a 64 bit record on
> a 32 bit machine (which is supported), this will be truncated.
> 
> Perhaps we need to make addr into a unsigned long long, and then add:
> 
> 	addr = (pevent->long_size == 8) ?
> 		*(unsigned long long *)(data + field->offset) :
> 		(unsigned long long )*(unsigned int *)(data + field->offset);

I agree, we need to handle both cases:

* Traces recorded using 32-bit addresses processed on a 64-bit machine
* Traces recorded using 64-bit addresses processed on a 32-bit machine

The change you suggested fixes both these cases. 

Will send a v2 of the patch with the changes.

Regards, 
KP

> 
> 
> 
> -- Steve
> 
> 
>> +
>> +			/* In case the long_size is 4. The higher 32bits
>> +			 * need to be masked for a successful lookup in
>> +			 * in the printk table. As the pointers are 32-bit
>> +			 * long. This could happen if a trace recorded on
>> +			 * 32-bit platform is processed using a 64-bit
>> +			 * binary
>> +			 */
>> +			if (pevent->long_size == 4)
>> +				addr = addr & 0xffffffff;
>> +
>>  			/* Check if it matches a print format */
>>  			printk = find_printk(pevent, addr);
>>  			if (printk)
> 

Re: [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces

[Namhyung Kim]

Hi,

On Thu, Sep 17, 2015 at 11:58 PM, Kapileshwar Singh
<kapileshwar.singh@arm.com> wrote:
> Hi Steve,
>
> Thanks for looking into this!
>
> On 17/09/15 14:11, Steven Rostedt wrote:
>> On Thu, 17 Sep 2015 12:14:36 +0100
>> Kapileshwar Singh <kapileshwar.singh@arm.com> wrote:
>>
>>> When a trace recorded on a 32-bit device is processed with a 64-bit
>>> binary, the higher 32-bits of the address need to be masked.
>>>
>>> The lack of this results in the output of the 64-bit pointer
>>> value to the trace as the 32-bit address lookup fails in find_printk.
>>>
>>> Before:
>>> burn-1778  [003]   548.600305: bputs:   0xc0046db2s: 2cec5c058d98c
>>>
>>> After:
>>> burn-1778  [003]   548.600305: bputs:        0xc0046db2s: RT throttling activated
>>>
>>> The problem occurs in PRINT_FEILD when the field is recognized as a pointer
>>> to a string (of the type const char *)
>>
>> Actually, there's two bugs here. You only fixed one of them.
>>
>>>
>>> Cc: Steven Rostedt <rostedt@goodmis.org>
>>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>> Cc: Javi Merino <javi.merino@arm.com>
>>> Cc: David Ahern <dsahern@gmail.com>
>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>> Reported-by: Juri-Lelli <juri.lelli@arm.com>
>>> Signed-off-by: Kapileshwar Singh <kapileshwar.singh@arm.com>
>>> ---
>>>  tools/lib/traceevent/event-parse.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
>>> index 4d885934b919..39163ea4a048 100644
>>> --- a/tools/lib/traceevent/event-parse.c
>>> +++ b/tools/lib/traceevent/event-parse.c
>>> @@ -3829,6 +3829,17 @@ static void print_str_arg(struct trace_seq *s, void *data, int size,
>>>              if (!(field->flags & FIELD_IS_ARRAY) &&
>>>                  field->size == pevent->long_size) {
>>>                      addr = *(unsigned long *)(data + field->offset);
>>
>> addr is of type unsigned long. That means if we read a 64 bit record on
>> a 32 bit machine (which is supported), this will be truncated.
>>
>> Perhaps we need to make addr into a unsigned long long, and then add:
>>
>>       addr = (pevent->long_size == 8) ?
>>               *(unsigned long long *)(data + field->offset) :
>>               (unsigned long long )*(unsigned int *)(data + field->offset);

What about this? (untested)

    addr = *(uint64_t *)(data + field->offset) &
                        ((1ULL << pevent->long_size * 8) - 1);

Do we also need to consider byte endians?  Maybe it'd be better adding
a helper to dereference pointers then..

Thanks,
Namhyung

Re: [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces

[Kapileshwar Singh]

Hi Namhyung, 

Thanks for looking into this!

On 17/09/15 16:26, Namhyung Kim wrote:
> Hi,
> 
> On Thu, Sep 17, 2015 at 11:58 PM, Kapileshwar Singh
> <kapileshwar.singh@arm.com> wrote:
>> Hi Steve,
>>
>> Thanks for looking into this!
>>
>> On 17/09/15 14:11, Steven Rostedt wrote:
>>> On Thu, 17 Sep 2015 12:14:36 +0100
>>> Kapileshwar Singh <kapileshwar.singh@arm.com> wrote:
>>>
>>>> When a trace recorded on a 32-bit device is processed with a 64-bit
>>>> binary, the higher 32-bits of the address need to be masked.
>>>>
>>>> The lack of this results in the output of the 64-bit pointer
>>>> value to the trace as the 32-bit address lookup fails in find_printk.
>>>>
>>>> Before:
>>>> burn-1778  [003]   548.600305: bputs:   0xc0046db2s: 2cec5c058d98c
>>>>
>>>> After:
>>>> burn-1778  [003]   548.600305: bputs:        0xc0046db2s: RT throttling activated
>>>>
>>>> The problem occurs in PRINT_FEILD when the field is recognized as a pointer
>>>> to a string (of the type const char *)
>>>
>>> Actually, there's two bugs here. You only fixed one of them.
>>>
>>>>
>>>> Cc: Steven Rostedt <rostedt@goodmis.org>
>>>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>>> Cc: Javi Merino <javi.merino@arm.com>
>>>> Cc: David Ahern <dsahern@gmail.com>
>>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>>> Reported-by: Juri-Lelli <juri.lelli@arm.com>
>>>> Signed-off-by: Kapileshwar Singh <kapileshwar.singh@arm.com>
>>>> ---
>>>>  tools/lib/traceevent/event-parse.c | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
>>>> index 4d885934b919..39163ea4a048 100644
>>>> --- a/tools/lib/traceevent/event-parse.c
>>>> +++ b/tools/lib/traceevent/event-parse.c
>>>> @@ -3829,6 +3829,17 @@ static void print_str_arg(struct trace_seq *s, void *data, int size,
>>>>              if (!(field->flags & FIELD_IS_ARRAY) &&
>>>>                  field->size == pevent->long_size) {
>>>>                      addr = *(unsigned long *)(data + field->offset);
>>>
>>> addr is of type unsigned long. That means if we read a 64 bit record on
>>> a 32 bit machine (which is supported), this will be truncated.
>>>
>>> Perhaps we need to make addr into a unsigned long long, and then add:
>>>
>>>       addr = (pevent->long_size == 8) ?
>>>               *(unsigned long long *)(data + field->offset) :
>>>               (unsigned long long )*(unsigned int *)(data + field->offset);
> 
> What about this? (untested)
> 
>     addr = *(uint64_t *)(data + field->offset) &
>                         ((1ULL << pevent->long_size * 8) - 1);

I tested this and it works fine.

> 
> Do we also need to consider byte endians?  Maybe it'd be better adding
> a helper to dereference pointers then..

In this particular case, since the address is just a key for a lookup into the 
printk_map, which seems like a (addr -> const char *) mapping for string 
literals in the trace file, the endian-ness should not matter (I could be wrong though).

Regards, 
KP
> 
> Thanks,
> Namhyung
> 

Re: [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces

[Steven Rostedt]

On Fri, 18 Sep 2015 11:55:47 +0100
Kapileshwar Singh <kapileshwar.singh@arm.com> wrote:

> >>> Perhaps we need to make addr into a unsigned long long, and then add:
> >>>
> >>>       addr = (pevent->long_size == 8) ?
> >>>               *(unsigned long long *)(data + field->offset) :
> >>>               (unsigned long long )*(unsigned int *)(data + field->offset);
> > 
> > What about this? (untested)
> > 
> >     addr = *(uint64_t *)(data + field->offset) &
> >                         ((1ULL << pevent->long_size * 8) - 1);
> 
> I tested this and it works fine.

Except that I think it may be buggy.

> 
> > 
> > Do we also need to consider byte endians?  Maybe it'd be better adding
> > a helper to dereference pointers then..

Yes and no.

> 
> In this particular case, since the address is just a key for a lookup into the 
> printk_map, which seems like a (addr -> const char *) mapping for string 
> literals in the trace file, the endian-ness should not matter (I could be wrong though).

Correct, which is why I said "no", BUT! this is why I think Namhyung's
version may be buggy (besides the overflow of the buffer).

If this is a 64 bit big endian reading a 32 bit little endian file, I
think the result will be incorrect.

The *(uint64_t *) will return a 64bit number, but the address (with
long_size == 4) only needs 32bits. Thus, we are getting 32 more bits
than needed. Let's say the address is 0x12345678 that is loaded in the
file. Being little endian, it would be loaded as "78 56 34 12". Let's
say the 32bits after that is 0xDEADBEEF, loaded as "EF BE AD DE". Now
the number returned to addr (being a 64 bit big endian) would be:
0x785643412EFBEADDE  But then we do the shift:

 (1ULL << pevent->long_size * 8) - 1; which would leave us with:

0xEFBEADDE

Not what we wanted.

My version only reads the necessary bytes, and also wont suffer from
reading past the data size of the buffer (which is another bug).

-- Steve

Re: [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces

[Kapileshwar Singh]

Hi Steve,

On 18/09/15 14:45, Steven Rostedt wrote:
> On Fri, 18 Sep 2015 11:55:47 +0100
> Kapileshwar Singh <kapileshwar.singh@arm.com> wrote:
> 
>>>>> Perhaps we need to make addr into a unsigned long long, and then add:
>>>>>
>>>>>       addr = (pevent->long_size == 8) ?
>>>>>               *(unsigned long long *)(data + field->offset) :
>>>>>               (unsigned long long )*(unsigned int *)(data + field->offset);
>>>
>>> What about this? (untested)
>>>
>>>     addr = *(uint64_t *)(data + field->offset) &
>>>                         ((1ULL << pevent->long_size * 8) - 1);
>>
>> I tested this and it works fine.
> 
> Except that I think it may be buggy.
> 
>>
>>>
>>> Do we also need to consider byte endians?  Maybe it'd be better adding
>>> a helper to dereference pointers then..
> 
> Yes and no.
> 
>>
>> In this particular case, since the address is just a key for a lookup into the 
>> printk_map, which seems like a (addr -> const char *) mapping for string 
>> literals in the trace file, the endian-ness should not matter (I could be wrong though).
> 
> Correct, which is why I said "no", BUT! this is why I think Namhyung's
> version may be buggy (besides the overflow of the buffer).
> 
> If this is a 64 bit big endian reading a 32 bit little endian file, I
> think the result will be incorrect.
> 
> The *(uint64_t *) will return a 64bit number, but the address (with
> long_size == 4) only needs 32bits. Thus, we are getting 32 more bits
> than needed. Let's say the address is 0x12345678 that is loaded in the
> file. Being little endian, it would be loaded as "78 56 34 12". Let's
> say the 32bits after that is 0xDEADBEEF, loaded as "EF BE AD DE". Now
> the number returned to addr (being a 64 bit big endian) would be:
> 0x785643412EFBEADDE  But then we do the shift:
> 
>  (1ULL << pevent->long_size * 8) - 1; which would leave us with:
> 
> 0xEFBEADDE
> 
> Not what we wanted.

Agreed.

> 
> My version only reads the necessary bytes, and also wont suffer from
> reading past the data size of the buffer (which is another bug).
> 

Thanks for noticing and explaining this, makes perfect sense now!

Will submit a v3 for this.

Regards, 
KP

> -- Steve
> 
> 
>