Re: [PATCH ipsec-next v1 6/7] bpf: selftests: test_tunnel: Disable CO-RE relocations
From: Eduard Zingerman <eddyz87@gmail.com>
Date: 2023-11-27 02:02:18
Also in:
bpf, linux-kselftest, lkml
Subsystem:
bpf [general] (safe dynamic programs and tools), bpf [selftests] (test runners & infrastructure), kernel selftest framework, the rest · Maintainers:
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi, Shuah Khan, Shuah Khan, Linus Torvalds
On Sun, 2023-11-26 at 18:04 -0600, Daniel Xu wrote: [...]
quoted
Tbh I'm not sure. This test passes with preserve_static_offset because it suppresses preserve_access_index. In general clang translates bitfield access to a set of IR statements like: C: struct foo { unsigned _; unsigned a:1; ... }; ... foo->a ... IR: %a = getelementptr inbounds %struct.foo, ptr %0, i32 0, i32 1 %bf.load = load i8, ptr %a, align 4 %bf.clear = and i8 %bf.load, 1 %bf.cast = zext i8 %bf.clear to i32 With preserve_static_offset the getelementptr+load are replaced by a single statement which is preserved as-is till code generation, thus load with align 4 is preserved. On the other hand, I'm not sure that clang guarantees that load or stores used for bitfield access would be always aligned according to verifier expectations. I think we should check if there are some clang knobs that prevent generation of unaligned memory access. I'll take a look.Is there a reason to prefer fixing in compiler? I'm not opposed to it, but the downside to compiler fix is it takes years to propagate and sprinkles ifdefs into the code. Would it be possible to have an analogue of BPF_CORE_READ_BITFIELD()?
Well, the contraption below passes verification, tunnel selftest
appears to work. I might have messed up some shifts in the macro, though.
Still, if clang would peek unlucky BYTE_{OFFSET,SIZE} for a particular
field access might be unaligned.
---
diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
index 3065a716544d..41cd913ac7ff 100644
--- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c@@ -9,6 +9,7 @@ #include "vmlinux.h" #include <bpf/bpf_helpers.h> #include <bpf/bpf_endian.h> +#include <bpf/bpf_core_read.h> #include "bpf_kfuncs.h" #include "bpf_tracing_net.h"
@@ -144,6 +145,38 @@ int ip6gretap_get_tunnel(struct __sk_buff *skb) return TC_ACT_OK; } +#define BPF_CORE_WRITE_BITFIELD(s, field, new_val) ({ \ + void *p = (void *)s + __CORE_RELO(s, field, BYTE_OFFSET); \ + unsigned byte_size = __CORE_RELO(s, field, BYTE_SIZE); \ + unsigned lshift = __CORE_RELO(s, field, LSHIFT_U64); \ + unsigned rshift = __CORE_RELO(s, field, RSHIFT_U64); \ + unsigned bit_size = (rshift - lshift); \ + unsigned long long nval, val, hi, lo; \ + \ + asm volatile("" : "=r"(p) : "0"(p)); \ + \ + switch (byte_size) { \ + case 1: val = *(unsigned char *)p; break; \ + case 2: val = *(unsigned short *)p; break; \ + case 4: val = *(unsigned int *)p; break; \ + case 8: val = *(unsigned long long *)p; break; \ + } \ + hi = val >> (bit_size + rshift); \ + hi <<= bit_size + rshift; \ + lo = val << (bit_size + lshift); \ + lo >>= bit_size + lshift; \ + nval = new_val; \ + nval <<= lshift; \ + nval >>= rshift; \ + val = hi | nval | lo; \ + switch (byte_size) { \ + case 1: *(unsigned char *)p = val; break; \ + case 2: *(unsigned short *)p = val; break; \ + case 4: *(unsigned int *)p = val; break; \ + case 8: *(unsigned long long *)p = val; break; \ + } \ +}) + SEC("tc") int erspan_set_tunnel(struct __sk_buff *skb) {
@@ -173,9 +206,9 @@ int erspan_set_tunnel(struct __sk_buff *skb) __u8 hwid = 7; md.version = 2; - md.u.md2.dir = direction; - md.u.md2.hwid = hwid & 0xf; - md.u.md2.hwid_upper = (hwid >> 4) & 0x3; + BPF_CORE_WRITE_BITFIELD(&md.u.md2, dir, direction); + BPF_CORE_WRITE_BITFIELD(&md.u.md2, hwid, (hwid & 0xf)); + BPF_CORE_WRITE_BITFIELD(&md.u.md2, hwid_upper, (hwid >> 4) & 0x3); #endif ret = bpf_skb_set_tunnel_opt(skb, &md, sizeof(md));
@@ -214,8 +247,9 @@ int erspan_get_tunnel(struct __sk_buff *skb) bpf_printk("\tindex %x\n", index); #else bpf_printk("\tdirection %d hwid %x timestamp %u\n", - md.u.md2.dir, - (md.u.md2.hwid_upper << 4) + md.u.md2.hwid, + BPF_CORE_READ_BITFIELD(&md.u.md2, dir), + (BPF_CORE_READ_BITFIELD(&md.u.md2, hwid_upper) << 4) + + BPF_CORE_READ_BITFIELD(&md.u.md2, hwid), bpf_ntohl(md.u.md2.timestamp)); #endif
@@ -252,9 +286,9 @@ int ip4ip6erspan_set_tunnel(struct __sk_buff *skb) __u8 hwid = 17; md.version = 2; - md.u.md2.dir = direction; - md.u.md2.hwid = hwid & 0xf; - md.u.md2.hwid_upper = (hwid >> 4) & 0x3; + BPF_CORE_WRITE_BITFIELD(&md.u.md2, dir, direction); + BPF_CORE_WRITE_BITFIELD(&md.u.md2, hwid, (hwid & 0xf)); + BPF_CORE_WRITE_BITFIELD(&md.u.md2, hwid_upper, (hwid >> 4) & 0x3); #endif ret = bpf_skb_set_tunnel_opt(skb, &md, sizeof(md));
@@ -294,8 +328,9 @@ int ip4ip6erspan_get_tunnel(struct __sk_buff *skb) bpf_printk("\tindex %x\n", index); #else bpf_printk("\tdirection %d hwid %x timestamp %u\n", - md.u.md2.dir, - (md.u.md2.hwid_upper << 4) + md.u.md2.hwid, + BPF_CORE_READ_BITFIELD(&md.u.md2, dir), + (BPF_CORE_READ_BITFIELD(&md.u.md2, hwid_upper) << 4) + + BPF_CORE_READ_BITFIELD(&md.u.md2, hwid), bpf_ntohl(md.u.md2.timestamp)); #endif