Re: [PATCH bpf] bpf: Keep progs alive until the trampoline image calling them is freed

On Thu, Aug 20, 2026 at 04:06:30PM +0000, [email protected] wrote:
> 20 août 2026 à 17:26 "Junseo Lim" <[email protected] mailto:[email protected]?to=%22Junseo%20Lim%22%20%3Czirajs7%40gmail.com%3E > a écrit:
> 
> > 
> > On Wed, Aug 19, 2026 at 12:22:50PM +0000, Florent Revest (Anthropic) wrote:
> > 
> > > 
> > > bpf_tramp_image_put() makes sure a trampoline image is not freed while
> > >  a task may still be running in it (call_rcu_tasks() + im->pcref), but
> > >  nothing similar is done for the progs called by that image. Since
> > >  commit e21aa341785c ("bpf: Fix fexit trampoline."), detach patches the
> > >  return path so that a task still in the original function skips the
> > >  fexit progs when it comes back, and counts on the prog's own RCU flavor
> > >  to cover a task that is inside a prog. On that basis the last prog
> > >  reference is dropped right away and the prog is freed after a single
> > >  RCU / RCU tasks trace grace period.
> > > 
> > >  [...] 
> > >  
> > >  Fix it by having the image take a reference on every prog it calls, in
> > >  bpf_tramp_image_alloc(), and drop them in bpf_tramp_image_free(). A
> > >  detached prog now stays loaded until the old image is gone, which
> > >  reverts a deliberate choice of commit e21aa341785c ("bpf: Fix fexit
> > >  trampoline."). Detached fexit progs still stop being called right away
> > >  since the return path is patched.
> > > 
> > This appears to be the same issue addressed by my earlier patch [1].
> > 
> > I think the flexible-array approach here is cleaner, so I'm fine with this
> > version going forward. Could you please carry the original Reported-by tag?
> > 
> > Reported-by: Sechang Lim <[email protected]>
> > 
> > [1] https://lore.kernel.org/bpf/[email protected]/T/
> > 
> Oh wow, sorry, I had no idea we raced on this!
> 
> I also like the idea of guarding this with CONFIG_PREEMPTION and of
> course I don't mind adding your Reported-by tag in a v2. I'll give a bit
> of time for others to chime in first.

Just in case it helps with the CONFIG_PREEMPTION angle: Sashiko pointed out
on my patch that a CONFIG_PREEMPTION guard would leave the unlink/update
failure path uncovered on CONFIG_PREEMPTION=n.

So your unconditional version looks right to me.

Cheers,
Junseo