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

On Wed, Jul 29, 2026 at 10:49 PM Sanghyun Park
<[email protected]> 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: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=cdd6c0925e12b0af60cc
> Reported-by: [email protected]
> Closes: https://lore.kernel.org/r/[email protected]
> Tested-by: Sun Jian <[email protected]>
> Reviewed-by: Puranjay Mohan <[email protected]>
> Acked-by: Ihor Solodrai <[email protected]>
> Signed-off-by: Sanghyun Park <[email protected]>
> ---
> v2:
>  - Drop irq_work_is_busy() and rely exclusively on active, as suggested by Ihor.
>
> v1: https://lore.kernel.org/bpf/[email protected]/
>
>  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..d6e2b65f6d052 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,9 @@ static inline bool bpf_mmap_unlock_get_irq_work(struct mmap_unlock_irq_work **wo

tbh, bpf_mmap_unlock_get_irq_work() name and its semantics of
returning true if we can't really do anything seems convoluted and
backwards.

maybe we should make bpf_mmap_unlock_get_irq_work() return `struct
mmap_unlock_irq_work *` which would be ERR_PTR(), and return
ERR_PTR(-EBUSY) if (active || irq_work_is_busy) (see below)? I'd
probably call it bpf_mmap_unlock_guard_get(), though. And then we will
have bpf_mmap_unlock_guard_put() (instead of
bpf_mmap_unlock_put_irq_work) with the same logic.

and so in code, we'll check

work = bpf_mmap_unlock_guard_get();
if (IS_ERR(work)) return /* womp womp */

....

bpf_mmap_unlock_guard_put();


thoughts?

>         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 (atomic_cmpxchg_acquire(&work->active, 0, 1)) {
>                                 irq_work_busy = true;
> +                               work = NULL;
>                         }
>                 } else {
>                         /*

[...]

> @@ -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);

we drop active to zero here, but irq_work_is_busy() can still return
true, I think we do need to check both

>  }
>
>  static int __init task_iter_init(void)
> --
> 2.48.1