On 6/5/25 5:25 PM, Alexei Starovoitov wrote:
On Thu, Jun 5, 2025 at 4:40 PM Ihor Solodrai <ihor.solodrai@xxxxxxxxx> wrote:
On 6/5/25 10:00 AM, Alexei Starovoitov wrote:
On Thu, Jun 5, 2025 at 9:42 AM Ihor Solodrai <ihor.solodrai@xxxxxxxxx> wrote:
On 6/5/25 9:24 AM, Andrii Nakryiko wrote:
On Wed, Jun 4, 2025 at 3:28 PM Ihor Solodrai <isolodrai@xxxxxxxx> wrote:
[...]
+SEC("socket")
+int map_ptr_is_never_null(void *ctx)
+{
+ struct bpf_map *maps[2] = { 0 };
+ struct bpf_map *map = NULL;
+ int __attribute__((aligned(8))) key = 0;
aligned(8) makes any difference?
Yes. If not aligned (explicitly or by accident), verification fails
with "math between fp pointer and register with unbounded min value is
not allowed" at maps[key]. See the log below.
It's not clear to me why "aligned" changes code gen,
but let's not rely on it.
Try 'unsigned int key' instead.
Also note that &key part pessimizes the code.
Do for (...; i < 2; i++) {u32 key = i; &key }
instead.
I've tried changing the test like this:
@@ -144,12 +144,12 @@ int map_ptr_is_never_null(void *ctx)
{
struct bpf_map *maps[2] = { 0 };
struct bpf_map *map = NULL;
- int __attribute__((aligned(8))) key = 0;
- for (key = 0; key < 2; key++) {
+ for (int i = 0; i < 2; i++) {
+ __u32 key = i;
map = bpf_map_lookup_elem(&map_in_map, &key);
if (map)
- maps[key] = map;
+ maps[i] = map;
else
return 0;
}
This version passes verification independent of the first patch being
applied, which kinda defeats the purpose of the test. Verifier log
below:
0: R1=ctx() R10=fp0
; int map_ptr_is_never_null(void *ctx) @ verifier_map_in_map.c:143
0: (b4) w1 = 0 ; R1_w=0
; __u32 key = i; @ verifier_map_in_map.c:149
1: (63) *(u32 *)(r10 -4) = r1
mark_precise: frame0: last_idx 1 first_idx 0 subseq_idx -1
mark_precise: frame0: regs=r1 stack= before 0: (b4) w1 = 0
2: R1_w=0 R10=fp0 fp-8=0000????
2: (bf) r2 = r10 ; R2_w=fp0 R10=fp0
3: (07) r2 += -4 ; R2_w=fp-4
; map = bpf_map_lookup_elem(&map_in_map, &key); @
verifier_map_in_map.c:150
4: (18) r1 = 0xff302cd6802e0a00 ;
R1_w=map_ptr(map=map_in_map,ks=4,vs=4)
6: (85) call bpf_map_lookup_elem#1 ;
R0_w=map_value_or_null(id=1,map=map_in_map,ks=4,vs=4)
; if (map) @ verifier_map_in_map.c:151
7: (15) if r0 == 0x0 goto pc+7 ; R0_w=map_ptr(ks=4,vs=4)
8: (b4) w1 = 1 ; R1_w=1
; __u32 key = i; @ verifier_map_in_map.c:149
9: (63) *(u32 *)(r10 -4) = r1 ; R1_w=1 R10=fp0 fp-8=mmmm????
10: (bf) r2 = r10 ; R2_w=fp0 R10=fp0
11: (07) r2 += -4 ; R2_w=fp-4
; map = bpf_map_lookup_elem(&map_in_map, &key); @
verifier_map_in_map.c:150
12: (18) r1 = 0xff302cd6802e0a00 ;
R1_w=map_ptr(map=map_in_map,ks=4,vs=4)
14: (85) call bpf_map_lookup_elem#1 ;
R0=map_value_or_null(id=2,map=map_in_map,ks=4,vs=4)
; } @ verifier_map_in_map.c:164
15: (b4) w0 = 0 ; R0_w=0
16: (95) exit
because the compiler removed 'if (!maps[0])' check?
Make maps volatile then.
Making maps and/or map volatile didn't help.
It's not clear to me what the point of the 'for' loop is.
Just one bpf_map_lookup_elem() from map_in_map should do ?
No. Using an array and loop was my attempt to put map_ptr on the
stack. But apparently this was not the reason it happened in the v1 of
the test.
If map[i] is changed back to map[key] like this:
for (int i = 0; i < 2; i++) {
__u32 key = i;
map = bpf_map_lookup_elem(&map_in_map, &key);
if (map)
maps[key] = map; /* change here */
else
return 0;
}
Verification fails with "invalid unbounded variable-offset write to
stack R2"
as it should, because both the compiler and the verifier
see that 'key' is unbounded in maps[key] access.
__u32 __attribute__((aligned(8))) key = i;
but that puts us back to square one.
It appears that alignment becomes a problem if the variable is used as
array index and also it's address is passed to a helper.
I bet this alignment "workaround" is fragile.
A different version of clang or gcc-bpf will change layout.
I agree, it's fragile.
After I fought compiler/verifier for a while I gave up and wrote a
test in asm:
SEC("socket")
__description("map_ptr is never null")
__success
__naked void map_ptr_is_never_null(void)
{
asm volatile (" \
r1 = 0; \
*(u32*)(r10 - 4) = r1; \
r2 = r10; \
r2 += -4; \
r1 = %[map_in_map] ll; \
call %[bpf_map_lookup_elem]; \
if r0 != 0 goto l0_%=; \
exit; \
l0_%=: *(u64 *)(r10 -16) = r0; \
r1 = *(u64 *)(r10 -16); \
if r1 == 0 goto l1_%=; \
exit; \
l1_%=: r10 = 42; \
exit; \
" :
: __imm(bpf_map_lookup_elem),
__imm_addr(map_in_map)
: __clobber_all);
}
What must happen to reproduce the situation is: map_ptr gets on a
stack, and then loaded and compared to 0.
It looks like I accidentally forced map_ptr on the stack by using
`key` both for map lookup and array access, which triggers those
alignment problems. Without that I wasn't able to figure out simple C
code that would produce bpf with map_ptr on the stack (besides the
other test, with rbs).
I guess I should've written an asm test right away.
