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.
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.
