Re: [PATCH bpf v4 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 platforms without CONFIG_PER_VMA_LOCK.
- [High] Incorrect file-backed VMA classification using !vma->vm_file instead of vma_is_anonymous(vma).
- [Medium] Severe I/O performance regression due to removal of VMA and build ID caching.
--

commit 5e68cb2c06b22387698d796c574ee7addecfea18
Author: Ihor Solodrai <ihor.solodrai@linux.dev>

bpf: Avoid faultable build ID reads under mm locks

This commit introduces a sleepable path for build ID parsing to avoid
blocking in __kernel_read() while holding mmap_lock or a per-VMA read lock.

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

[ ... ]

> +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 platforms without CONFIG_PER_VMA_LOCK? If
the fallback logic acquires the mmap_read_trylock() and successfully locates
the VMA, it looks like it will drop the lock and return NULL in the #else
block.

Could the locking abstraction be extended to hold the mmap_read_lock across
the get_file() call when per-VMA locks are unavailable?

> +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,
> +	};
> +	unsigned long vm_pgoff, vm_start;
> +	struct vm_area_struct *vma;
> +	struct file *file;
> +	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) {

Can anonymous VMAs (like private mappings of /dev/zero) have a valid vm_file
pointer? If so, using !vma->vm_file as a proxy for file-backed
classification might dispatch pseudo-files to build_id_parse_file().

Would using !vma_is_anonymous(vma) be more robust here?

> +			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;
> +		}
> +		fput(file);

Does this cause an I/O performance drop by removing the VMA and build ID
caching that exists in the non-sleepable path? Without caching prev_vma and
prev_build_id, this loop appears to perform get_file(), build_id_parse_file()
(which reads from the filesystem), and fput() for every single frame in the
stack trace.

I note that this caching is added back later in the series in commit
38cfe2cebe4d604860680459fcb33a68258f0654 ("bpf: Cache build IDs in
sleepable stackmap path"). Should this patch include the caching to avoid
introducing a temporary regression if bisecting?

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