llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
26.82k stars 11k forks source link

BPF target: incorrectly assumes 8byte pointers even for clang -target i386 #36954

Open be579e8c-b74b-475b-a126-3b8cefdd833c opened 6 years ago

be579e8c-b74b-475b-a126-3b8cefdd833c commented 6 years ago
Bugzilla Link 37606
Version trunk
OS Linux
Attachments Using pointers
CC @4ast,@tstellar,@yonghong-song

Extended Description

LLC mistranslates the "Using pointers" attachment example, derived from linux-kernel-4.9/samples/bpf/

   0:       61 12 18 00 00 00 00 00         r2 = *(u32 *)(r1 + 24)
   1:       61 13 1c 00 00 00 00 00         r3 = *(u32 *)(r1 + 28)
   2:       67 03 00 00 20 00 00 00         r3 <<= 32
   3:       4f 23 00 00 00 00 00 00         r3 |= r2
   4:       07 03 00 00 20 00 00 00         r3 += 32
   5:       bf a1 00 00 00 00 00 00         r1 = r10
   6:       07 01 00 00 f8 ff ff ff         r1 += -8
   7:       b7 02 00 00 04 00 00 00         r2 = 4
   8:       85 00 00 00 04 00 00 00         call 4

Inspection by "pahole" shows that the load-addresses used in the generated BPF assembly assume the layout of 4.9.0-0.bpo.6-amd64 (8byte pointers, 'dev' is at offset dec 32) rather than 4.9.0-0.bpo.6-686-pae (32byte pointers, 'dev' at offset dec 20), as one would expect, because the LLVM-IR was generated by Clang with "-target i386" and specifies the relevant data sizes as 4 byte rather than 8 byte in the datalayout string

The workaround is more than ugly. Currently it consists of storing pointers as u32 on the BPF stack and accessing members using "offsetof", which apparently gets correctly resolved at an earlier stage by clang.

#define _MEMBER(TYPE,P,M) (*((typeof(((TYPE*)0)->M)*) (((char*)(P)) + offsetof(TYPE, M))))

/* ... */
dev = (u32)_(_MEMBER(struct sk_buff,skb,dev))
tstellar commented 5 years ago

Thanks for the explanation, this is really helpful. Have you looked into trying to solve this in the clang frontend with some kind of -use-host-layout flag?

so clang will treat the layout as native (x64 or arm32/64), but will emit bpf IR ? that's an interesting idea! we should explore that.

Yes, I'm not sure what kind of defines the kernel header relies on, but you could modify the bpf target in clang to add defines like #define aarch64 depending on which host you were targeting. So something like: clang -target bpf -bpf-host-arch=aarch64

Even with the defines, though, you still may need to do something with the DataLayout to get the correct pointer size / alignment if the host uses something different than what the BPF target currently defines. I'm not sure how much work this will be, but it seems worth looking into.

4ast commented 5 years ago

Thanks for the explanation, this is really helpful. Have you looked into trying to solve this in the clang frontend with some kind of -use-host-layout flag?

so clang will treat the layout as native (x64 or arm32/64), but will emit bpf IR ? that's an interesting idea! we should explore that.

tstellar commented 5 years ago

Can you elaborate more on why having a bpf32 target won't help? To be clear I am suggesting adding a bpf32 target while still keeping the existing bpf target for 64-bit hosts.

because the difference in structs don't come purely from 32 vs 64-bitness of pointers. Layout changes depending on a ton of #defines that depend on arch too. This approach is not errorproof. Unfortunately we had issues with clang vs gcc. Like gcc had additional wrapping of some inner fields with anon struct {} and clang didn't. That changed offsets in 'struct task_struct' and a bunch of iovisor/bcc scripts broke. It's all fragile until we have proper BTF in llvm and kernel. BTF is a safe debug format. Meaning that kernel verifies its structure. There are several use cases for it. The main is to do adjustment of field offsets for the given host. So tracing bpf programs can be compiled once with -target bpf and offsets adjusted dynamically during prog load depending on the kernel.

Thanks for the explanation, this is really helpful. Have you looked into trying to solve this in the clang frontend with some kind of -use-host-layout flag?

4ast commented 5 years ago

despite being 'unsupported' clang -target native | llc -mach=bpf is the only way we can get appropriate offsets today.

Do we still need to do it this way if the native target is x86_64?

yes. since kernel structs may have different layout x64 vs arm64 vs bpf. Using -target bpf won't produce exactly the same structs as native.

bpf32 won't help, since even 64-bit arch will have different offsets. clang has to pick up native kernel autoconf.h and proceed through the same

defines to compute the same offsets as native kernel.

Longer term we're working on BTF (BPF Type Format) in both llvm and kernel tree to improve the situation.

Can you elaborate more on why having a bpf32 target won't help? To be clear I am suggesting adding a bpf32 target while still keeping the existing bpf target for 64-bit hosts.

because the difference in structs don't come purely from 32 vs 64-bitness of pointers. Layout changes depending on a ton of #defines that depend on arch too. This approach is not errorproof. Unfortunately we had issues with clang vs gcc. Like gcc had additional wrapping of some inner fields with anon struct {} and clang didn't. That changed offsets in 'struct task_struct' and a bunch of iovisor/bcc scripts broke. It's all fragile until we have proper BTF in llvm and kernel. BTF is a safe debug format. Meaning that kernel verifies its structure. There are several use cases for it. The main is to do adjustment of field offsets for the given host. So tracing bpf programs can be compiled once with -target bpf and offsets adjusted dynamically during prog load depending on the kernel.

tstellar commented 5 years ago

despite being 'unsupported' clang -target native | llc -mach=bpf is the only way we can get appropriate offsets today.

Do we still need to do it this way if the native target is x86_64?

bpf32 won't help, since even 64-bit arch will have different offsets. clang has to pick up native kernel autoconf.h and proceed through the same

defines to compute the same offsets as native kernel.

Longer term we're working on BTF (BPF Type Format) in both llvm and kernel tree to improve the situation.

Can you elaborate more on why having a bpf32 target won't help? To be clear I am suggesting adding a bpf32 target while still keeping the existing bpf target for 64-bit hosts.

4ast commented 5 years ago

despite being 'unsupported' clang -target native | llc -mach=bpf is the only way we can get appropriate offsets today. bpf32 won't help, since even 64-bit arch will have different offsets. clang has to pick up native kernel autoconf.h and proceed through the same #defines to compute the same offsets as native kernel. Longer term we're working on BTF (BPF Type Format) in both llvm and kernel tree to improve the situation.

tstellar commented 5 years ago

You can't use -target i386 when compling code for BPF. You have to use -target bpf. Also, you should not be piping the output of clang into llc, you can use clang to compile the C code directly to an object file.

I am sorry if I'm not making sense. I constructed the pipe based on the makefiles from the Linux Kernel BPF examples which, IIRC, too used an x86 target for the LLVM assembly (memory could serve me wrong though, I am on vacation).

Yes, I've seen those Makefiles, and that is not a supported way to use clang to compile code.

On the Cilium projects wiki, they write (Or did write?) that for trace BPF programs which use structs from Linux Kernel Headers, one should first compile with clang to LLVM assembly with the HOST-target (i386), because BPF characteristics are different which yields to incompatible layouts when accessing a running 32bit Linux kernels datastructures. One should then pass the assembly to llc whichs, as was my understanding/interpretation, zeroextends the 32bit accesses.

This approach may work some of the time, but as I mentioned above, generating IR for one target and then compiling it to machine code on another target is not supported by clang/llvm. This is especially not supported when the two targets have different pointer sizes.

Is this approach not anymore the correct thing to do? How else can I make my C/BPF scripts work on 32bit Linux?

The correct fix here would be to add a bpf32 target to clang/LLVM. I am not a BPF developer, so you may want to contact them directly to ask about work-arounds, but from my perspective, for a robust work-around, you will need to do something at the C-level (like maybe copy 32-bit structs from the kernel headers into the 64-bit layout expected by bpf) and also make sure that you compile with -target bpf.

be579e8c-b74b-475b-a126-3b8cefdd833c commented 5 years ago

You can't use -target i386 when compling code for BPF. You have to use -target bpf. Also, you should not be piping the output of clang into llc, you can use clang to compile the C code directly to an object file.

I am sorry if I'm not making sense. I constructed the pipe based on the makefiles from the Linux Kernel BPF examples which, IIRC, too used an x86 target for the LLVM assembly (memory could serve me wrong though, I am on vacation).

On the Cilium projects wiki, they write (Or did write?) that for trace BPF programs which use structs from Linux Kernel Headers, one should first compile with clang to LLVM assembly with the HOST-target (i386), because BPF characteristics are different which yields to incompatible layouts when accessing a running 32bit Linux kernels datastructures. One should then pass the assembly to llc whichs, as was my understanding/interpretation, zeroextends the 32bit accesses.

Is this approach not anymore the correct thing to do? How else can I make my C/BPF scripts work on 32bit Linux?

tstellar commented 5 years ago

You can't use -target i386 when compling code for BPF. You have to use -target bpf. Also, you should not be piping the output of clang into llc, you can use clang to compile the C code directly to an object file.

llvmbot commented 3 days ago

@llvm/issue-subscribers-clang-codegen

Author: None (be579e8c-b74b-475b-a126-3b8cefdd833c)

| | | | --- | --- | | Bugzilla Link | [37606](https://llvm.org/bz37606) | | Version | trunk | | OS | Linux | | Attachments | [Using pointers](https://user-images.githubusercontent.com/92601317/143757554-f9d9ed9a-b5c8-4389-a503-6fb689aa773a.gz) | | CC | @4ast,@tstellar,@yonghong-song | ## Extended Description LLC mistranslates the "Using pointers" attachment example, derived from linux-kernel-4.9/samples/bpf/ 0: 61 12 18 00 00 00 00 00 r2 = *(u32 *)(r1 + 24) 1: 61 13 1c 00 00 00 00 00 r3 = *(u32 *)(r1 + 28) 2: 67 03 00 00 20 00 00 00 r3 <<= 32 3: 4f 23 00 00 00 00 00 00 r3 |= r2 4: 07 03 00 00 20 00 00 00 r3 += 32 5: bf a1 00 00 00 00 00 00 r1 = r10 6: 07 01 00 00 f8 ff ff ff r1 += -8 7: b7 02 00 00 04 00 00 00 r2 = 4 8: 85 00 00 00 04 00 00 00 call 4 Inspection by "pahole" shows that the load-addresses used in the generated BPF assembly assume the layout of 4.9.0-0.bpo.6-amd64 (8byte pointers, 'dev' is at offset dec 32) rather than 4.9.0-0.bpo.6-686-pae (32byte pointers, 'dev' at offset dec 20), as one would expect, because the LLVM-IR was generated by Clang with "-target i386" and specifies the relevant data sizes as 4 byte rather than 8 byte in the datalayout string The workaround is more than ugly. Currently it consists of storing pointers as u32 on the BPF stack and accessing members using "offsetof", which apparently gets correctly resolved at an earlier stage by clang. #define _MEMBER(TYPE,P,M) (*((typeof(((TYPE*)0)->M)*) (((char*)(P)) + offsetof(TYPE, M)))) /* ... */ dev = (u32)_(_MEMBER(struct sk_buff,skb,dev))