On Wed, May 21, 2025 at 3:50 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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.
The difference is that in one case we need to know (and keep track of)
stack frame index, while in others we just record the fact that
dst/src reg is a stack pointer. I'd probably start with setting both
ISNS_F_STACK_ACCESS and INSNS_F_{SRC,DST}_REG_STACK for LDX/STX
instruction for now, knowing that we can squeeze out that one extra
bit, if absolutely necessary.
>
> Defer to your best judgement.
>
> [...]
