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

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)

Anyways I think the AI didn't find any other problems and the test issue
is fixed so this should be ready for another look.

[1] : https://lore.kernel.org/bpf/[email protected]/
> > +
> > +CFI_NOSEAL(bpf_file_release_dtor);
> > +
> >  #ifdef CONFIG_CGROUPS
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1