perf: add sanity check on addr in symbol__inc_addr_samples()
PATCH 1/1 perf: add sanity check on addr in symbol__inc_addr_samples()
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.
tools/perf/util/annotate.c +1 -1
@@ -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;
Em Thu, Feb 09, 2012 at 11:30:16AM +0100, Stephane Eranian escreveu:
Quoted 6 lines
Check the value of addr against the bounds of the symbol. This is needed given we compute an offset:
offset = addr - sym->startAnd 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
Quoted 15 lines
Signed-off-by Stephane Eranian <[email protected]>tools/perf/util/annotate.c +1
-1@@ -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;
Quoted 6 lines · Arnaldo Carvalho de Melo
Em Thu, Feb 09, 2012 at 11:30:16AM +0100, Stephane Eranian escreveu:
Quoted 5 lines
Check the value of addr against the bounds of the symbol. This is needed given we compute an offset:
offset = addr -sym->start
Quoted 9 lines
Quoted 1 line
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.
Quoted 17 lines
- Arnaldo
Quoted 15 lines
Signed-off-by Stephane Eranian <[email protected]>tools/perf/util/annotate.c +1
-1@@ -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;
Quoted 36 lines · Stephane Eranian
Quoted 6 lines · Arnaldo Carvalho de Melo
Em Thu, Feb 09, 2012 at 11:30:16AM +0100, Stephane Eranian escreveu:
Quoted 5 lines
Check the value of addr against the bounds of the symbol. This is needed given we compute an offset:
offset = addr -sym->startQuoted 9 lines
Quoted 1 line
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.
Quoted 16 lines
- Arnaldo
Quoted 14 lines
Signed-off-by Stephane Eranian <[email protected]>tools/perf/util/annotate.c +1
-1@@ -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;
Quoted 6 lines
Quoted 1 line
tools/perf/util/annotate.c int symbol__inc_addr_samples(struct symbol *sym, struct map *map, · line 70
Quoted 1 line
tools/perf/util/annotate.c +0
-0offset = addr - sym->start;--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
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 setal->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].
Em Thu, Feb 09, 2012 at 03:53:14PM +0100, Stephane Eranian escreveu:
Quoted 13 lines
Quoted 10 lines · Arnaldo Carvalho de Melo
Em Thu, Feb 09, 2012 at 11:30:16AM +0100, Stephane Eranian escreveu:
Quoted 6 lines
Check the value of addr against the bounds of the symbol. This is needed given we compute an offset:
offset = addr -sym->startAnd 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.
Quoted 6 lines
Quoted 3 lines
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
Quoted 17 lines
Quoted 17 lines
- Arnaldo
Quoted 15 lines
Signed-off-by Stephane Eranian <[email protected]>tools/perf/util/annotate.c +1
-1@@ -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;
Quoted 18 lines · Arnaldo Carvalho de Melo
Em Thu, Feb 09, 2012 at 03:53:14PM +0100, Stephane Eranian escreveu:
Quoted 16 lines
Quoted 14 lines · Arnaldo Carvalho de Melo
Em Thu, Feb 09, 2012 at 11:30:16AM +0100, Stephane Eranian escreveu:
Quoted 6 lines
Check the value of addr against the bounds of the symbol. This is needed given we compute an offset:
offset = addr -sym->startAnd 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?!
Quoted 8 lines
Quoted 2 lines
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.
Quoted 19 lines
- Arnaldo
Quoted 17 lines
Quoted 17 lines
- Arnaldo
Quoted 15 lines
Signed-off-by Stephane Eranian <[email protected]>tools/perf/util/annotate.c +1
-1@@ -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;
Em Thu, Feb 09, 2012 at 05:06:57PM +0200, Sorin Dumitru escreveu:
Quoted 30 lines · Stephane Eranian
Quoted 12 lines · Arnaldo Carvalho de Melo
Quoted 10 lines
Em Thu, Feb 09, 2012 at 11:30:16AM +0100, Stephane Eranian escreveu:
Quoted 5 lines
Check the value of addr against the bounds of the symbol. This is needed given we compute an offset:
offset = addr -sym->startAnd 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.
Quoted 6 lines
Quoted 3 lines
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 setal->symbased onal->address.The symbol in the
hist_entryis set to the one from al but in the call toperf_top__record_precise_ipwe pass in the address from the event struct which is sometimes different than the one in the al structure.
Quoted 7 lines
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].
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