On Fri, Jul 31, 2020 at 1:34 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote: > > On 7/31/20 7:41 PM, Andrii Nakryiko wrote: > > On Wed, Jul 29, 2020 at 3:12 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote: > >> On 7/30/20 12:05 AM, Andrii Nakryiko wrote: > >>> 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? > >> > >> 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