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 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.

>
>> +    /* total 12 bits are used now. */
>>   };
>>     static_assert(INSN_F_FRAMENO_MASK + 1 >= MAX_CALL_FRAMES);
>
> [...]
>
>> @@ -4402,6 +4418,8 @@ static int backtrack_insn(struct 
>> bpf_verifier_env *env, int idx, int subseq_idx,
>>                */
>>               return 0;
>>           } else if (BPF_SRC(insn->code) == BPF_X) {
>> +            bool dreg_precise, sreg_precise;
>> +
>>               if (!bt_is_reg_set(bt, dreg) && !bt_is_reg_set(bt, sreg))
>>                   return 0;
>>               /* dreg <cond> sreg
>> @@ -4410,8 +4428,16 @@ static int backtrack_insn(struct 
>> bpf_verifier_env *env, int idx, int subseq_idx,
>>                * before it would be equally necessary to
>>                * propagate it to dreg.
>>                */
>> -            bt_set_reg(bt, dreg);
>> -            bt_set_reg(bt, sreg);
>> +            if (!hist)
>> +                return 0;
>> +            dreg_precise = !insn_dreg_stack_ptr(hist->flags);
>> +            sreg_precise = !insn_sreg_stack_ptr(hist->flags);
>> +            if (!dreg_precise && !sreg_precise)
>> +                return 0;
>> +            if (dreg_precise)
>> +                bt_set_reg(bt, dreg);
>> +            if (sreg_precise)
>> +                bt_set_reg(bt, sreg);
>
> This check can be done in a generic way at backtrack_insn() callsite:
> check which register is pointer to stack and remove it from set 
> registers.

Looks like other cases in backtrack_insn() has been handled properly,
so it may still be worthwhile to put the code here.

>
>>           } else if (BPF_SRC(insn->code) == BPF_K) {
>>                /* dreg <cond> K
>>                 * Only dreg still needs precision before
>> @@ -16397,6 +16423,29 @@ static void sync_linked_regs(struct 
>> bpf_verifier_state *vstate, struct bpf_reg_s
>>       }
>>   }
>>   +static int push_cond_jmp_history(struct bpf_verifier_env *env, 
>> struct bpf_verifier_state *state,
>> +                 struct bpf_reg_state *dst_reg, struct bpf_reg_state 
>> *src_reg,
>> +                 u64 linked_regs)
>> +{
>> +    bool dreg_stack_ptr, sreg_stack_ptr;
>> +    int insn_flags;
>> +
>> +    if (!src_reg) {
>> +        if (linked_regs)
>> +            return push_insn_history(env, state, 0, linked_regs);
>> +        return 0;
>> +    }
>
> Nit: this 'if' is not needed, src_reg is always set (it might point to 
> a fake register,
>      but in that case it is a scalar without id).
>
Here, there is a bug here. Thanks for pointing this out. I need to check
BPF_SRC(insn->code) != BPF_X instead of "!src_reg". Basically passing one
more parameter (e.g., faked_sreg) to decide whether src_reg is faked or not.

>
>> +
>> +    dreg_stack_ptr = dst_reg->type == PTR_TO_STACK;
>> +    sreg_stack_ptr = src_reg->type == PTR_TO_STACK;
>> +
>> +    if (!dreg_stack_ptr && !sreg_stack_ptr && !linked_regs)
>> +        return 0;
>> +
>> +    insn_flags = insn_reg_access_flags(dreg_stack_ptr, sreg_stack_ptr);
>> +    return push_insn_history(env, state, insn_flags, linked_regs);
>> +}
>> +
>
> [...]
>