Re: [RFC] perf probe: Fix offline module name missmatch issue

[Masami Hiramatsu]

Hi Ravi,

On Mon, 25 Apr 2016 16:08:27 +0530
Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxxxxxxx> wrote:

> Perf can add a probe on kernel module which has not been loaded yet.
> Current implementation finds module name from path. But if filename
> is different from actual module name then perf fails to register
> probe while loading module because of mismatch in names. For example,
> samples/kobject/kobject-example.ko is loaded as kobject_example.

Ah! right, good catch!
Have some comment below;

>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 8319fbb..05d0905 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -265,6 +265,65 @@ static bool kprobe_warn_out_range(const char *symbol, unsigned long address)
>  	return true;
>  }
>  
> +/*
> + * NOTE:
> + * '.gnu.linkonce.this_module' section of kernel module elf directly
> + * maps to 'struct module' from linux/module.h. This section contains
> + * actual module name which will be used by kernel after loading it.
> + * But, we cannot use 'struct module' here since linux/module.h is not
> + * exposed to user-space. Offset of 'name' has remained same from long
> + * time, so hardcoding it here.
> + */
> +#ifdef __LP64__
> +#define MOD_NAME_OFFSET 24
> +#else
> +#define MOD_NAME_OFFSET 12
> +#endif
> +
> +/*
> + * @module can be module name of module file path. In case of path,
> + * inspect elf and find out what is actual module name.
> + * Caller has to free mod_name after using it.
> + */
> +char *find_module_name(const char *module)

Could you make this function static, since there is no caller outside
this file?

> +{
> +	int fd;
> +	Elf *elf;
> +	GElf_Ehdr ehdr;
> +	GElf_Shdr shdr;
> +	Elf_Data *data;
> +	Elf_Scn *sec;
> +	char *mod_name = NULL;
> +
> +	fd = open(module, O_RDONLY);
> +	if (fd < 0)
> +		return NULL;
> +
> +	elf = elf_begin(fd, PERF_ELF_C_READ_MMAP, NULL);
> +	if (elf == NULL)
> +		goto elf_err;
> +
> +	if (gelf_getehdr(elf, &ehdr) == NULL)
> +		goto ret_err;
> +
> +	sec = elf_section_by_name(elf, &ehdr, &shdr,
> +			".gnu.linkonce.this_module", NULL);
> +	if (!sec)
> +		goto ret_err;
> +
> +	data = elf_getdata(sec, NULL);
> +	if (!data || !data->d_buf)
> +		goto ret_err;
> +
> +	mod_name = strdup((char *)data->d_buf + MOD_NAME_OFFSET);
> +
> +ret_err:
> +	elf_end(elf);
> +elf_err:
> +	close(fd);
> +	return mod_name;
> +}
> +
>  #ifdef HAVE_DWARF_SUPPORT
>  
>  static int kernel_get_module_dso(const char *module, struct dso **pdso)
> @@ -583,32 +642,23 @@ static int add_module_to_probe_trace_events(struct probe_trace_event *tevs,
>  					    int ntevs, const char *module)
>  {
>  	int i, ret = 0;
> -	char *tmp;
> +	char *mod_name;
>  
>  	if (!module)
>  		return 0;
>  
> -	tmp = strrchr(module, '/');
> -	if (tmp) {
> -		/* This is a module path -- get the module name */
> -		module = strdup(tmp + 1);
> -		if (!module)
> -			return -ENOMEM;
> -		tmp = strchr(module, '.');
> -		if (tmp)
> -			*tmp = '\0';
> -		tmp = (char *)module;	/* For free() */
> -	}
> +	mod_name = find_module_name(module);
>  
>  	for (i = 0; i < ntevs; i++) {
> -		tevs[i].point.module = strdup(module);
> +		tevs[i].point.module =
> +			strdup(mod_name ? mod_name : module);
>  		if (!tevs[i].point.module) {
>  			ret = -ENOMEM;
>  			break;
>  		}
>  	}
>  
> -	free(tmp);
> +	free(mod_name);
>  	return ret;
>  }
>  
> diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
> index e54e7b0..0468fa3 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -166,4 +166,6 @@ int e_snprintf(char *str, size_t size, const char *format, ...)
>  int copy_to_probe_trace_arg(struct probe_trace_arg *tvar,
>  			    struct perf_probe_arg *pvar);
>  
> +char *find_module_name(const char *module);

And remove this.

Others looks good to me!

Thank you,

> +
>  #endif /*_PROBE_EVENT_H */
> -- 
> 2.1.4
> 


-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>