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 --- 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 #include /* 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