Re: [PATCH bpf-next v6 2/3] bpf: Avoid faultable build ID reads under mm locks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The lock fallback path unconditionally fails when CONFIG_PER_VMA_LOCK is not set, silently breaking build ID parsing in sleepable BPF programs.
- [Medium] Incorrect check for file-backed VMAs using `vma->vm_file` instead of `!vma_is_anonymous(vma)`.
--

commit 84d83effb87030cad9aaab2d51c0df334f8c48df
Author: Ihor Solodrai <ihor.solodrai@linux.dev>

bpf: Avoid faultable build ID reads under mm locks

This commit avoids calling sleepable build ID parsing while holding mmap_lock
or a per-VMA read lock to prevent blocking. It introduces a new function to
resolve VMAs and acquire references, dropping the VMA lock before parsing.

Link: https://lore.kernel.org/all/20251218005818.614819-1-shakeel.butt@linux.dev/
Link: https://lore.kernel.org/all/20260128183232.2854138-1-andrii@kernel.org/
Link: https://lore.kernel.org/bpf/2895ecd8-df1e-4cc0-b9f9-aef893dc2360@linux.dev/

> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
[ ... ]
> @@ -174,6 +175,107 @@ static inline void stack_map_build_id_set_valid(struct bpf_stack_build_id *id,
[ ... ]
> +static struct vm_area_struct *stack_map_lock_vma(struct stack_map_vma_lock *lock, unsigned long ip)
> +{
> +	struct mm_struct *mm = lock->mm;
> +	struct vm_area_struct *vma;
> +
> +	if (WARN_ON_ONCE(!mm))
> +		return NULL;
> +
> +	vma = lock_vma_under_rcu(mm, ip);
> +	if (vma)
> +		goto vma_locked;
> +
> +	/*
> +	 * Taking mmap_read_lock() is unsafe here, because the caller
> +	 * BPF program might already hold it, causing a deadlock.
> +	 */
> +	if (!mmap_read_trylock(mm))
> +		return NULL;
> +
> +	vma = vma_lookup(mm, ip);
> +	if (!vma) {
> +		mmap_read_unlock(mm);
> +		return NULL;
> +	}
> +
> +#ifdef CONFIG_PER_VMA_LOCK
> +	if (!vma_start_read_locked(vma)) {
> +		mmap_read_unlock(mm);
> +		return NULL;
> +	}
> +	mmap_read_unlock(mm);
> +#else
> +	mmap_read_unlock(mm);
> +	return NULL;
> +#endif

[Severity: High]
Does this unconditionally break sleepable build ID parsing when
CONFIG_PER_VMA_LOCK is not enabled?

Because lock_vma_under_rcu() returns NULL without per-VMA locks, the code
falls back to mmap_read_trylock() and vma_lookup(). However, when it reaches
the #else block here, it always unlocks and returns NULL, causing the
sleepable path to fail and degrade to raw IPs.

Prior to this patch, mmap_read_trylock() was used successfully regardless
of CONFIG_PER_VMA_LOCK. Could we retain the mmap_read_lock protected fallback
for kernels without per-VMA locks, or is there another way to keep this
feature working?

> +vma_locked:
> +	lock->vma_locked = true;
> +	lock->vma = vma;
> +	return vma;
> +}
[ ... ]
> +static void stack_map_get_build_id_offset_sleepable(struct bpf_stack_build_id *id_offs,
> +						    u32 trace_nr)
> +{
> +	struct mm_struct *mm = current->mm;
> +	struct stack_map_vma_lock lock = {
> +		.vma_locked = false,
> +		.vma = NULL,
> +		.mm = mm,
> +	};
> +	struct vm_area_struct *vma;
> +	struct file *file;
> +	u64 offset;
> +	u64 ip;
> +
> +	for (u32 i = 0; i < trace_nr; i++) {
> +		ip = READ_ONCE(id_offs[i].ip);
> +
> +		vma = stack_map_lock_vma(&lock, ip);
> +		if (!vma || !vma->vm_file) {

[Severity: Medium]
Is it possible for anonymous VMAs (such as private mappings of /dev/zero) to
bypass this check?

According to the MM guidelines, vma->vm_file can be non-NULL even for
anonymous VMAs, which might lead to taking unnecessary file references and
calling build_id_parse_file() on them.

Should this use !vma_is_anonymous(vma) instead of checking vma->vm_file
directly to ensure we only process properly file-backed VMAs?

> +			stack_map_build_id_set_ip(&id_offs[i]);
> +			stack_map_unlock_vma(&lock);
> +			continue;
> +		}
> +
> +		file = get_file(vma->vm_file);
> +		offset = stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip);
> +		stack_map_unlock_vma(&lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521225022.2695755-1-ihor.solodrai@linux.dev?part=2