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

[bot+bpf-ci@kernel.org]

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