On Tue, May 4, 2021 at 12:25 PM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 5/4/21 8:31 AM, Grant Seltzer Richman wrote:
> > On Mon, May 3, 2021 at 5:22 PM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> >>
> >> On Mon, May 3, 2021 at 1:20 PM Grant Seltzer Richman
> >> <grantseltzer@xxxxxxxxx> wrote:
> >>>
> >>> On Mon, May 3, 2021 at 2:43 PM Andrii Nakryiko
> >>> <andrii.nakryiko@xxxxxxxxx> wrote:
> >>>>
> >>>> On Mon, May 3, 2021 at 11:32 AM Grant Seltzer Richman
> >>>> <grantseltzer@xxxxxxxxx> wrote:
> >>>>>
> >>>>> On Wed, Apr 28, 2021 at 5:15 PM Andrii Nakryiko
> >>>>> <andrii.nakryiko@xxxxxxxxx> wrote:
> >>>>>>
> >>>>>> On Wed, Apr 28, 2021 at 1:53 PM Grant Seltzer Richman
> >>>>>> <grantseltzer@xxxxxxxxx> wrote:
> >>>>>>>
> >>>>>>> Hi all,
> >>>>>>>
> >>>>>>> I'm working on enabling CO:RE in a project I work on, tracee, and am
> >>>>>>> running into the dilemma of missing macros that we previously were
> >>>>>>> able to import from their various header files. I understand that
> >>>>>>> macros don't make their way into BTF and therefore the generated
> >>>>>>> vmlinux.h won't have them. However I can't import the various header
> >>>>>>> files because of multiple-definition issues.
> >>>>>>
> >>>>>> Sadly, copy/pasting has been the only way so far.
> >>>>>>
> >>>>>>>
> >>>>>>> Do people typically redefine each of these macros for their project?
> >>>>>>> If so is there anything I should be careful of, such as architectural
> >>>>>>> differences. Does anyone have creative ideas, even if not developed
> >>>>>>> fully yet that I can possibly contribute to libbpf?
> >>>>>>
> >>>>>> We've discussed adding Clang built-in to detect if a specific type is
> >>>>>> already defined and doing something like this in vmlinux.h:
> >>>>>>
> >>>>>> #if !__builtin_is_type_defined(struct task_struct)
> >>>>>> struct task_struct {
> >>>>>> ...
> >>>>>> }
> >>>>>> #endif
> >>>>>>
> >>>>>> And just do that for every struct, union, typedef. That would allow
> >>>>>> vmlinux.h to co-exist (somewhat) with other types.
> >>>>>>
> >>>>>> Another alternative is to not use vmlinux.h and use just linux
> >>>>>> headers, but mark necessary types with
> >>>>>> __attribute__((preserve_access_index)) to make them CO-RE relocatable.
> >>>>>> You can add that to existing types with the same pragma that vmlinux.h
> >>>>>> uses.
> >>>>>
> >>>>> I'm attempting to try doing the above. I'm just replacing
> >>>>> bpf_probe_read with bpf_core_read and not importing vmlinux.h, just
> >>>>> all the kernel headers I need.
> >>>>
> >>>> Yes, that will work, bpf_core_read() uses preserve_access_index
> >>>> built-in to achieve the same effect.
> >>>>
> >>>>>
> >>>>> When you say "Add that to existing types with the same pragma that
> >>>>> vmlinux.h uses", Should I be able to add the following to my bpf
> >>>>> source file before importing my headers?
> >>>>>
> >>>>> ifndef BPF_NO_PRESERVE_ACCESS_INDEX
> >>>>> #pragma clang attribute push (__attribute__((preserve_access_index)),
> >>>>> apply_to = record)
> >>>>> #endif
> >>>>>
> >>>>> and then pop the attribute at the bottom of the file, or after the
> >>>>> header includes.
> >>>>
> >>>> Yeah, that's the idea and that's what vmlinux.h does for all its
> >>>> structs. It doesn't add __attribute__((preserve_access_index)) after
> >>>> each struct/union. So I wonder why you are getting those unknown
> >>>> attribute errors. Can you paste an example?
> >>>
> >>> Here's a couple examples of the warnings:
> >>>
> >>> ```
> >>> tracee/tracee.bpf.c:5:46: warning: unknown attribute
> >>> 'preserve_access_index' ignored [-Wunknown-attributes]
> >>> #pragma clang attribute push (__attribute__((preserve_access_index)),
> >>> apply_to = record)
> >>> ^
> >>> /lib/modules/5.10.21-200.fc33.x86_64/source/include/linux/ipv6.h:185:1:
> >>> note: when applied to this declaration
> >>> struct ipv6_fl_socklist;
> >>> ^
> >>> tracee/tracee.bpf.c:5:46: warning: unknown attribute
> >>> 'preserve_access_index' ignored [-Wunknown-attributes]
> >>> #pragma clang attribute push (__attribute__((preserve_access_index)),
> >>> apply_to = record)
> >>> ^
> >>> /lib/modules/5.10.21-200.fc33.x86_64/source/include/linux/ipv6.h:187:1:
> >>> note: when applied to this declaration
> >>> struct inet6_cork {
> >>> ```
> >>>
> >>> after these warnings are emitted (it seems as if there's one for every
> >>> data type, though I can't confirm), I get errors that look like this:
> >>>
> >>> ```
> >>> tracee/tracee.bpf.c:445:22: error: nested
> >>> builtin_preserve_access_index() not supported
> >>> return READ_KERN(READ_KERN(task->thread_pid)->numbers[level].nr);
> >>> ^
> >>> tracee/tracee.bpf.c:206:27: note: expanded from macro 'READ_KERN'
> >>> bpf_core_read(&_val, sizeof(_val), &ptr); \
> >>> ```
> >>> I believe this is just a result of the warnings above, but if you're
> >>> curious it's what i'm doing here:
> >>> https://github.com/aquasecurity/tracee/blob/core-experiment/tracee-ebpf/tracee/tracee.bpf.c#L204-L208
> >>>
> >>
> >> Looking at your Makefile, you are not using `clang -target bpf` to
> >> compile BPF object files, which is probably what causes you trouble.
> >> preserve_access_index is a BPF target-only attribute. There is no need
> >> to do the legacy clang -emit-llvm | llc, especially when you are using
> >> CO-RE.
> >
> > Got it. Funny enough, it turns out this is just a continuation of a
> > conversation you had with my coworker Yaniv last year:
> > https://lore.kernel.org/bpf/CAEf4BzbshRMCX1T1ooAtYGYuUGefbbo2=ProkMg5iOtUKh3YtQ@xxxxxxxxxxxxxx/
> >
> > But to summarize our continued challenge: Adding the
> > `preserve_access_index` attribute, compiling with `-target bpf`, and
> > using the same kernel headers we used (not vmlinux.h) causes issues
> > because of architecture specific asm errors (likely stemming from
> > headers we include). Unless there's a way to get around those we're
> > going to need to include "vmlinux.h", change our Makefile to `-target
> > bpf`, and redefine macros and/or functions that vmlinux.h does not
> > provide.
> >
> > I think this is a pretty significant usability challenge. The idea you
> > mentioned of having a built-in to detect if a type is defined would be
> > a huge step forward. Has any progress been made towards this?
>
> I briefly looked at this probably one and half years ago.
> It will involve tweak clang frontend cpp side. Now I haven't
> done any concrete work yet. But will look at it in the future.
Got it - thanks for the info. Also any advice you or anyone else has
to avoid having to redefine functions would be very much appreciated!
>
> >
> > Another thought is having vmlinux.h include function definitions,
> > aren't they included in DWARF/BTF?
> >
> > Thanks for your help, as always, Andrii!
> >
> >>
> >>>>
> >>>> Also check that you use Clang that supports preserve_access_index, of course.
> >>>
> >>> I'm using clang 11.0 on Fedora 33. All dependencies appear properly
> >>> installed (libelf, zlib, dwarves [provides pahole], llvm, llc,
> >>> llvm-devel,...)
> >>>
> >>>>
> >>>>>
> >>>>> I've tried this and get a whole bunch of 'unknown attribute' warnings,
> >>>>> leading me to believe that I either have something installed
> >>>>> incorrectly or don't understand how to use clang attributes. Do I need
> >>>>> to edit the types in the actual header files?
> >>>>
> >>>> No, the whole idea is to not touch original headers.
> >>>
> >>> Got it - that's good to know.
> >>>
> >>>>
> >>>>>
> >>>>> Thank you very very much for the help!
> >>>>> - Grant
> >>>>>>
> >>>>>>>
> >>>>>>> Thanks so much,
> >>>>>>> Grant Seltzer
