Re: [PATCH] perf: kmaps: enforce usage of kmaps to protect futher bugs.

[Jiri Olsa <jolsa@redhat.com>]

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 <wangnan0@huawei.com>
> ---
>  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
>