diff mbox series

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

Message ID 20230703013618.1959621-1-liu.yun@linux.dev (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [v3,1/2] libbpf: kprobe.multi: cross filter using available_filter_functions and kallsyms | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-6 success Logs for set-matrix
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps on s390x with gcc

Commit Message

Jackie Liu July 3, 2023, 1:36 a.m. UTC 
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(-)

Comments

Jiri Olsa July 3, 2023, 12:59 p.m. UTC | #1 
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

> +	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
>
John Fastabend July 3, 2023, 7:38 p.m. UTC | #2 
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 = {};
> +
> +	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;
> +

Looks like you fixed up the fclose() issues, sorry about the noise
reading email backwards.


bit of a nit...

Its probably worth handling the case where ret == EOF and its
not feof(f) that man page claims can happen on read error for
example. Might never happen but would be good to distinguish from
-EINVAL below?

> +		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);
> +
> +	/* sort available functions */
> +	qsort(infos.syms, infos.cnt, sizeof(void *), qsort_compare_function);
> +
> +	f = fopen("/proc/kallsyms", "r");
> +	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;

Same off chance we get ret == EOF and !feof(f)?

> +
> +		if (ret != 2) {
> +			pr_warn("failed to read kallsyms entry: %d\n", ret);
> +			err = -EINVAL;
> +			break;
> +		}
> +
> +		if (!glob_match(sym_name, res->pattern))
> +			continue;
> +
> +		if (!bsearch(&sym_name, infos.syms, infos.cnt, sizeof(void *),
> +			     bsearch_compare_function))
> +			continue;

I'm wondering if we could get a debug print if the func was skipped? Its
not always clear when running many kernels what is going to be skipped
and where.

> +
> +		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;
> +	}
> +
> +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
> 
>
Jackie Liu July 4, 2023, 1:30 a.m. UTC | #3 
在 2023/7/4 03:38, John Fastabend 写道:
> 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 = {};
>> +
>> +	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;
>> +
> 
> Looks like you fixed up the fclose() issues, sorry about the noise
> reading email backwards.
> 
> 
> bit of a nit...
> 
> Its probably worth handling the case where ret == EOF and its
> not feof(f) that man page claims can happen on read error for
> example. Might never happen but would be good to distinguish from
> -EINVAL below?

I think it should not be necessary, we only care about whether we have
read the correct data or not, other cases are read failures.

> 
>> +		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);
>> +
>> +	/* sort available functions */
>> +	qsort(infos.syms, infos.cnt, sizeof(void *), qsort_compare_function);
>> +
>> +	f = fopen("/proc/kallsyms", "r");
>> +	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;
> 
> Same off chance we get ret == EOF and !feof(f)?
> 
>> +
>> +		if (ret != 2) {
>> +			pr_warn("failed to read kallsyms entry: %d\n", ret);
>> +			err = -EINVAL;
>> +			break;
>> +		}
>> +
>> +		if (!glob_match(sym_name, res->pattern))
>> +			continue;
>> +
>> +		if (!bsearch(&sym_name, infos.syms, infos.cnt, sizeof(void *),
>> +			     bsearch_compare_function))
>> +			continue;
> 
> I'm wondering if we could get a debug print if the func was skipped? Its
> not always clear when running many kernels what is going to be skipped
> and where.
> 

If there is no match, it will be skipped, and if you add printing, it
will become particularly noisy. And print here is not more helpful for
debugging, we don't care about skipped functions.
Jackie Liu July 4, 2023, 1:33 a.m. UTC | #4 
在 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.
Jiri Olsa July 4, 2023, 2:07 p.m. UTC | #5 
On Tue, Jul 04, 2023 at 09:33:15AM +0800, Jackie Liu wrote:

SNIP

> > 
> > 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, 

hum, libbpf_kallsyms_parse takes 'void *ctx', so you can pass anything
you want right? 

thanks,
jirka

> 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
> > >
Jiri Olsa July 4, 2023, 2:20 p.m. UTC | #6 
On Tue, Jul 04, 2023 at 04:07:48PM +0200, Jiri Olsa wrote:
> On Tue, Jul 04, 2023 at 09:33:15AM +0800, Jackie Liu wrote:
> 
> SNIP
> 
> > > 
> > > 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, 
> 
> hum, libbpf_kallsyms_parse takes 'void *ctx', so you can pass anything
> you want right? 

somthing like below should save some lines and ease up error handling

I'd add similar parse functions for both available_filter_functions and
available_filter_functions_addrs and add the logic to callbacks

jirka


---
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index b9282ef3f8a7..04b980293240 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -10559,8 +10559,31 @@ static int bsearch_compare_function(const void *a, const void *b)
 	return strcmp((const char *)a, *(const char **)b);
 }
 
+struct avail_kallsyms_data {
+	const char **syms;
+	struct kprobe_multi_resolve *res;
+};
+
+static int avail_kallsyms_cb(unsigned long long sym_addr, char sym_type,
+			     const char *sym_name, void *ctx)
+{
+	struct avail_kallsyms_data *data = ctx;
+	struct kprobe_multi_resolve *res = data->res;
+
+	if (!bsearch(&sym_name, data->syms, cnt, sizeof(void *), bsearch_compare_function))
+		continue;
+
+	err = libbpf_ensure_mem((void **)&res->addrs, &res->cap,
+				sizeof(unsigned long), res->cnt + 1);
+	if (err)
+		return err;
+	res->addrs[res->cnt++] = (unsigned long) sym_addr;
+	return 0;
+}
+
 static int libbpf_available_kallsyms_parse(struct kprobe_multi_resolve *res)
 {
+	struct avail_kallsyms_data data;
 	char sym_name[500];
 	const char *available_functions_file = tracefs_available_filter_functions();
 	FILE *f;
@@ -10614,42 +10637,13 @@ static int libbpf_available_kallsyms_parse(struct kprobe_multi_resolve *res)
 	/* sort available functions */
 	qsort(syms, cnt, sizeof(void *), qsort_compare_function);
 
-	f = fopen("/proc/kallsyms", "r");
-	if (!f) {
-		err = -errno;
-		pr_warn("failed to open /proc/kallsyms\n");
-		goto free_syms;
-	}
-
-	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;
-			goto cleanup;
-		}
-
-		if (!bsearch(&sym_name, syms, cnt, sizeof(void *), bsearch_compare_function))
-			continue;
-
-		err = libbpf_ensure_mem((void **)&res->addrs, &res->cap,
-					sizeof(unsigned long), res->cnt + 1);
-		if (err)
-			goto cleanup;
-
-		res->addrs[res->cnt++] = (unsigned long) sym_addr;
-	}
+	data.syms = syms;
+	data.res = res;
+	libbpf_kallsyms_parse(avail_kallsyms_cb, res);
 
 	if (!res->cnt)
 		err = -ENOENT;
 
-cleanup:
-	fclose(f);
 free_syms:
 	for (i = 0; i < cnt; i++)
 		free((char *)syms[i]);
diff mbox series

Patch

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 = {};
+
+	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);
+
+	/* sort available functions */
+	qsort(infos.syms, infos.cnt, sizeof(void *), qsort_compare_function);
+
+	f = fopen("/proc/kallsyms", "r");
+	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;
+
+		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;
+	}
+
+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) {