Re: [PATCH perf/core v2 2/3] [BUGFIX] perf probe: Check non-probe-able symbols when using symbol map

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

On 2015/06/16 3:57, Arnaldo Carvalho de Melo wrote:
> Em Sat, Jun 13, 2015 at 10:31:18AM +0900, Masami Hiramatsu escreveu:
>> Fix to check both of non-exist symbols and kprobe blacklist symbols at
>> symbol-map based converting too.
>>
>> E.g. without this fix, perf-probe failes to write the event.
>>   ----
>>   # perf probe vfs_caches_init_early
>>   Added new event:
>>   Failed to write event: Invalid argument
>>     Error: Failed to add events.
>>   ----
> 
> Well, here, after I applied 1/3 in this series, before this patch, I
> get:
> 
> 
> [root@zoo ~]# grep vfs_caches_init_early /proc/kallsyms 
> ffffffff81d8e60f T vfs_caches_init_early
> [root@zoo ~]# perf probe vfs_caches_init_early
> vfs_caches_init_early+0 is out of .text, skip it.
> Probe point 'vfs_caches_init_early' not found.
>   Error: Failed to add events.
> [root@zoo ~]#
> 
> What am I doind wrong? I am getting the message you said I should get after
> applying your patch, before I apply it... /me confused :-\

Hmm, right, I also checked you right. Maybe I misused bad binary
(of kernel?)... sorry about that.

Anyway, this patch is also fixing another bug to return error if
the symbol is blacklisted. I'll fix the comment and sort the bugs
out.

Thank you,

> 
> First patch applied, waiting for you on this and the next.
> 
> - Arnaldo
> 
>>
>> This fixes it to catch the error before adding probes.
>>   ----
>>   # perf probe vfs_caches_init_early
>>   vfs_caches_init_early is out of .text, skip it.
>>     Error: Failed to add events.
>>   ----
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> ---
>>  tools/perf/util/probe-event.c |  113 ++++++++++++++++++++++++++---------------
>>  1 file changed, 71 insertions(+), 42 deletions(-)
>>
>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> index c4ab588..85c8207 100644
>> --- a/tools/perf/util/probe-event.c
>> +++ b/tools/perf/util/probe-event.c
>> @@ -246,6 +246,20 @@ static void clear_probe_trace_events(struct probe_trace_event *tevs, int ntevs)
>>  		clear_probe_trace_event(tevs + i);
>>  }
>>  
>> +static bool kprobe_blacklist__listed(unsigned long address);
>> +static bool kprobe_warn_out_range(const char *symbol, unsigned long address)
>> +{
>> +	/* Get the address of _etext for checking non-probable text symbol */
>> +	if (kernel_get_symbol_address_by_name("_etext", false) < address)
>> +		pr_warning("%s is out of .text, skip it.\n", symbol);
>> +	else if (kprobe_blacklist__listed(address))
>> +		pr_warning("%s is blacklisted function, skip it.\n", symbol);
>> +	else
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>>  #ifdef HAVE_DWARF_SUPPORT
>>  
>>  static int kernel_get_module_dso(const char *module, struct dso **pdso)
>> @@ -559,7 +573,6 @@ static int post_process_probe_trace_events(struct probe_trace_event *tevs,
>>  					   bool uprobe)
>>  {
>>  	struct ref_reloc_sym *reloc_sym;
>> -	u64 etext_addr;
>>  	char *tmp;
>>  	int i, skipped = 0;
>>  
>> @@ -575,31 +588,28 @@ static int post_process_probe_trace_events(struct probe_trace_event *tevs,
>>  		pr_warning("Relocated base symbol is not found!\n");
>>  		return -EINVAL;
>>  	}
>> -	/* Get the address of _etext for checking non-probable text symbol */
>> -	etext_addr = kernel_get_symbol_address_by_name("_etext", false);
>>  
>>  	for (i = 0; i < ntevs; i++) {
>> -		if (tevs[i].point.address && !tevs[i].point.retprobe) {
>> -			/* If we found a wrong one, mark it by NULL symbol */
>> -			if (etext_addr < tevs[i].point.address) {
>> -				pr_warning("%s+%lu is out of .text, skip it.\n",
>> -				   tevs[i].point.symbol, tevs[i].point.offset);
>> -				tmp = NULL;
>> -				skipped++;
>> -			} else {
>> -				tmp = strdup(reloc_sym->name);
>> -				if (!tmp)
>> -					return -ENOMEM;
>> -			}
>> -			/* If we have no realname, use symbol for it */
>> -			if (!tevs[i].point.realname)
>> -				tevs[i].point.realname = tevs[i].point.symbol;
>> -			else
>> -				free(tevs[i].point.symbol);
>> -			tevs[i].point.symbol = tmp;
>> -			tevs[i].point.offset = tevs[i].point.address -
>> -					       reloc_sym->unrelocated_addr;
>> +		if (!tevs[i].point.address || tevs[i].point.retprobe)
>> +			continue;
>> +		/* If we found a wrong one, mark it by NULL symbol */
>> +		if (kprobe_warn_out_range(tevs[i].point.symbol,
>> +					  tevs[i].point.address)) {
>> +			tmp = NULL;
>> +			skipped++;
>> +		} else {
>> +			tmp = strdup(reloc_sym->name);
>> +			if (!tmp)
>> +				return -ENOMEM;
>>  		}
>> +		/* If we have no realname, use symbol for it */
>> +		if (!tevs[i].point.realname)
>> +			tevs[i].point.realname = tevs[i].point.symbol;
>> +		else
>> +			free(tevs[i].point.symbol);
>> +		tevs[i].point.symbol = tmp;
>> +		tevs[i].point.offset = tevs[i].point.address -
>> +				       reloc_sym->unrelocated_addr;
>>  	}
>>  	return skipped;
>>  }
>> @@ -2126,6 +2136,27 @@ kprobe_blacklist__find_by_address(struct list_head *blacklist,
>>  	return NULL;
>>  }
>>  
>> +static LIST_HEAD(kprobe_blacklist);
>> +
>> +static void kprobe_blacklist__init(void)
>> +{
>> +	if (!list_empty(&kprobe_blacklist))
>> +		return;
>> +
>> +	if (kprobe_blacklist__load(&kprobe_blacklist) < 0)
>> +		pr_debug("No kprobe blacklist support, ignored\n");
>> +}
>> +
>> +static void kprobe_blacklist__release(void)
>> +{
>> +	kprobe_blacklist__delete(&kprobe_blacklist);
>> +}
>> +
>> +static bool kprobe_blacklist__listed(unsigned long address)
>> +{
>> +	return !!kprobe_blacklist__find_by_address(&kprobe_blacklist, address);
>> +}
>> +
>>  static int perf_probe_event__sprintf(struct perf_probe_event *pev,
>>  				     const char *module,
>>  				     struct strbuf *result)
>> @@ -2409,8 +2440,6 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
>>  	char buf[64];
>>  	const char *event, *group;
>>  	struct strlist *namelist;
>> -	LIST_HEAD(blacklist);
>> -	struct kprobe_blacklist_node *node;
>>  	bool safename;
>>  
>>  	if (pev->uprobes)
>> @@ -2430,28 +2459,15 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
>>  		ret = -ENOMEM;
>>  		goto close_out;
>>  	}
>> -	/* Get kprobe blacklist if exists */
>> -	if (!pev->uprobes) {
>> -		ret = kprobe_blacklist__load(&blacklist);
>> -		if (ret < 0)
>> -			pr_debug("No kprobe blacklist support, ignored\n");
>> -	}
>>  
>>  	safename = (pev->point.function && !strisglob(pev->point.function));
>>  	ret = 0;
>>  	pr_info("Added new event%s\n", (ntevs > 1) ? "s:" : ":");
>>  	for (i = 0; i < ntevs; i++) {
>>  		tev = &tevs[i];
>> -		/* Skip if the symbol is out of .text (marked previously) */
>> +		/* Skip if the symbol is out of .text or blacklisted */
>>  		if (!tev->point.symbol)
>>  			continue;
>> -		/* Ensure that the address is NOT blacklisted */
>> -		node = kprobe_blacklist__find_by_address(&blacklist,
>> -							 tev->point.address);
>> -		if (node) {
>> -			pr_warning("Warning: Skipped probing on blacklisted function: %s\n", node->symbol);
>> -			continue;
>> -		}
>>  
>>  		if (pev->event)
>>  			event = pev->event;
>> @@ -2513,7 +2529,6 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
>>  			 tev->event);
>>  	}
>>  
>> -	kprobe_blacklist__delete(&blacklist);
>>  	strlist__delete(namelist);
>>  close_out:
>>  	close(fd);
>> @@ -2563,7 +2578,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>>  	struct perf_probe_point *pp = &pev->point;
>>  	struct probe_trace_point *tp;
>>  	int num_matched_functions;
>> -	int ret, i, j;
>> +	int ret, i, j, skipped = 0;
>>  
>>  	map = get_target_map(pev->target, pev->uprobes);
>>  	if (!map) {
>> @@ -2631,7 +2646,12 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>>  		}
>>  		/* Add one probe point */
>>  		tp->address = map->unmap_ip(map, sym->start) + pp->offset;
>> -		if (reloc_sym) {
>> +		/* If we found a wrong one, mark it by NULL symbol */
>> +		if (!pev->uprobes &&
>> +		    kprobe_warn_out_range(sym->name, tp->address)) {
>> +			tp->symbol = NULL;	/* Skip it */
>> +			skipped++;
>> +		} else if (reloc_sym) {
>>  			tp->symbol = strdup_or_goto(reloc_sym->name, nomem_out);
>>  			tp->offset = tp->address - reloc_sym->addr;
>>  		} else {
>> @@ -2667,6 +2687,10 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>>  		}
>>  		arch__fix_tev_from_maps(pev, tev, map);
>>  	}
>> +	if (ret == skipped) {
>> +		ret = -ENOENT;
>> +		goto err_out;
>> +	}
>>  
>>  out:
>>  	put_target_map(map, pev->uprobes);
>> @@ -2737,6 +2761,9 @@ int add_perf_probe_events(struct perf_probe_event *pevs, int npevs)
>>  	/* Loop 1: convert all events */
>>  	for (i = 0; i < npevs; i++) {
>>  		pkgs[i].pev = &pevs[i];
>> +		/* Init kprobe blacklist if needed */
>> +		if (!pkgs[i].pev->uprobes)
>> +			kprobe_blacklist__init();
>>  		/* Convert with or without debuginfo */
>>  		ret  = convert_to_probe_trace_events(pkgs[i].pev,
>>  						     &pkgs[i].tevs);
>> @@ -2744,6 +2771,8 @@ int add_perf_probe_events(struct perf_probe_event *pevs, int npevs)
>>  			goto end;
>>  		pkgs[i].ntevs = ret;
>>  	}
>> +	/* This just release blacklist only if allocated */
>> +	kprobe_blacklist__release();
>>  
>>  	/* Loop 2: add all events */
>>  	for (i = 0; i < npevs; i++) {
> 


-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com