Re: [PATCH bpf-next v5 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] Sleepable build ID parsing silently fails on kernels built without CONFIG_PER_VMA_LOCK.
- [Medium] Incorrect VMA classification using `!vma->vm_file` instead of `vma_is_anonymous()`.
--

commit 6ca3c963b80bfbdca796bfbc82d5d51cf0333054
Author: Ihor Solodrai <ihor.solodrai@linux.dev>

bpf: Avoid faultable build ID reads under mm locks

This commit modifies the sleepable stackmap path to parse build IDs without
holding the mmap_lock or per-VMA read locks. It locks the VMA via RCU or
mmap_read_trylock, captures the file reference, releases the lock, and then
performs the sleepable file read.

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

[ ... ]

> +static struct vm_area_struct *
> +stack_map_lock_vma(struct stack_map_vma_lock *lock, unsigned long ip)
> +{

[ ... ]

> +#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

Does the fallback for kernels built without CONFIG_PER_VMA_LOCK completely
disable the build ID parsing feature?

Looking at the #else branch, it unconditionally drops the mmap_read_lock and
returns NULL. This causes the caller to receive a NULL VMA for every IP,
which skips parsing and falls back to returning IP-only traces.

Could this instead keep the mmap_read_lock held, return the VMA, and allow
the caller to extract the file reference, vm_pgoff, and vm_start before
safely dropping the lock?

> +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)
> +{

[ ... ]

> +	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) {

Does checking !vma->vm_file properly identify file-backed VMAs?

Some anonymous VMAs, such as private mappings of /dev/zero, can have a
non-NULL vm_file but are conceptually anonymous since their vm_ops are NULL.
Allowing these through would result in taking a reference to the device file
and unnecessarily attempting to parse an ELF build ID from it.

Would it be safer to use vma_is_anonymous(vma) to accurately verify if the
VMA is genuinely file-backed?

> +			stack_map_build_id_set_ip(&id_offs[i]);
> +			stack_map_unlock_vma(&lock);
> +			continue;
> +		}

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