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

[Ihor Solodrai <ihor.solodrai@linux.dev>]

On 5/12/26 9:03 PM, sashiko-bot@kernel.org wrote:
> 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.

Sashiko essentially repeated the issues flagged by BPF CI bot,
which I already replied to. Same in the second message.

Ignoring these for now.

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