Re: [PATCH bpf-next 3/3] libbpf: Use bpf_probe_read_kernel

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Jul 31, 2020 at 1:34 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 7/31/20 7:41 PM, Andrii Nakryiko wrote:
> > On Wed, Jul 29, 2020 at 3:12 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >> On 7/30/20 12:05 AM, Andrii Nakryiko wrote:
> >>> On Wed, Jul 29, 2020 at 2:54 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>>> On 7/29/20 11:36 PM, Andrii Nakryiko wrote:
> >>>>> On Wed, Jul 29, 2020 at 2:01 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>>>>> On 7/29/20 6:06 AM, Andrii Nakryiko wrote:
> >>>>>>> On Tue, Jul 28, 2020 at 2:16 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>>>>>>> On 7/28/20 9:11 PM, Andrii Nakryiko wrote:
> >>>>>>>>> On Tue, Jul 28, 2020 at 5:15 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Yet another adaptation to commit 0ebeea8ca8a4 ("bpf: Restrict
> >>>>>>>>>> bpf_probe_read{, str}() only to archs where they work") that makes more
> >>>>>>>>>> samples compile on s390.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> >>>>>>>>>
> >>>>>>>>> Sorry, we can't do this yet. This will break on older kernels that
> >>>>>>>>> don't yet have bpf_probe_read_kernel() implemented. Met and Yonghong
> >>>>>>>>> are working on extending a set of CO-RE relocations, that would allow
> >>>>>>>>> to do bpf_probe_read_kernel() detection on BPF side, transparently for
> >>>>>>>>> an application, and will pick either bpf_probe_read() or
> >>>>>>>>> bpf_probe_read_kernel(). It should be ready soon (this or next week,
> >>>>>>>>> most probably), though it will have dependency on the latest Clang.
> >>>>>>>>> But for now, please don't change this.
> >>>>>>>>
> >>>>>>>> Could you elaborate what this means wrt dependency on latest clang? Given clang
> >>>>>>>> releases have a rather long cadence, what about existing users with current clang
> >>>>>>>> releases?
> >>>>>>>
> >>>>>>> So the overall idea is to use something like this to do kernel reads:
> >>>>>>>
> >>>>>>> static __always_inline int bpf_probe_read_universal(void *dst, u32 sz,
> >>>>>>> const void *src)
> >>>>>>> {
> >>>>>>>         if (bpf_core_type_exists(btf_bpf_probe_read_kernel))
> >>>>>>>             return bpf_probe_read_kernel(dst, sz, src);
> >>>>>>>         else
> >>>>>>>             return bpf_probe_read(dst, sz, src);
> >>>>>>> }
> >>>>>>>
> >>>>>>> And then use bpf_probe_read_universal() in BPF_CORE_READ and family.
> >>>>>>>
> >>>>>>> This approach relies on few things:
> >>>>>>>
> >>>>>>> 1. each BPF helper has a corresponding btf_<helper-name> type defined for it
> >>>>>>> 2. bpf_core_type_exists(some_type) returns 0 or 1, depending if
> >>>>>>> specified type is found in kernel BTF (so needs kernel BTF, of
> >>>>>>> course). This is the part me and Yonghong are working on at the
> >>>>>>> moment.
> >>>>>>> 3. verifier's dead code elimination, which will leave only
> >>>>>>> bpf_probe_read() or bpf_probe_read_kernel() calls and will remove the
> >>>>>>> other one. So on older kernels, there will never be unsupoorted call
> >>>>>>> to bpf_probe_read_kernel().
> >>>>>>>
> >>>>>>> The new type existence relocation requires the latest Clang. So the
> >>>>>>> way to deal with older Clangs would be to just fallback to
> >>>>>>> bpf_probe_read, if we detect that Clang is too old and can't emit
> >>>>>>> necessary relocation.
> >>>>>>
> >>>>>> Okay, seems reasonable overall. One question though: couldn't libbpf transparently
> >>>>>> fix up the selection of bpf_probe_read() vs bpf_probe_read_kernel()? E.g. it would
> >>>>>> probe the kernel whether bpf_probe_read_kernel() is available and if it is then it
> >>>>>> would rewrite the raw call number from the instruction from bpf_probe_read() into
> >>>>>> the one for bpf_probe_read_kernel()? I guess the question then becomes whether the
> >>>>>> original use for bpf_probe_read() was related to CO-RE. But I think this could also
> >>>>>> be overcome by adding a fake helper signature in libbpf with a unreasonable high
> >>>>>> number that is dedicated to probing mem via CO-RE and then libbpf picks the right
> >>>>>> underlying helper call number for the insn. That avoids fiddling with macros and
> >>>>>> need for new clang version, no (unless I'm missing something)?
> >>>>>
> >>>>> Libbpf could do it, but I'm a bit worried that unconditionally
> >>>>> changing bpf_probe_read() into bpf_probe_read_kernel() is going to be
> >>>>> wrong in some cases. If that wasn't the case, why wouldn't we just
> >>>>> re-purpose bpf_probe_read() into bpf_probe_read_kernel() in kernel
> >>>>> itself, right?
> >>>>
> >>>> Yes, that is correct, but I mentioned above that this new 'fake' helper call number
> >>>> that libbpf would be fixing up would only be used for bpf_probe_read{,str}() inside
> >>>> bpf_core_read.h.
> >>>>
> >>>> Small example, bpf_core_read.h would be changed to (just an extract):
> >>>>
> >>>> diff --git a/tools/lib/bpf/bpf_core_read.h b/tools/lib/bpf/bpf_core_read.h
> >>>> index eae5cccff761..4bddb2ddf3f0 100644
> >>>> --- a/tools/lib/bpf/bpf_core_read.h
> >>>> +++ b/tools/lib/bpf/bpf_core_read.h
> >>>> @@ -115,7 +115,7 @@ enum bpf_field_info_kind {
> >>>>      * (local) BTF, used to record relocation.
> >>>>      */
> >>>>     #define bpf_core_read(dst, sz, src)                                        \
> >>>> -       bpf_probe_read(dst, sz,                                             \
> >>>> +       bpf_probe_read_selector(dst, sz,                                                    \
> >>>>                           (const void *)__builtin_preserve_access_index(src))
> >>>>
> >>>>     /*
> >>>> @@ -124,7 +124,7 @@ enum bpf_field_info_kind {
> >>>>      * argument.
> >>>>      */
> >>>>     #define bpf_core_read_str(dst, sz, src)                                            \
> >>>> -       bpf_probe_read_str(dst, sz,                                         \
> >>>> +       bpf_probe_read_str_selector(dst, sz,                                        \
> >>>>                               (const void *)__builtin_preserve_access_index(src))
> >>>>
> >>>>     #define ___concat(a, b) a ## b
> >>>>
> >>>> And bpf_probe_read_{,str_}selector would be defined as e.g. ...
> >>>>
> >>>> static long (*bpf_probe_read_selector)(void *dst, __u32 size, const void *unsafe_ptr) = (void *) -1;
> >>>> static long (*bpf_probe_read_str_selector)(void *dst, __u32 size, const void *unsafe_ptr) = (void *) -2;
> >>>>
> >>>> ... where libbpf would do the fix up to either 4 or 45 for insn->imm. But it's still
> >>>> confined to usage in bpf_core_read.h when the CO-RE macros are used.
> >>>
> >>> Ah, I see. Yeah, I suppose that would work as well. Do you prefer me
> >>> to go this way?
> >>
> >> I would suggest we should try this path given this can be used with any clang version
> >> that has the BPF backend, not just latest upstream git.
> >
> > I have an even better solution, I think. Convert everything to
> > bpf_probe_read_kernel() or bpf_probe_read_user() unconditionally, but
> > let libbpf switch those two to bpf_probe_read() if _kernel()/_user()
> > variants are not yet in the kernel. That should handle both CO-RE
> > helpers and just pretty much any use case that was converted.
>
> Yes, agree, that is an even cleaner solution and avoids to 'pollute' the
> helper ID space with such remapping. The user intent with bpf_probe_read_kernel()
> or bpf_probe_read_user() is rather clear so bpf_probe_read() can be a fallback
> for this direction. Lets go with that.
>

Ok, I have all this working locally. I'll post patches once bpf-next re-opens.

> Thanks,
> Daniel