On Sat, 17 Feb 2024 at 18:14, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > On Fri, 2024-02-16 at 23:06 +0100, Kumar Kartikeya Dwivedi wrote: > [...] > > > I will add tests to exercise these cases, but the idea for STACK_ZERO > > was to treat it as if we had a NULL value in that stack slot, thus > > allowing merging with other resource pointers. Sometimes when NULL > > initializing something, it can be marked as STACK_ZERO in the verifier > > state. Therefore, we would prefer to treat it same as the case where a > > scalar reg known to be zero is spilled to the stack (that is what we > > do by using a fake struct bpf_reg_state). > > Agree that it is useful to merge 0/NULL/STACK_ZERO with PTR_TO_SOMETHING. > What I meant is that merging with 0 is a noop and there is no need to > add a new fd entry. However, I missed the following check: > > + static int gen_exception_frame_desc_reg_entry(struct bpf_verifier_env *env, struct bpf_reg_state *reg, int off, int frameno) > + { > + struct bpf_frame_desc_reg_entry fd = {}; > + > + if ((!reg->ref_obj_id && reg->type != NOT_INIT) || reg->type == SCALAR_VALUE) > + return 0; > > So, the intent is to skip adding fd entry if register has scalar type, right? > I tried the following test case: > > SEC("?tc") > __success > int test(struct __sk_buff *ctx) > { > asm volatile ( > "r7 = *(u32 *)(r1 + 0); \ > r1 = %[ringbuf] ll; \ > r2 = 8; \ > r3 = 0; \ > r0 = 0; \ > if r7 > 42 goto +1; \ > call %[bpf_ringbuf_reserve]; \ > *(u64 *)(r10 - 8) = r0; \ > r0 = 0; \ > r1 = 0; \ > call bpf_throw; \ > " : > : __imm(bpf_ringbuf_reserve), > __imm_addr(ringbuf) > : "memory"); > return 0; > } > > And it adds fp[-8] entry for one branch and skips fp[-8] for the other. > However, the following test passes as well (note 'r0 = 7'): > > SEC("?tc") > __success > int same_resource_many_regs(struct __sk_buff *ctx) > { > asm volatile ( > "r7 = *(u32 *)(r1 + 0); \ > r1 = %[ringbuf] ll; \ > r2 = 8; \ > r3 = 0; \ > r0 = 7; /* !!! */ \ > if r7 > 42 goto +1; \ > call %[bpf_ringbuf_reserve]; \ > *(u64 *)(r10 - 8) = r0; \ > r0 = 0; \ > r1 = 0; \ > call bpf_throw; \ > " : > : __imm(bpf_ringbuf_reserve), > __imm_addr(ringbuf) > : "memory"); > return 0; > } > > Which is probably not correct, as scalar 7 would be used as a > parameter for ringbuf destructor, right? I think you are right, we probably need to create an unmergeable slot in case we find a non-zero scalar value in the stack as well. I will fix this and add tests as well. Thanks a lot for your thorough reviews! Really appreciate it.