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

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

[Sanghyun Park]

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@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
Tested-by: Sun Jian <sun.jian.kdev@gmail.com>
Reviewed-by: Puranjay Mohan <puranjay@kernel.org>
Acked-by: Ihor Solodrai <ihor.solodrai@linux.dev>
Signed-off-by: Sanghyun Park <sanghyun.park.cnu@gmail.com>
---
v2:
 - Drop irq_work_is_busy() and rely exclusively on active, as suggested by Ihor.

v1: https://lore.kernel.org/bpf/20260722023004.1497923-2-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..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
 	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 {
 			/*
@@ -46,6 +48,33 @@ 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

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

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 4668 bytes --]

> commit 1be4c688511464bfec8b05e78fe8b3867a39f159
> Author: Sanghyun Park <sanghyun.park.cnu@gmail.com>
>
>     bpf: Fix mmap_lock leak in irq_work path
>
>     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")

Is the Fixes tag accurate? The described leak requires an interruptible
non-NMI holder of the slot. At bae77c5eb5b2, the slot was taken only under
'if (in_nmi())' and the only user was stack_map_get_build_id_offset(), which
contains no callback and cannot re-enter itself. NMIs do not nest, so two
acquisitions of the slot before the first queue are impossible at that commit.

The window appears to be opened by eac9153f2b58 ('bpf/stackmap: Fix deadlock
with rq_lock in bpf_get_stack()'), which changed 'if (in_nmi())' to 'if
(irqs_disabled())', allowing an interruptible non-NMI holder of the slot.
Commit 7c7e3d31e785, which shares the slot with bpf_find_vma, widens it
further.

Should the Fixes tag reference eac9153f2b58 instead? This also affects which
stable kernels receive the backport.

>     Reported-by: syzbot+cdd6c0925e12b0af60cc@syzkaller.appspotmail.com
>     Closes: https://syzkaller.appspot.com/bug?extid=cdd6c0925e12b0af60cc

Can this syzbot report be verified? Searching the lore archive for the ID
'cdd6c0925e12b0af60cc' returns only this patch's own v1/v2 postings and their
replies. No mail from syzbot containing that ID exists in the archive.

Restricting searches to from:syzbot or from:syzkaller plus that ID returns
zero results. The archive does index syzbot mail generally, so a genuine
report would be expected to be present.

Searches for any syzbot report touching mmap_unlock_work,
do_mmap_read_unlock, stack_map_get_build_id_offset, or
mmap_read_unlock_non_owner also return nothing relevant.

The only verifiable reporter is the next Reported-by tag. Can the syzbot
Reported-by and Closes tags be substantiated, or should they be dropped?

[ ... ]

> diff --git a/kernel/bpf/mmap_unlock_work.h b/kernel/bpf/mmap_unlock_work.h
> index 5d18d7d85bef..d6e2b65f6d05 100644
> --- a/kernel/bpf/mmap_unlock_work.h
> +++ b/kernel/bpf/mmap_unlock_work.h

[ ... ]

> @@ -46,6 +48,33 @@ 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;
> +}

The comment states "on failure no slot stays reserved", which is true after
bpf_mmap_unlock_put_irq_work() releases the reservation on the
mmap_read_trylock() failure path. However, *work is left non-NULL on that
path while unreserved, so a future caller following the comment's model ('*work
is the slot to hand to bpf_mmap_unlock_mm()') and passing work to
bpf_mmap_unlock_mm() would queue an unlock for a lock it does not hold.

Neither current caller does this (stackmap.c and task_iter.c both return or
goto out), but would it be safer if the code set *work = NULL on the
mmap_read_trylock() failure path, or if the comment stated that *work is
meaningful only on success?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/30518165492

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

[Andrii Nakryiko]

On Wed, Jul 29, 2026 at 10:49 PM Sanghyun Park
<sanghyun.park.cnu@gmail.com> 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@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
> Tested-by: Sun Jian <sun.jian.kdev@gmail.com>
> Reviewed-by: Puranjay Mohan <puranjay@kernel.org>
> Acked-by: Ihor Solodrai <ihor.solodrai@linux.dev>
> Signed-off-by: Sanghyun Park <sanghyun.park.cnu@gmail.com>
> ---
> v2:
>  - Drop irq_work_is_busy() and rely exclusively on active, as suggested by Ihor.
>
> v1: https://lore.kernel.org/bpf/20260722023004.1497923-2-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..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

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

[Sanghyun Park]

> 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.

Yes, I think this would make the code a bit more straightforward.

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

Yes, BUSY can still be set after active is cleared. However, since the work can
be queued again while the callback is still running, I thought active alone was
enough. I also tested it in a widened window, and didn't see a lock leak.

But I may be missing something. Could requeueing the work here cause a problem,
or is this mainly to keep the original -EBUSY behavior?

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

[Andrii Nakryiko]

On Mon, Aug 3, 2026 at 2:44 AM Sanghyun Park
<sanghyun.park.cnu@gmail.com> wrote:
>
> > 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.
>
> Yes, I think this would make the code a bit more straightforward.
>
> > we drop active to zero here, but irq_work_is_busy() can still return
> > true, I think we do need to check both
>
> Yes, BUSY can still be set after active is cleared. However, since the work can
> be queued again while the callback is still running, I thought active alone was
> enough. I also tested it in a widened window, and didn't see a lock leak.
>
> But I may be missing something. Could requeueing the work here cause a problem,
> or is this mainly to keep the original -EBUSY behavior?

I'm just not sure that re-queueing works if irq_work_is_busy() returns
true, please double check. If that doesn't work, then we can miss the
irq_work callback, which would be bad.

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

[Sanghyun Park]

On Mon, Aug 3, 2026 at 5:44 PM UTC, Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> I'm just not sure that re-queueing works if irq_work_is_busy() returns
> true, please double check. If that doesn't work, then we can miss the
> irq_work callback, which would be bad.

I see your point. I double-checked this, and re-queueing seems to work.

However, keeping the irq_work_is_busy() check only adds one flag read, so the
performance impact is quite negligible. I'll keep both checks in a conservative
manner.