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?
