Re: [PATCH bpf-next v2 2/3] selftests/bpf: add cmp_map_pointer_with_const test

[Yonghong Song <yonghong.song@linux.dev>]

On 6/4/25 1:58 PM, Ihor Solodrai wrote:
> On 6/4/25 1:42 PM, Yonghong Song wrote:
>>
>>
>> On 6/4/25 9:44 AM, Ihor Solodrai wrote:
>>> On 6/3/25 5:37 PM, Ihor Solodrai wrote:
>>>> Add a test for CONST_PTR_TO_MAP comparison with a non-0 constant. A
>>>> BPF program with this code must not pass verification in unpriv.
>>>>
>>>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
>>>> ---
>>>>   .../selftests/bpf/progs/verifier_unpriv.c       | 17 
>>>> +++++++++++++++++
>>>>   1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/progs/verifier_unpriv.c b/ 
>>>> tools/testing/selftests/bpf/progs/verifier_unpriv.c
>>>> index 28200f068ce5..85b41f927272 100644
>>>> --- a/tools/testing/selftests/bpf/progs/verifier_unpriv.c
>>>> +++ b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
>>>> @@ -634,6 +634,23 @@ l0_%=:    r0 = 0;                        \
>>>>       : __clobber_all);
>>>>   }
>>>>   +SEC("socket")
>>>> +__description("unpriv: cmp map pointer with const")
>>>> +__success __failure_unpriv __msg_unpriv("R1 pointer comparison 
>>>> prohibited")
>>>> +__retval(0)
>>>> +__naked void cmp_map_pointer_with_const(void)
>>>> +{
>>>> +    asm volatile ("                    \
>>>> +    r1 = 0;                        \
>>>> +    r1 = %[map_hash_8b] ll;                \
>>>> +    if r1 == 0xcafefeeddeadbeef goto l0_%=;        \
>>>
>>> GCC BPF caught (correctly) that this is not a valid instruction 
>>> because imm is supposed to be 32bit [1]:
>>>
>>>     progs/verifier_unpriv.c: Assembler messages:
>>>     progs/verifier_unpriv.c:643: Error: immediate out of range, 
>>> shall fit in 32 bits
>>>     make: *** [Makefile:751: /tmp/work/bpf/bpf/src/tools/testing/ 
>>> selftests/bpf/bpf_gcc/verifier_unpriv.bpf.o] Error 1
>>>
>>> But LLVM 20 let it compile and the test passes. I wonder whether 
>>> it's a bug in LLVM worth reporting?
>>>
>>> [1] https://github.com/kernel-patches/bpf/actions/runs/15430930573/ 
>>> job/43428666342
>>
>> This is a missed case for llvm. See:
>> https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/BPF/ 
>> MCTargetDesc/BPFMCCodeEmitter.cpp#L82-L85
>> Basically for the following code,
>>
>> unsigned BPFMCCodeEmitter::getMachineOpValue(const MCInst &MI,
>>                                               const MCOperand &MO,
>> SmallVectorImpl<MCFixup> &Fixups,
>>                                               const MCSubtargetInfo 
>> &STI) const {
>>    if (MO.isReg())
>>      return MRI.getEncodingValue(MO.getReg());
>>    if (MO.isImm())
>>      return static_cast<unsigned>(MO.getImm());
>>
>> For 'static_cast<unsigned>(MO.getImm())', MO.getImm() value is a s64, 
>> so casting to u32 should check
>> the value range and we didn't check them, hence didn't report an error.
>
> I see. Out of curiosity I looked at llvm-objdump and indeed only lower
> 32 bits are in the binary:
>
> 0000000000000320 <cmp_map_pointer_with_const>:
>      100:       b7 01 00 00 00 00 00 00 r1 = 0x0
>      101:       18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 
> 0x0 ll
>      103:       15 01 00 00 ef be ad de if r1 == -0x21524111 goto +0x0 
> <l0_11>

FYI, I just submitted a fix https://github.com/llvm/llvm-project/pull/142989
which will emit the same error messages as gcc if the value cannot fit into
32bit (32bit value will do sign extension to 64bit value).