Re: [PATCH bpf-next v3 1/2] bpf: Do not include stack ptr register in precision backtracking bookkeeping

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

On 5/21/25 3:49 PM, Eduard Zingerman wrote:
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
>> On Wed, May 21, 2025 at 1:35 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>>
>>>
>>> On 5/21/25 11:55 AM, Eduard Zingerman wrote:
>>>> [...]
>>>>
>>>>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>>>>> index 78c97e12ea4e..e73a910e4ece 100644
>>>>> --- a/include/linux/bpf_verifier.h
>>>>> +++ b/include/linux/bpf_verifier.h
>>>>> @@ -357,6 +357,10 @@ enum {
>>>>>        INSN_F_SPI_SHIFT = 3, /* shifted 3 bits to the left */
>>>>>          INSN_F_STACK_ACCESS = BIT(9), /* we need 10 bits total */
>>>>> +
>>>>> +    INSN_F_DST_REG_STACK = BIT(10), /* dst_reg is PTR_TO_STACK */
>>>>> +    INSN_F_SRC_REG_STACK = BIT(11), /* src_reg is PTR_TO_STACK */
>>>> INSN_F_STACK_ACCESS can be inferred from INSN_F_DST_REG_STACK
>>>> and INSN_F_SRC_REG_STACK if insn_stack_access_flags() is adjusted
>>>> to track these flags instead. So, can be one less flag/bit.
>>> You are correct, we could have BIT(9) for both INSN_F_STACK_ACCESS and INSN_F_DST_REG_STACK,
>>> and BIT(10) for INSN_F_SRC_REG_STACK. But it makes code a little bit
>>> complicated. I am okay with this if Andrii also thinks it is
>>> worthwhile to do this.
>> I originally wanted to replace INSN_F_STACK_ACCESS with either
>> INSN_F_DST_REG_STACK or INSN_F_SRC_REG_STACK depending on STX/LDX. But
>> then I realized that INSN_F_STACK_ACCESS implies the use of that spi
>> mask, while xxx_REG_STACK doesn't. So it might be a bit simpler if we
>> keep them distinct, and for LDX/STX we'll set either just
>> INSN_F_STACK_ACCESS or INSN_F_STACK_ACCESS|INSN_F_xxx_REG_STACK
>> (whichever makes most sense).
>>
>> We have enough bits, so I'd probably use two new bits and keep the
>> existing STACK_ACCESS one as is. Unless Eduard thinks that this setup
>> is actually more confusing?
> Idk, I don't see much difference between these flags for LDX/STX or JMP.
> In both cases it's a signal PTR_TO_STACK on the left / PTR_TO_STACK on
> the right. So, having two ways to express the same thing seems a bit
> confusing to me.
>
> Defer to your best judgement.

Thanks for all discusions. I would like to use two new bits for now.