On Fri, 22 Nov 2024 at 01:32, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Thu, Nov 21, 2024 at 2:07 PM Kumar Kartikeya Dwivedi
> <memxor@xxxxxxxxx> wrote:
> >
> > On Thu, 21 Nov 2024 at 21:21, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> > >
> > > On Wed, 2024-11-20 at 16:53 -0800, Kumar Kartikeya Dwivedi wrote:
> > > > Teach the verifier about IRQ-disabled sections through the introduction
> > > > of two new kfuncs, bpf_local_irq_save, to save IRQ state and disable
> > > > them, and bpf_local_irq_restore, to restore IRQ state and enable them
> > > > back again.
> > > >
> > > > For the purposes of tracking the saved IRQ state, the verifier is taught
> > > > about a new special object on the stack of type STACK_IRQ_FLAG. This is
> > > > a 8 byte value which saves the IRQ flags which are to be passed back to
> > > > the IRQ restore kfunc.
> > > >
> > > > To track a dynamic number of IRQ-disabled regions and their associated
> > > > saved states, a new resource type RES_TYPE_IRQ is introduced, which its
> > > > state management functions: acquire_irq_state and release_irq_state,
> > > > taking advantage of the refactoring and clean ups made in earlier
> > > > commits.
> > > >
> > > > One notable requirement of the kernel's IRQ save and restore API is that
> > > > they cannot happen out of order. For this purpose, resource state is
> > > > extended with a new type-specific member 'prev_id'. This is used to
> > > > remember the ordering of acquisitions of IRQ saved states, so that we
> > > > maintain a logical stack in acquisition order of resource identities,
> > > > and can enforce LIFO ordering when restoring IRQ state. The top of the
> > > > stack is maintained using bpf_func_state's active_irq_id.
> > > >
> > > > The logic to detect initialized and unitialized irq flag slots, marking
> > > > and unmarking is similar to how it's done for iterators. We do need to
> > > > update ressafe to perform check_ids based satisfiability check, and
> > > > additionally match prev_id for RES_TYPE_IRQ entries in the resource
> > > > array.
> > > >
> > > > The kfuncs themselves are plain wrappers over local_irq_save and
> > > > local_irq_restore macros.
> > > >
> > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > > > ---
> > >
> > > I think this matches what is done for iterators and dynptrs.
> > >
> > > Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> > >
> > > [...]
> > >
> > > > @@ -263,10 +267,16 @@ struct bpf_resource_state {
> > > > * is used purely to inform the user of a resource leak.
> > > > */
> > > > int insn_idx;
> > > > - /* Use to keep track of the source object of a lock, to ensure
> > > > - * it matches on unlock.
> > > > - */
> > > > - void *ptr;
> > > > + union {
> > > > + /* Use to keep track of the source object of a lock, to ensure
> > > > + * it matches on unlock.
> > > > + */
> > > > + void *ptr;
> > > > + /* Track the reference id preceding the IRQ entry in acquisition
> > > > + * order, to enforce an ordering on the release.
> > > > + */
> > > > + int prev_id;
> > > > + };
> > >
> > > Nit: Do we anticipate any other resource kinds that would need LIFO acquire/release?
> > > If we do, an alternative to prev_id would be to organize bpf_func_state->res as
> > > a stack (by changing erase_resource_state() implementation).
> >
> > I don't think so, this was the weird case requiring such an ordering,
> > so I tried to find the least intrusive way.
>
> Acquire_refs is already a stack.
> Manual push/pop via prev_id looks unnecessary.
> Just search the top of acquired_refs for id.
> If it doesn't match error.
Ok.
> I don't like this bit either:
> + if (id != state->active_irq_id)
> + return -EPROTO;
>
>
> Why invent new error codes for such conditions?
> It's EACESS or EINVAL like everywhere else in the verifier.
Ok, though we do use EPROTO in a bunch of places. I just needed
something to distinguish the two errors.
I'll switch to these.
