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 1:58 PM, Eduard Zingerman wrote:
> Yonghong Song <yonghong.song@linux.dev> writes:
>
> [...]
>
>>>> @@ -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.
> I don't think any checks are needed.
> Fake register is always scalar and it cannot be collected as a linked register.
> So it won't end up in the instruction history flags.

Let us say that we remove the code

+    if (!src_reg) {
+        if (linked_regs)
+            return push_insn_history(env, state, 0, linked_regs);
+        return 0;
+    }

The code should still work. But we might end up with more unnecessary
jump history entries. For example,
    dreg->type == PTR_TO_STACK, sreg is faked (i.e., BPF_SRC(insn->code) == BPF_K)
    and linked_regs = 0

In this particular, we will still generate a jump table entry which
is not used in backtrack_insn().

>
> [...]