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

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

On 6/5/25 10:17 AM, Ihor Solodrai wrote:
> On 6/5/25 9:08 AM, Alexei Starovoitov wrote:
>> On Wed, Jun 4, 2025 at 8:04 PM Ihor Solodrai 
>> <ihor.solodrai@linux.dev> wrote:
>>>
>>> On 6/4/25 3:41 PM, Alexei Starovoitov wrote:
>>>> On Wed, Jun 4, 2025 at 3:28 PM Ihor Solodrai <isolodrai@meta.com> 
>>>> 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..c4a48b57e167 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 == 0xdeadbeef goto l0_%=;         \
>>>>
>>>> I bet this doesn't fit into imm32 either.
>>>> It should fit into _signed_ imm32.
>>>
>>> Apparently it's fine both for gcc and clang:
>>> https://github.com/kernel-patches/bpf/actions/runs/15454151804
>>
>> Both compilers are buggy then.
>>
>>> I guess the value from inline asm is just put into IMM bytes as
>>> is. llvm-objdump is exactly the same, although the value is pretty
>>> printed as negative:
>>>
>>> 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
>>
>> It's 64-bit 0xFFFFffffdeadbeef
>> Not the same as 0xdeadbeef
>
> I am not sure what the issue is, would appreciate an explanation.
>
> Inline asm contains a 32bit literal (without a sign). Compiler takes
> this literal as is and puts it into imm field of the instruction,
> which is also 32bit. The instruction is valid and this value _means_
> signed integer, in particular for the verifier.
>
> Are you saying that compiler should check the sign of the literal and
> verify it's in signed 32bit range? In other words if you want
> 0xdeadbeef bytes in the imm, you must write -0x21524111 in the asm?
>
> AFAIU it'd be different from C then, because you can write:
>
>    int k = 0xdeadbeef;
>    printf("%d\n", k); // prints -559038737
>
> and it's fine.
>
> Looking at Yonghong's llvm pr [1], it will not error for 0xdeadbeef
> because it's less than UINT_MAX:
>
>     if (MO.isImm()) {
>         int64_t Imm = MO.getImm();
>         if (MI.getOpcode() != BPF::LD_imm64 && (Imm < INT_MIN || Imm > 
> UINT_MAX))
>           Ctx.reportError(MI.getLoc(),
>                           "immediate out of range, shall fit in 32 
> bits");
>         return static_cast<unsigned>(Imm);
>       }
>
> [1] https://github.com/llvm/llvm-project/pull/142989
>
>
If we have C code like
   if (var == 0xdeadbeef) { ... }

The compiler will actually convert 'var == imm' to 'rX == rY' and
rY will have content of 0xdeadbeef. This will happen during IR lowering
from middle end to machine instructions.

The tricky thing is inline asm. I am debating myself whether we
should align with GCC or not to allow 'rX == 0xdeadbeef' in inline asm.
in llvm the inline asm code is processed at MC level (after all 
optimizations).
Ultimately I aligned with GCC for compatibility. My first response to this
thread is to only allow in range or INT_MIN and INT_MAX.

So the question is that we treat inline asm as the pure encoding
or it should have other semantics.