Re: [PATCH bpf-next 4/6] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers

[Song Liu <songliubraving@fb.com>]

> On Oct 1, 2019, at 3:42 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> On Tue, Oct 1, 2019 at 2:46 PM Song Liu <songliubraving@fb.com> wrote:
>> 
>> 
>> 
>>> On Oct 1, 2019, at 2:25 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>>> 
>>> On Tue, Oct 1, 2019 at 2:14 PM Song Liu <songliubraving@fb.com> wrote:
>>>> 
>>>> 
>>>>> On Sep 30, 2019, at 11:58 AM, Andrii Nakryiko <andriin@fb.com> wrote:
>>>>> 
>>>>> Add few macros simplifying BCC-like multi-level probe reads, while also
>>>>> emitting CO-RE relocations for each read.
>>>>> 
>>>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>>>> ---
>>>>> tools/lib/bpf/bpf_helpers.h | 151 +++++++++++++++++++++++++++++++++++-
>>>>> 1 file changed, 147 insertions(+), 4 deletions(-)
>>>>> 
>>>>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
>>>>> index a1d9b97b8e15..51e7b11d53e8 100644
>>>>> --- a/tools/lib/bpf/bpf_helpers.h
>>>>> +++ b/tools/lib/bpf/bpf_helpers.h
>>>>> @@ -19,6 +19,10 @@
>>>>> */
>>>>> #define SEC(NAME) __attribute__((section(NAME), used))
>>>>> 
>>>>> +#ifndef __always_inline
>>>>> +#define __always_inline __attribute__((always_inline))
>>>>> +#endif
>>>>> +
>>>>> /* helper functions called from eBPF programs written in C */
>>>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
>>>>>     (void *) BPF_FUNC_map_lookup_elem;
>>>>> @@ -505,7 +509,7 @@ struct pt_regs;
>>>>> #endif
>>>>> 
>>>>> /*
>>>>> - * BPF_CORE_READ abstracts away bpf_probe_read() call and captures offset
>>>>> + * bpf_core_read() abstracts away bpf_probe_read() call and captures field
>>>>> * relocation for source address using __builtin_preserve_access_index()
>>>>> * built-in, provided by Clang.
>>>>> *
>>>>> @@ -520,8 +524,147 @@ struct pt_regs;
>>>>> * actual field offset, based on target kernel BTF type that matches original
>>>>> * (local) BTF, used to record relocation.
>>>>> */
>>>>> -#define BPF_CORE_READ(dst, src)                                              \
>>>>> -     bpf_probe_read((dst), sizeof(*(src)),                           \
>>>>> -                    __builtin_preserve_access_index(src))
>>>>> +#define bpf_core_read(dst, sz, src)                                      \
>>>>> +     bpf_probe_read(dst, sz,                                             \
>>>>> +                    (const void *)__builtin_preserve_access_index(src))
>>>>> +
>>>>> +/*
>>>>> + * bpf_core_read_str() is a thin wrapper around bpf_probe_read_str()
>>>>> + * additionally emitting BPF CO-RE field relocation for specified source
>>>>> + * argument.
>>>>> + */
>>>>> +#define bpf_core_read_str(dst, sz, src)                                          \
>>>>> +     bpf_probe_read_str(dst, sz,                                         \
>>>>> +                        (const void *)__builtin_preserve_access_index(src))
>>>>> +
>>>>> +#define ___concat(a, b) a ## b
>>>>> +#define ___apply(fn, n) ___concat(fn, n)
>>>>> +#define ___nth(_1, _2, _3, _4, _5, _6, _7, _8, _9, _10, __11, N, ...) N
>>>> 
>>>> We are adding many marcos with simple names: ___apply(), ___nth. So I worry
>>>> they may conflict with macro definitions from other libraries. Shall we hide
>>>> them in .c files or prefix/postfix them with _libbpf or something?
>>> 
>>> Keep in mind, this is the header that's included from BPF code.
>>> 
>>> They are prefixed with three underscores, I was hoping it's good
>>> enough to avoid accidental conflicts. It's unlikely someone will have
>>> macros with the same names **in BPF-side code**.
>> 
>> BPF side code would include kernel headers. So there are many headers
>> to conflict with. And we won't know until somebody want to trace certain
>> kernel structure.
> 
> We have all the kernel sources at our disposal, there's no need to
> guess :) There is no instance of ___apply, ___concat, ___nth,
> ___arrow, ___last, ___nolast, or ___type, not even speaking about
> other more specific names. There are currently two instances of
> "____last_____" used in a string. And I'm certainly not afraid that
> user code can use triple-underscored identifier with exact those names
> and complain about bpf_helpers.h :)

I worry more about _future_ conflicts, that someone may add ___apply to 
some kernel header file and break some BPF programs. Since these BPF
programs are not in-tree, it is very difficult to test them properly.
We have had name conflicts from other libraries, so I hope we don't create 
more ourselves. 

Thanks,
Song