Re: [PATCH bpf-next v3 1/2] bpf: Implement dtor for struct file BTF ID

On Fri, Apr 24, 2026 at 1:12 PM Justin Suess <[email protected]> wrote:
>
> On Fri, Apr 24, 2026 at 07:59:57PM +0000, [email protected] wrote:
> > Sashiko AI review found 1 potential issue(s):
> > - [High] Calling `fput()` directly from `bpf_file_release_dtor` can lead to a hard deadlock when invoked from NMI context.
> > --
> >
> > commit 6368f91be1fd91bc9cb38d3498f35d279bad04c9
> > Author: Justin Suess <[email protected]>
> >
> > bpf: Implement dtor for struct file BTF ID
> >
> > This commit implements a destructor for struct file, allowing it to be
> > stored in a map as a referenced kptr. The destructor calls fput to
> > release the reference.
> >
> > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > index 2bb60200c2665e..ee5ceb82e4f812 100644
> > > --- a/kernel/bpf/helpers.c
> > > +++ b/kernel/bpf/helpers.c
> > [ ... ]
> > > @@ -2752,6 +2753,13 @@ __bpf_kfunc void bpf_task_release_dtor(void *p)
> > >  }
> > >  CFI_NOSEAL(bpf_task_release_dtor);
> > >
> > > +__bpf_kfunc void bpf_file_release_dtor(void *file)
> > > +{
> > > +   fput((struct file *)file);
> > > +}
> >
> > Can this cause a deadlock if the destructor is invoked from NMI context?
> >
> > If an NMI-context BPF program (such as a perf event tracing program)
> > updates or deletes a map element containing a struct file kptr, the BPF
> > framework will synchronously invoke this destructor to clean up the old
> > kptr.
> >
> > When fput() drops the reference count to zero, it calls
> > __fput_deferred(). Since in_interrupt() evaluates to true in NMI
> > context, __fput_deferred() bypasses task_work_add() and instead calls
> > schedule_delayed_work().
> >
> > This leads to the following call chain:
> >
> > bpf_file_release_dtor()
> >    fput()
> >       __fput_deferred()
> >          schedule_delayed_work()
> >             add_timer()
> >
> > The add_timer() function attempts to acquire the timer_base->lock raw
> > spinlock. If the NMI interrupted a thread that was already holding this
> > lock, the NMI handler will spin indefinitely waiting for it, resulting
> > in a hard deadlock.
> >
> > Would it be safer to defer the fput() operation using an explicitly
> > NMI-safe mechanism like irq_work_queue() to handle arbitrary BPF
> > contexts?
> >
> For (human) reviewer context: The NMI stuff needs to be addressed separately. [1]
>
> I guess the AI isn't gonna understand but just doing irq_work wouldn't
> be right either as it would break operation ordering for maps and
> fixing it here wouldn't fix the other dtors broken in NMI.
> (cgroup/task_struct)

Will break operation ordering?
What do you mean?

I feel we should fix things first before being subject
to more of these bugs.
Why cannot we defer to irq_work the whole map element if in_nmi
and call all dtors there?
should be a simple fix.