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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Sleepable build ID parsing unconditionally fails on systems where CONFIG_PER_VMA_LOCK is disabled.
- [High] Incorrect VMA classification using `!vma->vm_file` violates MM API and risks unsafe driver reads.
- [High] Missing caching causes a severe performance regression due to redundant file I/O for IPs within the same VMA.
--

commit 8402e095f9b6a82fa06a32af10007e005d6c5e76
Author: Ihor Solodrai <ihor.solodrai@linux.dev>
bpf: Avoid faultable build ID reads under mm locks

This commit updates the stackmap sleepable path to avoid calling
build_id_parse_file() while holding mmap_lock or a per-VMA read lock. It
achieves this by acquiring a stable read-side reference to the backing file,
dropping the VMA lock, and then parsing the build ID.

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/

> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index 4ef0fd06cea58..c1e96df360c3f 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c

[ ... ]

> @@ -158,6 +159,105 @@ static inline void stack_map_build_id_set_ip(struct bpf_stack_build_id *id)
>  	memset(id->build_id, 0, BUILD_ID_SIZE_MAX);
>  }
>  
> +struct stack_map_vma_lock {
> +	bool vma_locked;
> +	struct vm_area_struct *vma;
> +	struct mm_struct *mm;
> +};
> +
> +static struct vm_area_struct *
> +stack_map_lock_vma(struct stack_map_vma_lock *lock, unsigned long ip)
> +{

[ ... ]

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

Does this unconditionally fail on systems where CONFIG_PER_VMA_LOCK is
disabled?

Looking at the fallback block, it unlocks the mmap_read_lock and returns
NULL. Will this cause stack_map_get_build_id_offset_sleepable() to always
receive a NULL VMA and fail to extract any build IDs on architectures lacking
per-VMA lock support?

[ ... ]

> +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,
> +	};
> +	unsigned long vm_pgoff, vm_start;
> +	struct vm_area_struct *vma;
> +	struct file *file;
> +	u64 ip;
> +
> +	for (u32 i = 0; i < trace_nr; i++) {

I see this is addressed in a later commit in the series ("bpf: Cache build
IDs in sleepable stackmap path"), but is there a risk of a performance
regression here?

The original non-sleepable path cached the previous VMA and build ID. Since
that caching logic is missing here, will adjacent stack frames from the same
binary repeatedly trigger file reference acquisitions and blocking reads for
every single IP?

> +		ip = READ_ONCE(id_offs[i].ip);
> +		vma = stack_map_lock_vma(&lock, ip);
> +		if (!vma || !vma->vm_file) {

Is checking vma->vm_file sufficient to determine if the mapping is
file-backed?

Anonymous VMAs, such as private mappings of /dev/zero or driver mappings
calling vma_set_anonymous(), can legitimately have vm_file set.

Could passing these anonymous VMAs to build_id_parse_file() invoke
__kernel_read() on a driver's read handler, leading to unintended side
effects or hangs?

Would it be safer to use !vma_is_anonymous(vma) instead?

> +			stack_map_build_id_set_ip(&id_offs[i]);
> +			stack_map_unlock_vma(&lock);
> +			continue;
> +		}
> +
> +		file = get_file(vma->vm_file);
> +		vm_pgoff = vma->vm_pgoff;
> +		vm_start = vma->vm_start;
> +		stack_map_unlock_vma(&lock);
> +
> +		/* build_id_parse_file() may block on filesystem reads */
> +		if (build_id_parse_file(file, id_offs[i].build_id, NULL)) {
> +			stack_map_build_id_set_ip(&id_offs[i]);
> +			fput(file);
> +			continue;
> +		}

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