Re: [PATCH v3 1/2] libbpf: kprobe.multi: cross filter using available_filter_functions and kallsyms

[Jackie Liu <liu.yun@linux.dev>]

在 2023/7/3 20:59, Jiri Olsa 写道:
> On Mon, Jul 03, 2023 at 09:36:17AM +0800, Jackie Liu wrote:
>> From: Jackie Liu <liuyun01@kylinos.cn>
>>
>> When using regular expression matching with "kprobe multi", it scans all
>> the functions under "/proc/kallsyms" that can be matched. However, not all
>> of them can be traced by kprobe.multi. If any one of the functions fails
>> to be traced, it will result in the failure of all functions. The best
>> approach is to filter out the functions that cannot be traced to ensure
>> proper tracking of the functions.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202307030355.TdXOHklM-lkp@intel.com/
>> Suggested-by: Jiri Olsa <jolsa@kernel.org>
>> Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>> ---
>>   v2->v3: fix 'fscanf' may overflow
>>
>>   tools/lib/bpf/libbpf.c | 122 ++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 109 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 214f828ece6b..232268215bb7 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -10224,6 +10224,12 @@ static const char *tracefs_uprobe_events(void)
>>   	return use_debugfs() ? DEBUGFS"/uprobe_events" : TRACEFS"/uprobe_events";
>>   }
>>   
>> +static const char *tracefs_available_filter_functions(void)
>> +{
>> +	return use_debugfs() ? DEBUGFS"/available_filter_functions" :
>> +			       TRACEFS"/available_filter_functions";
>> +}
>> +
>>   static void gen_kprobe_legacy_event_name(char *buf, size_t buf_sz,
>>   					 const char *kfunc_name, size_t offset)
>>   {
>> @@ -10539,23 +10545,113 @@ struct kprobe_multi_resolve {
>>   	size_t cnt;
>>   };
>>   
>> -static int
>> -resolve_kprobe_multi_cb(unsigned long long sym_addr, char sym_type,
>> -			const char *sym_name, void *ctx)
>> +static int qsort_compare_function(const void *a, const void *b)
>>   {
>> -	struct kprobe_multi_resolve *res = ctx;
>> -	int err;
>> +	return strcmp(*(const char **)a, *(const char **)b);
>> +}
>>   
>> -	if (!glob_match(sym_name, res->pattern))
>> -		return 0;
>> +static int bsearch_compare_function(const void *a, const void *b)
>> +{
>> +	return strcmp((const char *)a, *(const char **)b);
>> +}
>>   
>> -	err = libbpf_ensure_mem((void **) &res->addrs, &res->cap, sizeof(unsigned long),
>> -				res->cnt + 1);
>> -	if (err)
>> +static int libbpf_available_kallsyms_parse(struct kprobe_multi_resolve *res)
>> +{
>> +	char sym_name[500];
>> +	const char *available_functions_file = tracefs_available_filter_functions();
>> +	FILE *f;
>> +	int err = 0, ret, i;
>> +	struct function_info {
>> +		const char **syms;
>> +		size_t cap;
>> +		size_t cnt;
>> +	} infos = {};
> 
> do you need to define new struct for this? there's just on infos
> variable of that, you could use just:
> 
> 	const char **syms = NULL;
> 	size_t cap = 0, cnt = 0;
> 
>> +
>> +	f = fopen(available_functions_file, "r");
>> +	if (!f) {
>> +		err = -errno;
>> +		pr_warn("failed to open %s\n", available_functions_file);
>>   		return err;
>> +	}
>>   
>> -	res->addrs[res->cnt++] = (unsigned long) sym_addr;
>> -	return 0;
>> +	while (true) {
>> +		char *name;
>> +
>> +		ret = fscanf(f, "%499s%*[^\n]\n", sym_name);
>> +		if (ret == EOF && feof(f))
>> +			break;
>> +
>> +		if (ret != 1) {
>> +			pr_warn("failed to read available function file entry: %d\n",
>> +				ret);
>> +			err = -EINVAL;
>> +			goto cleanup;
>> +		}
>> +
>> +		if (!glob_match(sym_name, res->pattern))
>> +			continue;
>> +
>> +		err = libbpf_ensure_mem((void **)&infos.syms, &infos.cap,
>> +					sizeof(void *), infos.cnt + 1);
>> +		if (err)
>> +			goto cleanup;
>> +
>> +		name = strdup(sym_name);
>> +		if (!name) {
>> +			err = -errno;
>> +			goto cleanup;
>> +		}
>> +
>> +		infos.syms[infos.cnt++] = name;
>> +	}
>> +	fclose(f);
> 
> should you check if you found anything (infos.cnt != 0) and return early
> if there's nothing found
> 
>> +
>> +	/* sort available functions */
>> +	qsort(infos.syms, infos.cnt, sizeof(void *), qsort_compare_function);
>> +
>> +	f = fopen("/proc/kallsyms", "r");
> 
> why not use libbpf_kallsyms_parse for kallsyms parsing? the call below
> would be in its callback

This place cannot directly use libbpf_kallsyms_parse, because we need
info.syms, this value cannot be passed into the parameters of
libbpf_kallsyms_parse, and we cannot turn info.syms into a global
variable, which is unnecessary. The easiest way is to reimplement a A
copy of libbpf_kallsyms_parse.

Modifications to other parts will be carried along with the next
version.

-- 
Jackie

> 
>> +	if (!f) {
>> +		err = -errno;
>> +		pr_warn("failed to open /proc/kallsyms\n");
>> +		goto free_infos;
>> +	}
>> +
>> +	while (true) {
>> +		unsigned long long sym_addr;
>> +
>> +		ret = fscanf(f, "%llx %*c %499s%*[^\n]\n", &sym_addr, sym_name);
>> +		if (ret == EOF && feof(f))
>> +			break;
>> +
>> +		if (ret != 2) {
>> +			pr_warn("failed to read kallsyms entry: %d\n", ret);
>> +			err = -EINVAL;
>> +			break;
>> +		}
>> +
>> +		if (!glob_match(sym_name, res->pattern))
>> +			continue;
> 
> hm, we don't need to call glob_match again, we just want to check
> if the kallsyms symbol is in infos.syms
> 
>> +
>> +		if (!bsearch(&sym_name, infos.syms, infos.cnt, sizeof(void *),
>> +			     bsearch_compare_function))
>> +			continue;
>> +
>> +		err = libbpf_ensure_mem((void **)&res->addrs, &res->cap,
>> +					sizeof(unsigned long), res->cnt + 1);
>> +		if (err)
>> +			break;
>> +
>> +		res->addrs[res->cnt++] = (unsigned long) sym_addr;
>> +	}
> 
> res->cnt is check outside for 0, so we should be find here
> 
> jirka
> 
>> +
>> +cleanup:
>> +	fclose(f);
>> +free_infos:
>> +	for (i = 0; i < infos.cnt; i++)
>> +		free((char *)infos.syms[i]);
>> +	free(infos.syms);
>> +
>> +	return err;
>>   }
>>   
>>   struct bpf_link *
>> @@ -10594,7 +10690,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
>>   		return libbpf_err_ptr(-EINVAL);
>>   
>>   	if (pattern) {
>> -		err = libbpf_kallsyms_parse(resolve_kprobe_multi_cb, &res);
>> +		err = libbpf_available_kallsyms_parse(&res);
>>   		if (err)
>>   			goto error;
>>   		if (!res.cnt) {
>> -- 
>> 2.25.1
>>