Re: [PATCH v2 2/9] bpf/btf: tracing: Move finding func-proto API and getting func-param API to BTF

[Alan Maguire]

On 19/07/2023 16:24, Masami Hiramatsu (Google) wrote:
> On Wed, 19 Jul 2023 13:36:48 +0100
> Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
> 
>> On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
>>> From: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
>>>
>>> Move generic function-proto find API and getting function parameter API
>>> to BTF library code from trace_probe.c. This will avoid redundant efforts
>>> on different feature.
>>>
>>> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
>>> ---
>>>  include/linux/btf.h        |    4 ++++
>>>  kernel/bpf/btf.c           |   45 ++++++++++++++++++++++++++++++++++++++++
>>>  kernel/trace/trace_probe.c |   50 +++++++++++++-------------------------------
>>>  3 files changed, 64 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>>> index cac9f304e27a..98fbbcdd72ec 100644
>>> --- a/include/linux/btf.h
>>> +++ b/include/linux/btf.h
>>> @@ -221,6 +221,10 @@ const struct btf_type *
>>>  btf_resolve_size(const struct btf *btf, const struct btf_type *type,
>>>  		 u32 *type_size);
>>>  const char *btf_type_str(const struct btf_type *t);
>>> +const struct btf_type *btf_find_func_proto(struct btf *btf,
>>> +					   const char *func_name);
>>> +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto,
>>> +					   s32 *nr);
>>>  
>>>  #define for_each_member(i, struct_type, member)			\
>>>  	for (i = 0, member = btf_type_member(struct_type);	\
>>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>>> index 817204d53372..e015b52956cb 100644
>>> --- a/kernel/bpf/btf.c
>>> +++ b/kernel/bpf/btf.c
>>> @@ -1947,6 +1947,51 @@ btf_resolve_size(const struct btf *btf, const struct btf_type *type,
>>>  	return __btf_resolve_size(btf, type, type_size, NULL, NULL, NULL, NULL);
>>>  }
>>>  
>>> +/*
>>> + * Find a functio proto type by name, and return it.
>>> + * Return NULL if not found, or return -EINVAL if parameter is invalid.
>>> + */
>>> +const struct btf_type *btf_find_func_proto(struct btf *btf, const char *func_name)
>>> +{
>>> +	const struct btf_type *t;
>>> +	s32 id;
>>> +
>>> +	if (!btf || !func_name)
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	id = btf_find_by_name_kind(btf, func_name, BTF_KIND_FUNC);
>>
>> as mentioned in my other mail, there are cases where the function name
>> may have a .isra.0 suffix, but the BTF representation will not. I looked
>> at this and it seems like symbol names are validated via
>> traceprobe_parse_event_name() - will this validation allow a "."-suffix
>> name? I tried the following (with pahole v1.25 that emits BTF for
>> schedule_work.isra.0):
>>
>> [45454] FUNC 'schedule_work' type_id=45453 linkage=static
> 
> That's a good point! I'm checking fprobe, but kprobes actually
> uses those suffixed names.
> 
>>
>> $ echo 'f schedule_work.isra.0 $arg*' >> dynamic_events
>> bash: echo: write error: No such file or directory
> 
> So maybe fprobe doesn't accept that.
> 
>> So presuming that such "."-suffixed names are allowed, would it make
>> sense to fall back to search BTF for the prefix ("schedule_work")
>> instead of the full name ("schedule_work.isra.0"), as the former is what
>> makes it into the BTF representation? Thanks!
> 
> OK, that's not a problem. My concern is that some "constprop" functions
> will replace a part of parameter with constant value (so it can skip
> passing some arguments). BTF argument may not work in such case.
> Let me check it.
>

If the --btf_skip_inconsistent_proto option (also specified for pahole
v1.25) is working as expected, any such cases shouldn't make it into
BTF. The idea is we skip representing any static functions in BTF that

1. have multiple different function prototypes (since we don't yet have
good mechanisms to choose which one the user was referring to); or
2. use an unexpected register for a parameter. We gather that info from
DWARF and then make choices on whether to skip adding the function to
BTF or not.

See dwarf_loader.c in https://github.com/acmel/dwarves for more details
on the process used if needed.

The only exception for case 2 - where we allow unexpected registers - is
where multiple registers are used to represent a >64 bit structure
passed as a parameter by value. It might make sense to exclude such
functions from fprobe support (there's only a few of these functions in
the kernel if I remember, so it's no huge loss).

So long story short, if a function made it into in BTF, it is likely
using the registers you expect it to, unless it has a struct parameter.
It might be worth excluding such functions if you're worried about
getting unreliable data.

Thanks!

Alan

> Thank you,
> 
>>
>> Alan
>>
>>> +	if (id <= 0)
>>> +		return NULL;
>>> +
>>> +	/* Get BTF_KIND_FUNC type */
>>> +	t = btf_type_by_id(btf, id);
>>> +	if (!t || !btf_type_is_func(t))
>>> +		return NULL;
>>> +
>>> +	/* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
>>> +	t = btf_type_by_id(btf, t->type);
>>> +	if (!t || !btf_type_is_func_proto(t))
>>> +		return NULL;
>>> +
>>> +	return t;
>>> +}
>>> +
>>> +/*
>>> + * Get function parameter with the number of parameters.
>>> + * This can return NULL if the function has no parameters.
>>> + */
>>> +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s32 *nr)
>>> +{
>>> +	if (!func_proto || !nr)
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	*nr = btf_type_vlen(func_proto);
>>> +	if (*nr > 0)
>>> +		return (const struct btf_param *)(func_proto + 1);
>>> +	else
>>> +		return NULL;
>>> +}
>>> +
>>>  static u32 btf_resolved_type_id(const struct btf *btf, u32 type_id)
>>>  {
>>>  	while (type_id < btf->start_id)
>>> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
>>> index c68a72707852..cd89fc1ebb42 100644
>>> --- a/kernel/trace/trace_probe.c
>>> +++ b/kernel/trace/trace_probe.c
>>> @@ -371,47 +371,23 @@ static const char *type_from_btf_id(struct btf *btf, s32 id)
>>>  	return NULL;
>>>  }
>>>  
>>> -static const struct btf_type *find_btf_func_proto(const char *funcname)
>>> -{
>>> -	struct btf *btf = traceprobe_get_btf();
>>> -	const struct btf_type *t;
>>> -	s32 id;
>>> -
>>> -	if (!btf || !funcname)
>>> -		return ERR_PTR(-EINVAL);
>>> -
>>> -	id = btf_find_by_name_kind(btf, funcname, BTF_KIND_FUNC);
>>> -	if (id <= 0)
>>> -		return ERR_PTR(-ENOENT);
>>> -
>>> -	/* Get BTF_KIND_FUNC type */
>>> -	t = btf_type_by_id(btf, id);
>>> -	if (!t || !btf_type_is_func(t))
>>> -		return ERR_PTR(-ENOENT);
>>> -
>>> -	/* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
>>> -	t = btf_type_by_id(btf, t->type);
>>> -	if (!t || !btf_type_is_func_proto(t))
>>> -		return ERR_PTR(-ENOENT);
>>> -
>>> -	return t;
>>> -}
>>> -
>>>  static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr,
>>>  						   bool tracepoint)
>>>  {
>>> +	struct btf *btf = traceprobe_get_btf();
>>>  	const struct btf_param *param;
>>>  	const struct btf_type *t;
>>>  
>>> -	if (!funcname || !nr)
>>> +	if (!funcname || !nr || !btf)
>>>  		return ERR_PTR(-EINVAL);
>>>  
>>> -	t = find_btf_func_proto(funcname);
>>> -	if (IS_ERR(t))
>>> +	t = btf_find_func_proto(btf, funcname);
>>> +	if (IS_ERR_OR_NULL(t))
>>>  		return (const struct btf_param *)t;
>>>  
>>> -	*nr = btf_type_vlen(t);
>>> -	param = (const struct btf_param *)(t + 1);
>>> +	param = btf_get_func_param(t, nr);
>>> +	if (IS_ERR_OR_NULL(param))
>>> +		return param;
>>>  
>>>  	/* Hide the first 'data' argument of tracepoint */
>>>  	if (tracepoint) {
>>> @@ -490,8 +466,8 @@ static const struct fetch_type *parse_btf_retval_type(
>>>  	const struct btf_type *t;
>>>  
>>>  	if (btf && ctx->funcname) {
>>> -		t = find_btf_func_proto(ctx->funcname);
>>> -		if (!IS_ERR(t))
>>> +		t = btf_find_func_proto(btf, ctx->funcname);
>>> +		if (!IS_ERR_OR_NULL(t))
>>>  			typestr = type_from_btf_id(btf, t->type);
>>>  	}
>>>  
>>> @@ -500,10 +476,14 @@ static const struct fetch_type *parse_btf_retval_type(
>>>  
>>>  static bool is_btf_retval_void(const char *funcname)
>>>  {
>>> +	struct btf *btf = traceprobe_get_btf();
>>>  	const struct btf_type *t;
>>>  
>>> -	t = find_btf_func_proto(funcname);
>>> -	if (IS_ERR(t))
>>> +	if (!btf)
>>> +		return false;
>>> +
>>> +	t = btf_find_func_proto(btf, funcname);
>>> +	if (IS_ERR_OR_NULL(t))
>>>  		return false;
>>>  
>>>  	return t->type == 0;
>>>
>>>
> 
>