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

[Ihor Solodrai <ihor.solodrai@linux.dev>]

On 2026-07-21 7:30 p.m., Sanghyun Park 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.

Hi Sanghyun,

Acked-by: Ihor Solodrai <ihor.solodrai@linux.dev>

The fix looks correct to me, thanks!
Just one non-blocking nit below.

> 
> Fixes: bae77c5eb5b2 ("bpf: enable stackmap with build_id in nmi context")
> Reported-by: syzbot+cdd6c0925e12b0af60cc@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=cdd6c0925e12b0af60cc
> Reported-by: sashiko-bot@kernel.org
> Closes: https://lore.kernel.org/r/20260630033745.B80201F000E9@smtp.kernel.org
> Signed-off-by: Sanghyun Park <sanghyun.park.cnu@gmail.com>
> ---
>   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)) {

It appears you could use the `active` guard exclusively to cover the
leak.

It is taken before the trylock and cleared only at the end of the
callback. So can irq_work_is_busy(&work->irq_work) be dropped here?


>   				irq_work_busy = true;
> +				work = NULL;
>   			}
>   		} else {
>   			/*
> [...]