[PATCH] perf: add sanity check on addr in symbol__inc_addr_samples()

[PATCH] perf: add sanity check on addr in symbol__inc_addr_samples()

[Stephane Eranian]

Check the value of addr against the bounds of the symbol.
This is needed given we compute an offset:
    	offset = addr - sym->start
    
And we don't want the offset to become negative.
    
Signed-off-by: Stephane Eranian <eranian@google.com>

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 011ed26..8248d80 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -64,7 +64,7 @@ int symbol__inc_addr_samples(struct symbol *sym, struct map *map,
 
 	pr_debug3("%s: addr=%#" PRIx64 "\n", __func__, map->unmap_ip(map, addr));
 
-	if (addr >= sym->end)
+	if (addr >= sym->end || addr < sym->start)
 		return 0;
 
 	offset = addr - sym->start;

Re: [PATCH] perf: add sanity check on addr in symbol__inc_addr_samples()

[Arnaldo Carvalho de Melo]

Em Thu, Feb 09, 2012 at 11:30:16AM +0100, Stephane Eranian escreveu:
> 
> Check the value of addr against the bounds of the symbol.
> This is needed given we compute an offset:
>     	offset = addr - sym->start
>     
> And we don't want the offset to become negative.

I'll try and add a debug option to show the backtrace and values of
addr, sym, etc, so that we can fix the real problem.

I.e. this function shouldn't be receiving any such invalid addresses, as
the symbol lookup was done, the symbol was found to be this one, then
why it would be out of bounds at this point?!

- Arnaldo
     
> Signed-off-by: Stephane Eranian <eranian@google.com>
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 011ed26..8248d80 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -64,7 +64,7 @@ int symbol__inc_addr_samples(struct symbol *sym, struct map *map,
>  
>  	pr_debug3("%s: addr=%#" PRIx64 "\n", __func__, map->unmap_ip(map, addr));
>  
> -	if (addr >= sym->end)
> +	if (addr >= sym->end || addr < sym->start)
>  		return 0;
>  
>  	offset = addr - sym->start;

Re: [PATCH] perf: add sanity check on addr in symbol__inc_addr_samples()

[Stephane Eranian]

On Thu, Feb 9, 2012 at 3:48 PM, Arnaldo Carvalho de Melo
<acme@redhat.com> wrote:
> Em Thu, Feb 09, 2012 at 11:30:16AM +0100, Stephane Eranian escreveu:
>>
>> Check the value of addr against the bounds of the symbol.
>> This is needed given we compute an offset:
>>       offset = addr - sym->start
>>
>> And we don't want the offset to become negative.
>
> I'll try and add a debug option to show the backtrace and values of
> addr, sym, etc, so that we can fix the real problem.
>
> I.e. this function shouldn't be receiving any such invalid addresses, as
> the symbol lookup was done, the symbol was found to be this one, then
> why it would be out of bounds at this point?!
>
I tend to agree with you on this. But then I don't see why the first test
was there.

> - Arnaldo
>
>> Signed-off-by: Stephane Eranian <eranian@google.com>
>>
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index 011ed26..8248d80 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -64,7 +64,7 @@ int symbol__inc_addr_samples(struct symbol *sym, struct map *map,
>>
>>       pr_debug3("%s: addr=%#" PRIx64 "\n", __func__, map->unmap_ip(map, addr));
>>
>> -     if (addr >= sym->end)
>> +     if (addr >= sym->end || addr < sym->start)
>>               return 0;
>>
>>       offset = addr - sym->start;

Re: [PATCH] perf: add sanity check on addr in symbol__inc_addr_samples()

[Sorin Dumitru]

On Thu, Feb 9, 2012 at 4:53 PM, Stephane Eranian <eranian@google.com> wrote:
> On Thu, Feb 9, 2012 at 3:48 PM, Arnaldo Carvalho de Melo
> <acme@redhat.com> wrote:
>> Em Thu, Feb 09, 2012 at 11:30:16AM +0100, Stephane Eranian escreveu:
>>>
>>> Check the value of addr against the bounds of the symbol.
>>> This is needed given we compute an offset:
>>>       offset = addr - sym->start
>>>
>>> And we don't want the offset to become negative.
>>
>> I'll try and add a debug option to show the backtrace and values of
>> addr, sym, etc, so that we can fix the real problem.
>>
>> I.e. this function shouldn't be receiving any such invalid addresses, as
>> the symbol lookup was done, the symbol was found to be this one, then
>> why it would be out of bounds at this point?!
>>
> I tend to agree with you on this. But then I don't see why the first test
> was there.
>
>> - Arnaldo
>>
>>> Signed-off-by: Stephane Eranian <eranian@google.com>
>>>
>>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>>> index 011ed26..8248d80 100644
>>> --- a/tools/perf/util/annotate.c
>>> +++ b/tools/perf/util/annotate.c
>>> @@ -64,7 +64,7 @@ int symbol__inc_addr_samples(struct symbol *sym, struct map *map,
>>>
>>>       pr_debug3("%s: addr=%#" PRIx64 "\n", __func__, map->unmap_ip(map, addr));
>>>
>>> -     if (addr >= sym->end)
>>> +     if (addr >= sym->end || addr < sym->start)
>>>               return 0;
>>>
>>>       offset = addr - sym->start;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

I reported the same problem a couple of weeks ago. From what i can
tell the problem
is in perf_event__process_sample. When calling
perf_event__process_sample, we set
al->sym based on al->address. The symbol in the hist_entry is set to
the one from al
but in the call to perf_top__record_precise_ip we pass in the address
from the event
struct which is sometimes different than the one in the al structure.
When this situation
occurs, when calculating the offset in symbol__inc_addr_samples,
because addr is not
in the symbol [start,end] range, we get a very big value which causes
the segfault when
we use it to index something. I've sent a patch that works for me, but
i don't know if it's
the right solution at [1].

[1] https://lkml.org/lkml/2012/1/29/59

Re: [PATCH] perf: add sanity check on addr in symbol__inc_addr_samples()

[Arnaldo Carvalho de Melo]

Em Thu, Feb 09, 2012 at 03:53:14PM +0100, Stephane Eranian escreveu:
> On Thu, Feb 9, 2012 at 3:48 PM, Arnaldo Carvalho de Melo
> <acme@redhat.com> wrote:
> > Em Thu, Feb 09, 2012 at 11:30:16AM +0100, Stephane Eranian escreveu:
> >>
> >> Check the value of addr against the bounds of the symbol.
> >> This is needed given we compute an offset:
> >>       offset = addr - sym->start
> >>
> >> And we don't want the offset to become negative.
> >
> > I'll try and add a debug option to show the backtrace and values of
> > addr, sym, etc, so that we can fix the real problem.

> > I.e. this function shouldn't be receiving any such invalid addresses, as
> > the symbol lookup was done, the symbol was found to be this one, then
> > why it would be out of bounds at this point?!

> I tend to agree with you on this. But then I don't see why the first test
> was there.

Its wrong as well, we should leave it there, together with the new test,
but as:

	BUG_ON(addr >= sym->end || addr < sym->start)

- Arnaldo
 
> > - Arnaldo
> >
> >> Signed-off-by: Stephane Eranian <eranian@google.com>
> >>
> >> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> >> index 011ed26..8248d80 100644
> >> --- a/tools/perf/util/annotate.c
> >> +++ b/tools/perf/util/annotate.c
> >> @@ -64,7 +64,7 @@ int symbol__inc_addr_samples(struct symbol *sym, struct map *map,
> >>
> >>       pr_debug3("%s: addr=%#" PRIx64 "\n", __func__, map->unmap_ip(map, addr));
> >>
> >> -     if (addr >= sym->end)
> >> +     if (addr >= sym->end || addr < sym->start)
> >>               return 0;
> >>
> >>       offset = addr - sym->start;

Re: [PATCH] perf: add sanity check on addr in symbol__inc_addr_samples()

[Stephane Eranian]

On Fri, Feb 10, 2012 at 2:46 PM, Arnaldo Carvalho de Melo
<acme@redhat.com> wrote:
> Em Thu, Feb 09, 2012 at 03:53:14PM +0100, Stephane Eranian escreveu:
>> On Thu, Feb 9, 2012 at 3:48 PM, Arnaldo Carvalho de Melo
>> <acme@redhat.com> wrote:
>> > Em Thu, Feb 09, 2012 at 11:30:16AM +0100, Stephane Eranian escreveu:
>> >>
>> >> Check the value of addr against the bounds of the symbol.
>> >> This is needed given we compute an offset:
>> >>       offset = addr - sym->start
>> >>
>> >> And we don't want the offset to become negative.
>> >
>> > I'll try and add a debug option to show the backtrace and values of
>> > addr, sym, etc, so that we can fix the real problem.
>
>> > I.e. this function shouldn't be receiving any such invalid addresses, as
>> > the symbol lookup was done, the symbol was found to be this one, then
>> > why it would be out of bounds at this point?!
>
>> I tend to agree with you on this. But then I don't see why the first test
>> was there.
>
> Its wrong as well, we should leave it there, together with the new test,
> but as:
>
>        BUG_ON(addr >= sym->end || addr < sym->start)
>
Fine with me. It makes more sense.

> - Arnaldo
>
>> > - Arnaldo
>> >
>> >> Signed-off-by: Stephane Eranian <eranian@google.com>
>> >>
>> >> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> >> index 011ed26..8248d80 100644
>> >> --- a/tools/perf/util/annotate.c
>> >> +++ b/tools/perf/util/annotate.c
>> >> @@ -64,7 +64,7 @@ int symbol__inc_addr_samples(struct symbol *sym, struct map *map,
>> >>
>> >>       pr_debug3("%s: addr=%#" PRIx64 "\n", __func__, map->unmap_ip(map, addr));
>> >>
>> >> -     if (addr >= sym->end)
>> >> +     if (addr >= sym->end || addr < sym->start)
>> >>               return 0;
>> >>
>> >>       offset = addr - sym->start;

Re: [PATCH] perf: add sanity check on addr in symbol__inc_addr_samples()

[Arnaldo Carvalho de Melo]

Em Thu, Feb 09, 2012 at 05:06:57PM +0200, Sorin Dumitru escreveu:
> On Thu, Feb 9, 2012 at 4:53 PM, Stephane Eranian <eranian@google.com> wrote:
> > On Thu, Feb 9, 2012 at 3:48 PM, Arnaldo Carvalho de Melo <acme@redhat.com> wrote:
> >> Em Thu, Feb 09, 2012 at 11:30:16AM +0100, Stephane Eranian escreveu:

> >>> Check the value of addr against the bounds of the symbol.
> >>> This is needed given we compute an offset:
> >>>       offset = addr - sym->start

> >>> And we don't want the offset to become negative.

> >> I'll try and add a debug option to show the backtrace and values of
> >> addr, sym, etc, so that we can fix the real problem.

> >> I.e. this function shouldn't be receiving any such invalid addresses, as
> >> the symbol lookup was done, the symbol was found to be this one, then
> >> why it would be out of bounds at this point?!

> > I tend to agree with you on this. But then I don't see why the first test
> > was there.
 
> I reported the same problem a couple of weeks ago. From what i can
> tell the problem is in perf_event__process_sample.

> When calling perf_event__process_sample, we set al->sym based on
> al->address.

> The symbol in the hist_entry is set to the one from al but in the call
> to perf_top__record_precise_ip we pass in the address from the event
> struct which is sometimes different than the one in the al structure.

> When this situation occurs, when calculating the offset in
> symbol__inc_addr_samples, because addr is not in the symbol
> [start,end] range, we get a very big value which causes the segfault
> when we use it to index something. I've sent a patch that works for
> me, but i don't know if it's the right solution at [1].
 
> [1] https://lkml.org/lkml/2012/1/29/59

Sorry for not having followed up on that one, now I'm trying to check if
it is valid but:

1. addr_location is set by perf_event__preprocess_sample that will call
   several routines that will figure out if the DSO symtab is loaded,
   load it if not, deal with prelinking, etc.

2. so at the end of this symbol resolution al.addr will point to the
   result of:

	al->map->map_ip(al->map, event->ip.ip)

3. in perf_top__record_precise_ip we'll use:

        he->ms.map->map_ip(he->ms.map, event->ip.ip)

And:

	he->ms.map == al->map

	So yeah, we can, as a worthwhile simplification, just pass
al.addr to perf_top__record_precise_ip as in your patch, but then we
need to remove the call to he->ms.map->map_ip.

	This extra call is harmless for identity mapped DSOs, as it
boils down to:

u64 identity__map_ip(struct map *map __used, u64 ip)
{               
        return ip;
} 

	But if the DSO uses:

u64 map__map_ip(struct map *map, u64 ip)
{                    
        return ip - map->start + map->pgoff;
} 

	Then we would be mapping the IP possibly to a negative one.

	So we need to revisit the details of your case and the one
Stephane (or Roberto) experienced, i.e. what kind of DSO? Prelinked?
etc.

- Arnaldo