Re: [RFC PATCH v1 05/14] bpf: Implement BPF exception frame descriptor generation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux