* [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces @ 2015-09-17 11:14 Kapileshwar Singh 2015-09-17 13:11 ` Steven Rostedt 0 siblings, 1 reply; 7+ messages in thread From: Kapileshwar Singh @ 2015-09-17 11:14 UTC (permalink / raw) To: linux-kernel Cc: Kapileshwar Singh, Steven Rostedt, Arnaldo Carvalho de Melo, Namhyung Kim, Javi Merino, David Ahern, Jiri Olsa 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces 2015-09-17 11:14 [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces Kapileshwar Singh @ 2015-09-17 13:11 ` Steven Rostedt 2015-09-17 14:58 ` Kapileshwar Singh 0 siblings, 1 reply; 7+ messages in thread From: Steven Rostedt @ 2015-09-17 13:11 UTC (permalink / raw) To: Kapileshwar Singh Cc: linux-kernel, Arnaldo Carvalho de Melo, Namhyung Kim, Javi Merino, David Ahern, Jiri Olsa 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) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces 2015-09-17 13:11 ` Steven Rostedt @ 2015-09-17 14:58 ` Kapileshwar Singh 2015-09-17 15:26 ` Namhyung Kim 0 siblings, 1 reply; 7+ messages in thread From: Kapileshwar Singh @ 2015-09-17 14:58 UTC (permalink / raw) To: Steven Rostedt Cc: linux-kernel, Arnaldo Carvalho de Melo, Namhyung Kim, Javi Merino, David Ahern, Jiri Olsa 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) > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces 2015-09-17 14:58 ` Kapileshwar Singh @ 2015-09-17 15:26 ` Namhyung Kim 2015-09-18 10:55 ` Kapileshwar Singh 0 siblings, 1 reply; 7+ messages in thread From: Namhyung Kim @ 2015-09-17 15:26 UTC (permalink / raw) To: Kapileshwar Singh Cc: Steven Rostedt, linux-kernel, Arnaldo Carvalho de Melo, Javi Merino, David Ahern, Jiri Olsa 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces 2015-09-17 15:26 ` Namhyung Kim @ 2015-09-18 10:55 ` Kapileshwar Singh 2015-09-18 13:45 ` Steven Rostedt 0 siblings, 1 reply; 7+ messages in thread From: Kapileshwar Singh @ 2015-09-18 10:55 UTC (permalink / raw) To: Namhyung Kim Cc: Steven Rostedt, linux-kernel, Arnaldo Carvalho de Melo, Javi Merino, David Ahern, Jiri Olsa 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces 2015-09-18 10:55 ` Kapileshwar Singh @ 2015-09-18 13:45 ` Steven Rostedt 2015-09-18 14:29 ` Kapileshwar Singh 0 siblings, 1 reply; 7+ messages in thread From: Steven Rostedt @ 2015-09-18 13:45 UTC (permalink / raw) To: Kapileshwar Singh Cc: Namhyung Kim, linux-kernel, Arnaldo Carvalho de Melo, Javi Merino, David Ahern, Jiri Olsa 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces 2015-09-18 13:45 ` Steven Rostedt @ 2015-09-18 14:29 ` Kapileshwar Singh 0 siblings, 0 replies; 7+ messages in thread From: Kapileshwar Singh @ 2015-09-18 14:29 UTC (permalink / raw) To: Steven Rostedt Cc: Namhyung Kim, linux-kernel, Arnaldo Carvalho de Melo, Javi Merino, David Ahern, Jiri Olsa 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 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-09-18 14:29 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-09-17 11:14 [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces Kapileshwar Singh 2015-09-17 13:11 ` Steven Rostedt 2015-09-17 14:58 ` Kapileshwar Singh 2015-09-17 15:26 ` Namhyung Kim 2015-09-18 10:55 ` Kapileshwar Singh 2015-09-18 13:45 ` Steven Rostedt 2015-09-18 14:29 ` Kapileshwar Singh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).