Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> writes:
> On Wed, May 21, 2025 at 1:35 PM Yonghong Song <yonghong.song@xxxxxxxxx> 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.
[...]
