Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752463AbbDCLL5 (ORCPT ); Fri, 3 Apr 2015 07:11:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38019 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751214AbbDCLLx (ORCPT ); Fri, 3 Apr 2015 07:11:53 -0400 Date: Fri, 3 Apr 2015 13:11:46 +0200 From: Jiri Olsa To: Wang Nan Cc: namhyung@kernel.org, jolsa@kernel.org, acme@redhat.com, mingo@kernel.org, linux-kernel@vger.kernel.org, pi3orama@163.com, Adrian Hunter Subject: Re: [PATCH] perf: kmaps: enforce usage of kmaps to protect futher bugs. Message-ID: <20150403111146.GC5571@krava.brq.redhat.com> References: <20150403064822.GB29383@gmail.com> <1428050877-54093-1-git-send-email-wangnan0@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1428050877-54093-1-git-send-email-wangnan0@huawei.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9380 Lines: 274 On Fri, Apr 03, 2015 at 08:47:57AM +0000, Wang Nan wrote: > This patch add checkings on every place where uses map__kmap and get > kmaps from struct kmap. Error messages are added at map__kmap to warn > invalud accessing of kmap (for the case of !map->dso->kernel, kmap(map) > is not exists at all). Also, introduces map__kmaps() to warn > uninitialized kmaps. looks ok, CC-ing Adrian thanks, jirka > > Signed-off-by: Wang Nan > --- > tools/perf/util/machine.c | 5 ++++- > tools/perf/util/map.h | 15 +++++++++++++++ > tools/perf/util/probe-event.c | 2 ++ > tools/perf/util/session.c | 3 +++ > tools/perf/util/symbol-elf.c | 16 +++++++++++----- > tools/perf/util/symbol.c | 34 ++++++++++++++++++++++++++++------ > 6 files changed, 63 insertions(+), 12 deletions(-) > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c > index e335330..9c7feec 100644 > --- a/tools/perf/util/machine.c > +++ b/tools/perf/util/machine.c > @@ -679,6 +679,9 @@ int __machine__create_kernel_maps(struct machine *machine, struct dso *kernel) > machine->vmlinux_maps[type]->unmap_ip = > identity__map_ip; > kmap = map__kmap(machine->vmlinux_maps[type]); > + if (!kmap) > + return -1; > + > kmap->kmaps = &machine->kmaps; > map_groups__insert(&machine->kmaps, > machine->vmlinux_maps[type]); > @@ -700,7 +703,7 @@ void machine__destroy_kernel_maps(struct machine *machine) > kmap = map__kmap(machine->vmlinux_maps[type]); > map_groups__remove(&machine->kmaps, > machine->vmlinux_maps[type]); > - if (kmap->ref_reloc_sym) { > + if (kmap && kmap->ref_reloc_sym) { > /* > * ref_reloc_sym is shared among all maps, so free just > * on one of them. > diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h > index 0e42438..2ba83c8 100644 > --- a/tools/perf/util/map.h > +++ b/tools/perf/util/map.h > @@ -78,9 +78,24 @@ void map_groups__put(struct map_groups *mg); > > static inline struct kmap *map__kmap(struct map *map) > { > + if (!map->dso || !map->dso->kernel) { > + pr_err("Internal error: map__kmap with a non-kernel map\n"); > + return NULL; > + } > return (struct kmap *)(map + 1); > } > > +static inline struct map_groups *kmaps map__kmaps(struct map *map) > +{ > + struct kmap *kmap = map__kmap(map); > + > + if (!kmap || !kmap->kmaps) { > + pr_err("Internal error: map__kmaps with a non-kernel map\n"); > + return NULL; > + } > + return kmap->kmaps; > +} > + > static inline u64 map__map_ip(struct map *map, u64 ip) > { > return ip - map->start + map->pgoff; > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c > index 8feac07..4fd49f0 100644 > --- a/tools/perf/util/probe-event.c > +++ b/tools/perf/util/probe-event.c > @@ -135,6 +135,8 @@ static struct ref_reloc_sym *kernel_get_ref_reloc_sym(void) > return NULL; > > kmap = map__kmap(host_machine->vmlinux_maps[MAP__FUNCTION]); > + if (!kmap) > + return NULL; > return kmap->ref_reloc_sym; > } > > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c > index adf0740..7caedb4 100644 > --- a/tools/perf/util/session.c > +++ b/tools/perf/util/session.c > @@ -1456,6 +1456,9 @@ int maps__set_kallsyms_ref_reloc_sym(struct map **maps, > > for (i = 0; i < MAP__NR_TYPES; ++i) { > struct kmap *kmap = map__kmap(maps[i]); > + > + if (!kmap) > + continue; > kmap->ref_reloc_sym = ref; > } > > diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c > index 476268c..a7ab606 100644 > --- a/tools/perf/util/symbol-elf.c > +++ b/tools/perf/util/symbol-elf.c > @@ -776,6 +776,7 @@ int dso__load_sym(struct dso *dso, struct map *map, > symbol_filter_t filter, int kmodule) > { > struct kmap *kmap = dso->kernel ? map__kmap(map) : NULL; > + struct map_groups *kmaps = kmap ? map__kmaps(map) : NULL; > struct map *curr_map = map; > struct dso *curr_dso = dso; > Elf_Data *symstrs, *secstrs; > @@ -791,6 +792,9 @@ int dso__load_sym(struct dso *dso, struct map *map, > int nr = 0; > bool remap_kernel = false, adjust_kernel_syms = false; > > + if (kmap && !kmaps) > + return -1; > + > dso->symtab_type = syms_ss->type; > dso->is_64_bit = syms_ss->is_64_bit; > dso->rel = syms_ss->ehdr.e_type == ET_REL; > @@ -958,8 +962,10 @@ int dso__load_sym(struct dso *dso, struct map *map, > map->map_ip = map__map_ip; > map->unmap_ip = map__unmap_ip; > /* Ensure maps are correctly ordered */ > - map_groups__remove(kmap->kmaps, map); > - map_groups__insert(kmap->kmaps, map); > + if (kmaps) { > + map_groups__remove(kmaps, map); > + map_groups__insert(kmaps, map); > + } > } > > /* > @@ -983,7 +989,7 @@ int dso__load_sym(struct dso *dso, struct map *map, > snprintf(dso_name, sizeof(dso_name), > "%s%s", dso->short_name, section_name); > > - curr_map = map_groups__find_by_name(kmap->kmaps, map->type, dso_name); > + curr_map = map_groups__find_by_name(kmaps, map->type, dso_name); > if (curr_map == NULL) { > u64 start = sym.st_value; > > @@ -1013,7 +1019,7 @@ int dso__load_sym(struct dso *dso, struct map *map, > curr_map->unmap_ip = identity__map_ip; > } > curr_dso->symtab_type = dso->symtab_type; > - map_groups__insert(kmap->kmaps, curr_map); > + map_groups__insert(kmaps, curr_map); > /* > * The new DSO should go to the kernel DSOS > */ > @@ -1075,7 +1081,7 @@ new_symbol: > * We need to fixup this here too because we create new > * maps here, for things like vsyscall sections. > */ > - __map_groups__fixup_end(kmap->kmaps, map->type); > + __map_groups__fixup_end(kmaps, map->type); > } > } > err = nr; > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c > index fddeb90..201f6c4c 100644 > --- a/tools/perf/util/symbol.c > +++ b/tools/perf/util/symbol.c > @@ -630,13 +630,16 @@ static int dso__load_all_kallsyms(struct dso *dso, const char *filename, > static int dso__split_kallsyms_for_kcore(struct dso *dso, struct map *map, > symbol_filter_t filter) > { > - struct map_groups *kmaps = map__kmap(map)->kmaps; > + struct map_groups *kmaps = map__kmaps(map); > struct map *curr_map; > struct symbol *pos; > int count = 0, moved = 0; > struct rb_root *root = &dso->symbols[map->type]; > struct rb_node *next = rb_first(root); > > + if (!kmaps) > + return -1; > + > while (next) { > char *module; > > @@ -682,8 +685,8 @@ static int dso__split_kallsyms_for_kcore(struct dso *dso, struct map *map, > static int dso__split_kallsyms(struct dso *dso, struct map *map, u64 delta, > symbol_filter_t filter) > { > - struct map_groups *kmaps = map__kmap(map)->kmaps; > - struct machine *machine = kmaps->machine; > + struct map_groups *kmaps = map__kmaps(map); > + struct machine *machine; > struct map *curr_map = map; > struct symbol *pos; > int count = 0, moved = 0; > @@ -691,6 +694,11 @@ static int dso__split_kallsyms(struct dso *dso, struct map *map, u64 delta, > struct rb_node *next = rb_first(root); > int kernel_range = 0; > > + if (!kmaps) > + return -1; > + > + machine = kmaps->machine; > + > while (next) { > char *module; > > @@ -1025,9 +1033,12 @@ static bool filename_from_kallsyms_filename(char *filename, > static int validate_kcore_modules(const char *kallsyms_filename, > struct map *map) > { > - struct map_groups *kmaps = map__kmap(map)->kmaps; > + struct map_groups *kmaps = map__kmaps(map); > char modules_filename[PATH_MAX]; > > + if (!kmaps) > + return -EINVAL; > + > if (!filename_from_kallsyms_filename(modules_filename, "modules", > kallsyms_filename)) > return -EINVAL; > @@ -1043,6 +1054,9 @@ static int validate_kcore_addresses(const char *kallsyms_filename, > { > struct kmap *kmap = map__kmap(map); > > + if (!kmap) > + return -EINVAL; > + > if (kmap->ref_reloc_sym && kmap->ref_reloc_sym->name) { > u64 start; > > @@ -1081,8 +1095,8 @@ static int kcore_mapfn(u64 start, u64 len, u64 pgoff, void *data) > static int dso__load_kcore(struct dso *dso, struct map *map, > const char *kallsyms_filename) > { > - struct map_groups *kmaps = map__kmap(map)->kmaps; > - struct machine *machine = kmaps->machine; > + struct map_groups *kmaps = map__kmaps(map); > + struct machine *machine; > struct kcore_mapfn_data md; > struct map *old_map, *new_map, *replacement_map = NULL; > bool is_64_bit; > @@ -1090,6 +1104,11 @@ static int dso__load_kcore(struct dso *dso, struct map *map, > char kcore_filename[PATH_MAX]; > struct symbol *sym; > > + if (!kmaps) > + return -EINVAL; > + > + machine = kmaps->machine; > + > /* This function requires that the map is the kernel map */ > if (map != machine->vmlinux_maps[map->type]) > return -EINVAL; > @@ -1202,6 +1221,9 @@ static int kallsyms__delta(struct map *map, const char *filename, u64 *delta) > struct kmap *kmap = map__kmap(map); > u64 addr; > > + if (!kmap) > + return -1; > + > if (!kmap->ref_reloc_sym || !kmap->ref_reloc_sym->name) > return 0; > > -- > 1.8.3.4 > -- 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/