* Re: [PATCH bpf-next] bpf: Fix mmap_lock leak in irq_work path
2026-07-22 2:30 [PATCH bpf-next] bpf: Fix mmap_lock leak in irq_work path Sanghyun Park
@ 2026-07-22 6:24 ` sun jian
2026-07-29 16:58 ` Puranjay Mohan
2026-07-29 20:03 ` Ihor Solodrai
2 siblings, 0 replies; 4+ messages in thread
From: sun jian @ 2026-07-22 6:24 UTC (permalink / raw)
To: Sanghyun Park
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Eduard Zingerman, Kumar Kartikeya Dwivedi, Martin KaFai Lau,
Song Liu, Yonghong Song, Jiri Olsa, Emil Tsalapatis,
Ihor Solodrai, Puranjay Mohan, bpf, linux-kernel,
syzbot+cdd6c0925e12b0af60cc, sashiko-bot
On Wed, Jul 22, 2026 at 10:31 AM 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
> 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)) {
> 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@gmail.com>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH bpf-next] bpf: Fix mmap_lock leak in irq_work path
2026-07-22 2:30 [PATCH bpf-next] bpf: Fix mmap_lock leak in irq_work path Sanghyun Park
2026-07-22 6:24 ` sun jian
@ 2026-07-29 16:58 ` Puranjay Mohan
2026-07-29 20:03 ` Ihor Solodrai
2 siblings, 0 replies; 4+ messages in thread
From: Puranjay Mohan @ 2026-07-29 16:58 UTC (permalink / raw)
To: Sanghyun Park
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Eduard Zingerman, Sun Jian, Kumar Kartikeya Dwivedi,
Martin KaFai Lau, Song Liu, Yonghong Song, Jiri Olsa,
Emil Tsalapatis, Ihor Solodrai, bpf, linux-kernel,
syzbot+cdd6c0925e12b0af60cc, sashiko-bot
On Wed, Jul 22, 2026 at 3:31 AM 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
> 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)) {
> 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
Thanks for this, it looks good to me.
Reviewed-by: Puranjay Mohan <puranjay@kernel.org>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH bpf-next] bpf: Fix mmap_lock leak in irq_work path
2026-07-22 2:30 [PATCH bpf-next] bpf: Fix mmap_lock leak in irq_work path Sanghyun Park
2026-07-22 6:24 ` sun jian
2026-07-29 16:58 ` Puranjay Mohan
@ 2026-07-29 20:03 ` Ihor Solodrai
2 siblings, 0 replies; 4+ messages in thread
From: Ihor Solodrai @ 2026-07-29 20:03 UTC (permalink / raw)
To: Sanghyun Park, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Eduard Zingerman
Cc: Sun Jian, Kumar Kartikeya Dwivedi, Martin KaFai Lau, Song Liu,
Yonghong Song, Jiri Olsa, Emil Tsalapatis, Puranjay Mohan, bpf,
linux-kernel, syzbot+cdd6c0925e12b0af60cc, sashiko-bot
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 {
> /*
> [...]
^ permalink raw reply [flat|nested] 4+ messages in thread