Re: [PATCH v4] libbpf: kprobe.multi: Filter with available_filter_functions

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

在 2023/6/8 08:00, Andrii Nakryiko 写道:
> On Wed, Jun 7, 2023 at 4:22 PM Jiri Olsa <olsajiri@gmail.com> wrote:
>>
>> On Fri, Jun 02, 2023 at 10:27:31AM -0700, Andrii Nakryiko wrote:
>>> On Thu, May 25, 2023 at 6:38 PM Jackie Liu <liu.yun@linux.dev> wrote:
>>>>
>>>> Hi Andrii.
>>>>
>>>> 在 2023/5/26 04:43, Andrii Nakryiko 写道:
>>>>> On Thu, May 25, 2023 at 3:28 AM Jackie Liu <liu.yun@linux.dev> 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.
>>>>>>
>>>>>> Use available_filter_functions check first, if failed, fallback to
>>>>>> kallsyms.
>>>>>>
>>>>>> Here is the test eBPF program [1].
>>>>>> [1] https://github.com/JackieLiu1/ketones/commit/a9e76d1ba57390e533b8b3eadde97f7a4535e867
>>>>>>
>>>>>> Suggested-by: Jiri Olsa <olsajiri@gmail.com>
>>>>>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>>>>>> ---
>>>>>>    tools/lib/bpf/libbpf.c | 92 +++++++++++++++++++++++++++++++++++++-----
>>>>>>    1 file changed, 83 insertions(+), 9 deletions(-)
>>>>>>
>>>>>
>>>>> Question to you and Jiri: what happens when multi-kprobe's syms has
>>>>> duplicates? Will the program be attached multiple times? If yes, then
>>>>> it sounds like a problem? Both available_filters and kallsyms can have
>>>>> duplicate function names in them, right?
>>>>
>>>> If I understand correctly, there should be no problem with repeated
>>>> function registration, because the bottom layer is done through fprobe
>>>> registration addrs, kprobe.multi itself does not do this work, but
>>>> fprobe is based on ftrace, it will register addr by makes a hash,
>>>> that is, if it is the same address, it should be filtered out.
>>>>
>>>
>>> Looking at kernel code, it seems kernel will actually return error if
>>> user specifies multiple duplicated names. Because kernel will
>>> bsearch() to the first instance, and never resolve the second
>>> duplicated instance. And then will assume that not all symbols are
>>> resolved.
>>
>> right, as I wrote in here [1] it will fail
>>
>> [1] https://lore.kernel.org/bpf/ZHB0xNEbjmwHv18d@krava/
>>
>>>
>>> So, it worries me that we'll switch from kallsyms to available_filters
>>> by default, because that introduces new failure modes.
>>
>> we did not care about duplicate with kallsyms because we used addresses,
>> and I think with duplicate addresss the kprobe_multi link will probably
>> attach (need to check) while with duplicate symbols it won't..
>>
>> perhaps we could make sure we don't pass duplicate symbols?
> 
> I think we have to stick to kallsyms and addresses. What if I actually
> want to attach to all instances of type_show? We should take into
> account available_filter_functions, but still use addresses from
> kallsyms.
> 
> I'd also advocate working on having an available_filter_functions
> version reporting not just function names, but also its associated
> address. That would actually eliminate the need for kallsyms.
> 
> I chatted with Steven Rostedt about this at the last LSF/MM/BPF
> conference, and I think we both agreed that we both a) have all the
> information in the kernel to implement this and b) it's a good idea to
> expose all that to user space. For backwards compat reasons it will
> have to be a separate file, but it's generated on the fly, so it's not
> a big deal in terms of resource usage.

Yes, I noticed that the latest version of the kernel has added 
touched_functions and enabled_functions, are they? I'm not sure.
Perhaps we can wait for such an interface to appear before directly
switching to that interface, and then submit this patch again.

-- 
Jackie Liu

> 
> 
>>
>> we do the kprobe_multi bench with symbol names read from available_filter_functions
>> and we filter out duplicates
>>
>> jirka
>>
>>>
>>> Either way, let's add a selftest that uses a duplicate function name
>>> and see what happens?
>>>
>>>> The main problem here is not the problem of repeated registration of
>>>> functions, but some functions are not allowed to hook. For example, when
>>>> I track vfs_*, vfs_set_acl_prepare_kgid and vfs_set_acl_prepare_kuid are
>>>> not allowed to hook. These exist under kallsyms, but
>>>> available_filter_functions does not, I have observed for a while,
>>>> matching through available_filter_functions can effectively prevent this
>>>> from happening.
>>>
>>> Yeah, I understand that. My point above is that a)
>>> available_filter_functions contains duplicates and b) doesn't contain
>>> addresses. So we are forced to rely on kernel string -> addr
>>> resolution, which doesn't seem to handle duplicate entries well (let's
>>> test).
>>>
>>> So it's a regression to switch to that without taking any other precautions.
>>>
>>>>
>>>>>
>>>>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>>>>> index ad1ec893b41b..3dd72d69cdf7 100644
>>>>>> --- a/tools/lib/bpf/libbpf.c
>>>>>> +++ b/tools/lib/bpf/libbpf.c
>>>>>> @@ -10417,13 +10417,14 @@ static bool glob_match(const char *str, const char *pat)
>>>>>>    struct kprobe_multi_resolve {
>>>>>>           const char *pattern;
>>>>>>           unsigned long *addrs;
>>>>>> +       const char **syms;
>>>>>>           size_t cap;
>>>>>>           size_t cnt;
>>>>>>    };
>>>>>>
>>>
>>> [...]