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?
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.
>
> > 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(-)
> >>>>
> >>>
> >>> [...]
> >>>
> >>
>
