On 4/1/26 11:28 AM, David Faust wrote:
attach_probe. The manually reduced test (attached here) showed the
failing case (is essentially beefier version of the test in this patch,
but same in spirit)
Pasting here the attached test for reference:
/* Extracted from BPF selftests attach_probe: verifier failure.
Return value of first call is reused as argument of second.
It either needs to be explicitly truncated to s32 or w regs need
to be used. */
/* { dg-do compile } */
/* { dg-options "-O2 -mcpu=v4" } */
extern int bpf_copy_from_user_str(int dst__sz);
_Bool verify_sleepable_user_copy_str(void)
{
int ret;
char data_short_pad[4];
ret = bpf_copy_from_user_str(sizeof(data_short_pad));
if (ret != 4)
return false;
/* ret above is guaranteed to be 4, same as sizeof. */
ret = bpf_copy_from_user_str(sizeof(data_short_pad));
if (ret != 4)
return false;
return true;
}
/* Don't generate 64-bit mov for arg, do 32-bit zero extended mov. */
/* { dg-final { scan-assembler-not {r1 = r0} } } */
/* { dg-final { scan-assembler-not {r1 = \(s32\) r0} } } */
/* { dg-final { scan-assembler-times {w1 = w0} 1 } } */
I'm confused by the comment at the top.
First:
Return value of first call is reused as argument of second.
Where? The only calls I see are identical:
ret = bpf_copy_from_user_str (sizeof (data_short_pad));
The return value is not reused as an argument.
I meant semantically. At the time of second call, @ret from first call
is guaranteed to be 4 by virtue of control flow.
And sizeof (data_short_pad) happens to be 4 as well, hence the reason
gcc:cse1 decides to reuse the reg/value for ret as arg for second call.
Secondly:
It either needs to be explicitly truncated to s32 or w regs need
to be used. */
Why? Assuming the value were actually being reused, we would be
taking an int return value and passing it as an int argument to a
function taking an int. There is no conversion needed.
Correct they are all int, so yes no conversion required, but we are
forgetting that big boss "verifier" is watching everything and causes
issues, see below...
So with the ABI hooks and everything in place, what we were seeing was
following RTL stream
In the RTL below it's clear that the values are all being treated
in SImode:
(call_insn 7 6 8 2 (set (reg:SI 0 %r0)
So the call return value is SImode.
(call (mem:DI (symbol_ref:DI ("bpf_copy_from_user_str") [flags
0x41] <function_decl 0x7f10c1547300 bpf_copy_from_user_str>) [0
bpf_copy_from_user_str S8 A64])
(const_int 0 [0])))
(expr_list:REG_CALL_DECL (symbol_ref:DI ("bpf_copy_from_user_str")
[flags 0x41] <function_decl 0x7f10c1547300 bpf_copy_from_user_str>)
(nil))
(expr_list:SI (use (reg:SI 1 %r1))
(use (reg:SI 1 %r1)) : the arg is passed SI mode
(nil)))
(insn 8 7 9 2 (set (reg/v:SI 19 [ ret ])
(reg:SI 0 %r0))
Return value copied to pseudo r19 in SImode to be used for
the comparison.
Yeah everything is SI mode and its fine from gcc's codegen point of view.
(nil))
(jump_insn 9 8 10 2 (set (pc)
(if_then_else (ne (reg/v:SI 19 [ ret ])
(const_int 4 [0x4]))
(label_ref:DI 33)
(pc))) {*branch_on_si}
-> 33)
(insn 11 10 12 4 (set (reg:SI 1 %r1)
(const_int 4 [0x4]))
(nil))
cse1 equivalence logic deduces that reg 19 = 4 = SI, guaranteed by
control flow, so we end up with
Note that r19 is SImode a priori, to match the mode of the value from %r0.
It is not SI mode as a result of cse1 determining that it holds 4.
Right.
(insn 11 10 12 3 (set (reg:SI 1 %r1)
(reg/v:SI 19 [ ret ])) {*movsi}
(expr_list:REG_EQUAL (const_int 4 [0x4])
(nil)))
And this carries all the way to the end, which if generates rN is would
be problematic and was the reason for this change.
And again, what precisely is problematic about generating rN = rM
for this set?
Verifier trips up, #12 attach _probe
(tools/testing/selftests/bpf/progs/test_attach_probe.c)
If %r1 is subsequently used as the int argument to a function call,
then it is valid as is.
If it would be passed as a 'unsigned long' or something else requiring
a conversion then we would rather have e.g. a zero_extend:DI to do that,
not a simple set.
The rN based move copies the full 64-bits, but the subsequent arg is
int: verifier wants to make sure top bits are cleared, which is not
guaranteed given the current codegen.
Full log below, relevant snippets extracted:
22: (85) call bpf_copy_from_user_str#85440 ; R0=scalar()
...
24: (bf) r7 = r0 ; R0=scalar(id=1) R7=scalar(id=1)
...
29: (85) call bpf_strncmp#182 ; R0=scalar()
30: (55) if r0 != 0x0 goto pc+1 ; R0=0
31: (16) if w7 == 0x4 goto pc+2 34: R0=0
R6=map_value(map=test_att.bss,ks=4,vs=56)
R7=scalar(id=1,smin=0x8000000000000004,smax=0x7fffffff00000004,umin=smin32=umin32=4,umax=0xffffffff00000004,smax32=umax32=4,var_off=(0x4;
0xffffffff00000000)) R10=fp0 fp-16=???????m fp-24=mmmmmmmm fp-88=????mmmm
...
36: (bf) r2 = r7 ;
R2=scalar(id=1,smin=0x8000000000000004,smax=0x7fffffff00000004,umin=smin32=umin32=4,umax=0xffffffff00000004,smax32=umax32=4,var_off=(0x4;
0xffffffff00000000))
R7=scalar(id=1,smin=0x8000000000000004,smax=0x7fffffff00000004,umin=smin32=umin32=4,umax=0xffffffff00000004,smax32=umax32=4,var_off=(0x4;
0xffffffff00000000))
...
39: (85) call bpf_copy_from_user_str#85440
R2 min value is negative, either use unsigned or 'var &= const'
Now, thinking about it, part of the problem could also be #31 using w7
reg for comparison. If it had used the rN reg too, maybe verifier would
be confident about the top bits too and won't trip up. But that again
would point to impedance mismatch between movsi (only using rN regs) and
branch_on_si, which would use wN regs.
I did a quick hack to have branch_on_ generate rN forms (there's no way
this can be productized, since we might have even more issues with
verifier dealing with 64-bits now.
- case EQ: return "{jeq<msuffix>\t%0,%1,%2|if %w0 == %w1 goto %2}"; break;
+ case EQ: return "{jeq<msuffix>\t%0,%1,%2|if %0 == %1 goto %2}"; break;
And now it does generate rN for branch n compare
verify_sleepable_user_copy_str:
r1 = 4
call bpf_copy_from_user_str
if r0 == 4 goto .L5
r0 = 0
exit
.L5:
r1 = r0
call bpf_copy_from_user_str
w0 ^= 4
w0 = w0
r0 += -1
r0 >>= 63
exit
But this is besides the point, just a curiosity, since I'm pretty sure
verifier will hit way more false positives with rN regs in branch and
compare.
Full attach probe log (w/o the above hack) and code to generate it on my
gh portal [1]
[1] [email protected]:vineetgarc/gcc.git #bpf/promotion-n-abi
Thx,
-Vineet
--- #12 attach_probe:
libbpf: elf: skipping unrecognized data section(17) .comment
test_attach_probe:PASS:skel_open 0 nsec
libbpf: prog 'handle_uprobe_byname3_sleepable': BPF program load failed: -EACCES
libbpf: prog 'handle_uprobe_byname3_sleepable': -- BEGIN PROG LOAD LOG --
0: R1=ctx() R10=fp0
; @ vmlinux.h:49637
0: (b7) r2 = 9 ; R2=9
; @ test_attach_probe.c:92
1: (18) r6 = 0xffa00000017cd000 ;
R6=map_value(map=test_att.bss,ks=4,vs=56)
3: (bf) r1 = r10 ; R1=fp0 R10=fp0
4: (79) r3 = *(u64 *)(r6 +0) ; R3=scalar()
R6=map_value(map=test_att.bss,ks=4,vs=56)
5: (07) r1 += -24 ; R1=fp-24
6: (85) call bpf_copy_from_user#148 ; R0=scalar() fp-16=???????m
fp-24=mmmmmmmm
; @ test_attach_probe.c:93
7: (bf) r1 = r10 ; R1=fp0 R10=fp0
8: (18) r3 = 0xff11000113510930 ;
R3=map_value(map=.rodata.str1.1,ks=4,vs=10)
10: (b7) r2 = 9 ; R2=9
11: (07) r1 += -24 ; R1=fp-24
12: (85) call bpf_strncmp#182 ; R0=scalar()
; @ test_attach_probe.c:147
13: (55) if r0 != 0x0 goto pc+3 ; R0=0
; @ test_attach_probe.c:148
14: (18) r0 = 0xffa00000017cd01c ;
R0=map_value(map=test_att.bss,ks=4,vs=56,imm=28)
16: (62) *(u32 *)(r0 +0) = 9 ;
R0=map_value(map=test_att.bss,ks=4,vs=56,imm=28)
; @ test_attach_probe.c:105
17: (79) r3 = *(u64 *)(r6 +0) ; R3=scalar()
R6=map_value(map=test_att.bss,ks=4,vs=56)
18: (b7) r2 = 4 ; R2=4
19: (b7) r4 = 0 ; R4=0
20: (bf) r1 = r10 ; R1=fp0 R10=fp0
21: (07) r1 += -88 ; R1=fp-88
22: (85) call bpf_copy_from_user_str#85440 ; R0=scalar()
; @ test_attach_probe.c:107
23: (bf) r1 = r10 ; R1=fp0 R10=fp0
; @ test_attach_probe.c:105
24: (bf) r7 = r0 ; R0=scalar(id=1) R7=scalar(id=1)
; @ test_attach_probe.c:107
25: (18) r3 = 0xff11000113514d30 ;
R3=map_value(map=test_att.rodata,ks=4,vs=16)
27: (b7) r2 = 4 ; R2=4
28: (07) r1 += -88 ; R1=fp-88
29: (85) call bpf_strncmp#182 ; R0=scalar()
30: (55) if r0 != 0x0 goto pc+1 ; R0=0
31: (16) if w7 == 0x4 goto pc+2 34: R0=0
R6=map_value(map=test_att.bss,ks=4,vs=56)
R7=scalar(id=1,smin=0x8000000000000004,smax=0x7fffffff00000004,umin=smin32=umin32=4,umax=0xffffffff00000004,smax32=umax32=4,var_off=(0x4;
0xffffffff00000000)) R10=fp0 fp-16=???????m fp-24=mmmmmmmm fp-88=????mmmm
; @ test_attach_probe.c:110
34: (b7) r4 = 1 ; R4=1
35: (79) r3 = *(u64 *)(r6 +0) ; R3=scalar()
R6=map_value(map=test_att.bss,ks=4,vs=56)
36: (bf) r2 = r7 ;
R2=scalar(id=1,smin=0x8000000000000004,smax=0x7fffffff00000004,umin=smin32=umin32=4,umax=0xffffffff00000004,smax32=umax32=4,var_off=(0x4;
0xffffffff00000000))
R7=scalar(id=1,smin=0x8000000000000004,smax=0x7fffffff00000004,umin=smin32=umin32=4,umax=0xffffffff00000004,smax32=umax32=4,var_off=(0x4;
0xffffffff00000000))
37: (bf) r1 = r10 ; R1=fp0 R10=fp0
38: (07) r1 += -80 ; R1=fp-80
39: (85) call bpf_copy_from_user_str#85440
R2 min value is negative, either use unsigned or 'var &= const'
arg#0 arg#1 memory, len pair leads to invalid memory access
processed 36 insns (limit 1000000) max_states_per_insn 0 total_states 3
peak_states 3 mark_read 0
-- END PROG LOAD LOG --
libbpf: prog 'handle_uprobe_byname3_sleepable': failed to load: -EACCES
libbpf: failed to load object 'test_attach_probe'
libbpf: failed to load BPF skeleton 'test_attach_probe': -EACCES
test_attach_probe:FAIL:skel_load unexpected error: -13 (errno 13)
test_attach_probe:PASS:uprobe_ref_ctr_cleanup 0 nsec
#12 attach_probe:FAIL
