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 11:11 AM, Alexei Starovoitov wrote:
> On Thu, Jun 5, 2025 at 10:42 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>>
>> 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.
> Not quite. It's signed imm32 in _runtime_.
>
>>> 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.
> ... and the compiler will use ld_imm64 insn to store 0xdeadbeef in rY.
>
>> 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.
> I think both compilers should error (or warn) when imm32 doesn't fit
> into int_min/max, because the asm code for
> if r1 == 0xdeadbeef goto l0_%=;
> will not do what the author expects.

if we intend to use 'int' range instead then we will have more cases like below.
For example,

store with imm:
    int foo(long *a, long *b) {
       *a = 0xabababab;
       *b = 0x76543210;
       return 0;
    }

In this example, using an inline asm to do
    *(u64 *)(r1 + 0) = 0xabababab
will not be what user expected to get.

The same issue for conditional like 'long a; if (a > 0xabababab) ...', or
alu64 operations like 'long a; if (a & 0xabababab) ...'.

I can expand checking in llvm for the these patterns.