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

[Florent Revest]

On Fri Aug 21, 2026 at 7:58 AM UTC, Junseo Lim wrote:
> On Thu, Aug 20, 2026 at 04:06:30PM +0000, florent.revest@xxxxxxxxx wrote:
> > 20 août 2026 à 17:26 "Junseo Lim" <zirajs7@xxxxxxxxx mailto:zirajs7@xxxxxxxxx?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 <rhkrqnwk98@xxxxxxxxx>
> > > 
> > > [1] https://lore.kernel.org/bpf/20260815071927.147049-1-zirajs7@xxxxxxxxx/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.

Actually, the #ifdef CONFIG_PREEMPT guard wouldn't have been enough
anyway. :/ Preemption was one easy way to cause that bug but if a task
sleeps in a sleepable prog and the prog that runs after it in the same
image is detached and freed, the same problem would happen. That could
also happen on PREEMPT_NONE.

But I'll keep your Reported-by in a v2 yep!