Re: [PATCH bpf-next v3 4/7] bpf: Introduce support for bpf_local_irq_{save,restore}

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2024-11-28 at 05:39 +0100, Kumar Kartikeya Dwivedi wrote:

[...]

> > > +static bool is_irq_flag_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> > > +{
> > > +     struct bpf_func_state *state = func(env, reg);
> > > +     struct bpf_stack_state *slot;
> > > +     int spi, i;
> > > +
> > > +     /* For -ERANGE (i.e. spi not falling into allocated stack slots), we
> > > +      * will do check_mem_access to check and update stack bounds later, so
> > > +      * return true for that case.
> > > +      */
> > > +     spi = irq_flag_get_spi(env, reg);
> > > +     if (spi == -ERANGE)
> > > +             return true;
> > 
> > Nit: is it possible to swap is_irq_flag_reg_valid_uninit() and
> >      check_mem_access(), so that ERANGE special case would be not needed?
> > 
> 
> I don't think so. For dynptr, iter, irq, ERANGE indicates stack needs
> to be grown, so check_mem_access will naturally do that when writing.
> When not ERANGE, we need to catch cases where we have a bad slot_type.
> If we overwrote it with check_mem_access, then it would scrub the slot
> type as well.
> 
> When I fixed this stuff for dynptr, we had to additionally
> destroy_if_dynptr_stack_slot because it wasn't required to 'release' a
> dynptr when overwriting it.
> Andrii made sure this was necessary for iters so now slot_type ==
> STACK_ITER is just rejected instead of overwrite without a destroy
> operation.
> Similar idea is followed for irq flag.
> 
> Just paging in context for all this, but I may be missing if you have
> something in mind.

I see, makes sense. And is_dynptr_reg_valid_uninit() has the same check.
Thank you for explaining.

> > > +     if (spi < 0)
> > > +             return false;
> > > +
> > > +     slot = &state->stack[spi];
> > > +
> > > +     for (i = 0; i < BPF_REG_SIZE; i++)
> > > +             if (slot->slot_type[i] == STACK_IRQ_FLAG)
> > > +                     return false;
> > > +     return true;
> > > +}
> > 
> > [...]
> >