On Wed, Jul 29, 2020 at 2:54 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> On 7/29/20 11:36 PM, Andrii Nakryiko wrote:
> > On Wed, Jul 29, 2020 at 2:01 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
> >> On 7/29/20 6:06 AM, Andrii Nakryiko wrote:
> >>> On Tue, Jul 28, 2020 at 2:16 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
> >>>> On 7/28/20 9:11 PM, Andrii Nakryiko wrote:
> >>>>> On Tue, Jul 28, 2020 at 5:15 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >>>>>
> >>>>> 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?
>
> > But fear not about old Clang support. The bpf_core_type_exists() will
> > use a new built-in, and I'll be able to detect its presence with
> > __has_builtin(X) check in Clang. So it will be completely transparent
> > to users in the end.
>
> Ok.
>
> >>> If that's not an acceptable plan, then one can "parameterize"
> >>> BPF_CORE_READ macro family by re-defining bpf_core_read() macro. Right
> >>> now it's defined as:
> >>>
> >>> #define bpf_core_read(dst, sz, src) \
> >>> bpf_probe_read(dst, sz, (const void *)__builtin_preserve_access_index(src))
> >>>
> >>> Re-defining it in terms of bpf_probe_read_kernel is trivial, but I
> >>> can't do it for BPF_CORE_READ, because it will break all the users of
> >>> bpf_core_read.h that run on older kernels.
> >>>
> >>>
> >>>>
> >>>>>> tools/lib/bpf/bpf_core_read.h | 51 ++++++++++++++++++-----------------
> >>>>>> tools/lib/bpf/bpf_tracing.h | 15 +++++++----
> >>>>>> 2 files changed, 37 insertions(+), 29 deletions(-)
> >>>>>>
> >>>>>
> >>>>> [...]
> >>>>>
> >>>>
> >>
>
