Re: [PATCH bpf-next] bpf: Fix mmap_lock leak in irq_work path

[sun jian]

On Wed, Jul 22, 2026 at 10:31 AM Sanghyun Park
<sanghyun.park.cnu@xxxxxxxxx> wrote:
>
> stack_map_get_build_id_offset() introduced a per-CPU irq_work to defer
> mmap_read_unlock() from NMI context, and bpf_find_vma() later reused the
> same mmap_unlock_work. Both callers only check whether the work is busy
> before taking mmap_lock, so a nested caller can reuse the slot before the
> first caller queues it. Two read locks may then be acquired while only one
> deferred unlock runs, leaking a read lock and blocking exit_mmap().
>
> Reserve the per-CPU slot before mmap_read_trylock(). Use the same wrapper
> in stackmap and bpf_find_vma() so both callers release the reservation on
> trylock failure. Release it after the irq_work callback unlocks the mm.
>
> Fixes: bae77c5eb5b2 ("bpf: enable stackmap with build_id in nmi context")
> Reported-by: syzbot+cdd6c0925e12b0af60cc@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=cdd6c0925e12b0af60cc
> Reported-by: sashiko-bot@xxxxxxxxxx
> Closes: https://lore.kernel.org/r/20260630033745.B80201F000E9@xxxxxxxxxxxxxxx
> Signed-off-by: Sanghyun Park <sanghyun.park.cnu@xxxxxxxxx>
> ---
>  kernel/bpf/mmap_unlock_work.h | 37 +++++++++++++++++++++++++++++++----
>  kernel/bpf/stackmap.c         |  3 +--
>  kernel/bpf/task_iter.c        |  7 +++----
>  3 files changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/bpf/mmap_unlock_work.h b/kernel/bpf/mmap_unlock_work.h
> index 5d18d7d85bef9..d416e4337635f 100644
> --- a/kernel/bpf/mmap_unlock_work.h
> +++ b/kernel/bpf/mmap_unlock_work.h
> @@ -4,12 +4,14 @@
>
>  #ifndef __MMAP_UNLOCK_WORK_H__
>  #define __MMAP_UNLOCK_WORK_H__
> +#include <linux/atomic.h>
>  #include <linux/irq_work.h>
>
>  /* irq_work to run mmap_read_unlock() in irq_work */
>  struct mmap_unlock_irq_work {
>         struct irq_work irq_work;
>         struct mm_struct *mm;
> +       atomic_t active;
>  };
>
>  DECLARE_PER_CPU(struct mmap_unlock_irq_work, mmap_unlock_work);
> @@ -18,8 +20,8 @@ DECLARE_PER_CPU(struct mmap_unlock_irq_work, mmap_unlock_work);
>   * We cannot do mmap_read_unlock() when the irq is disabled, because of
>   * risk to deadlock with rq_lock. To look up vma when the irqs are
>   * disabled, we need to run mmap_read_unlock() in irq_work. We use a
> - * percpu variable to do the irq_work. If the irq_work is already used
> - * by another lookup, we fall over.
> + * percpu variable to do the irq_work. The active flag reserves the slot
> + * before mmap_read_trylock() and until the irq_work callback consumes mm.
>   */
>  static inline bool bpf_mmap_unlock_get_irq_work(struct mmap_unlock_irq_work **work_ptr)
>  {
> @@ -29,9 +31,10 @@ static inline bool bpf_mmap_unlock_get_irq_work(struct mmap_unlock_irq_work **wo
>         if (irqs_disabled()) {
>                 if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
>                         work = this_cpu_ptr(&mmap_unlock_work);
> -                       if (irq_work_is_busy(&work->irq_work)) {
> -                               /* cannot queue more up_read, fallback */
> +                       if (irq_work_is_busy(&work->irq_work) ||
> +                           atomic_cmpxchg_acquire(&work->active, 0, 1)) {
>                                 irq_work_busy = true;
> +                               work = NULL;
>                         }
>                 } else {
>                         /*
> @@ -46,6 +49,32 @@ static inline bool bpf_mmap_unlock_get_irq_work(struct mmap_unlock_irq_work **wo
>         return irq_work_busy;
>  }
>
> +static inline void bpf_mmap_unlock_put_irq_work(struct mmap_unlock_irq_work *work)
> +{
> +       if (work)
> +               atomic_set_release(&work->active, 0);
> +}
> +
> +/*
> + * Try to take mm->mmap_lock for reading on behalf of a BPF helper that may
> + * run with IRQs disabled. On success, *work is the slot to hand to
> + * bpf_mmap_unlock_mm() (NULL when the unlock can be done inline); on failure
> + * no slot stays reserved and the caller must fall back.
> + */
> +static inline bool bpf_mmap_read_trylock(struct mm_struct *mm,
> +                                        struct mmap_unlock_irq_work **work)
> +{
> +       if (bpf_mmap_unlock_get_irq_work(work))
> +               return false;
> +
> +       if (!mmap_read_trylock(mm)) {
> +               bpf_mmap_unlock_put_irq_work(*work);
> +               return false;
> +       }
> +
> +       return true;
> +}
> +
>  static inline void bpf_mmap_unlock_mm(struct mmap_unlock_irq_work *work, struct mm_struct *mm)
>  {
>         if (!work) {
> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index 41fe87d7302f2..166fc8efab8b2 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
> @@ -415,7 +415,6 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
>                                           u32 trace_nr, bool user, bool may_fault)
>  {
>         struct mmap_unlock_irq_work *work = NULL;
> -       bool irq_work_busy = bpf_mmap_unlock_get_irq_work(&work);
>         bool has_user_ctx = user && current && current->mm;
>         struct stack_map_build_id_cache cache = {};
>         struct vm_area_struct *vma;
> @@ -430,7 +429,7 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
>          * fallback is used for kernel stack (!user) on a stackmap with
>          * build_id.
>          */
> -       if (!has_user_ctx || irq_work_busy || !mmap_read_trylock(current->mm)) {
> +       if (!has_user_ctx || !bpf_mmap_read_trylock(current->mm, &work)) {
>                 /* cannot access current->mm, fall back to ips */
>                 for (i = 0; i < trace_nr; i++)
>                         stack_map_build_id_set_ip(&id_offs[i]);
> diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> index b256fb9c1214e..e66e3484f923d 100644
> --- a/kernel/bpf/task_iter.c
> +++ b/kernel/bpf/task_iter.c
> @@ -755,7 +755,6 @@ BPF_CALL_5(bpf_find_vma, struct task_struct *, task, u64, start,
>  {
>         struct mmap_unlock_irq_work *work = NULL;
>         struct vm_area_struct *vma;
> -       bool irq_work_busy = false;
>         bool __maybe_unused mmput_needed = false;
>         struct mm_struct *mm;
>         int ret = -ENOENT;
> @@ -792,9 +791,7 @@ BPF_CALL_5(bpf_find_vma, struct task_struct *, task, u64, start,
>         if (!mm)
>                 return -ENOENT;
>
> -       irq_work_busy = bpf_mmap_unlock_get_irq_work(&work);
> -
> -       if (irq_work_busy || !mmap_read_trylock(mm)) {
> +       if (!bpf_mmap_read_trylock(mm, &work)) {
>                 ret = -EBUSY;
>                 goto out;
>         }
> @@ -1191,6 +1188,8 @@ static void do_mmap_read_unlock(struct irq_work *entry)
>
>         work = container_of(entry, struct mmap_unlock_irq_work, irq_work);
>         mmap_read_unlock_non_owner(work->mm);
> +       work->mm = NULL;
> +       bpf_mmap_unlock_put_irq_work(work);
>  }
>
>  static int __init task_iter_init(void)
> --
> 2.48.1

Hi Sanghyun,

I tested this patch (HEAD 384544c1660b) with a local IRQ-to-NMI
interleaving harness. In two runs, the nested bpf_find_vma() invocation
was reached after the outer path had reserved the per-CPU work slot, and
it returned -EBUSY as expected. The outer path then completed and the
loader exited normally.

  run 1: outer=103475 callback=103475 nmi_seen=1200
         inner=1 inner_find_vma=1 inner_ret=-16
  run 2: outer=177463 callback=177463 nmi_seen=1198
         inner=1 inner_find_vma=1 inner_ret=-16

I also ran:

  ./test_progs -t find_vma -v

  #132     find_vma:OK
  Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED

Tested-by: Sun Jian <sun.jian.kdev@xxxxxxxxx>