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.
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
