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.
