On 4/1/26 17:02, Vineet Gupta wrote:
> 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.
Ahhhhhh, so it is a verifier issue! Not a code correctness issue.
OK, that makes more sense.
>
> 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.
Hmm, looking at these verifier logs (thanks for providing), I think
the issue may stem from the bpf_copy_from_user_str call.
Ultimately the return value propagates r0 -> r7 -> r2.
BUT, I am confused. This test passes on my system compiled with
gcc trunk. After the call, we emit a movs32 when moving the return value
to a different register, which is enough for the verifier to know the
value is restricted to signed integer range.
Are you seeing this with trunk or with the PROMOTE_MODE or some other
change applied?
>
> 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
>
>
>
