Re: [PATCH v1] mm/vmalloc: read vm_map_ram block regions from the requested offset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: off-by-one region size in vmap_ram_vread_iter()
--

commit 5892fe6e95a3367f16e7bc56c39b500d0294baac
Author: Nickolai Zeldovich <nickolai@csail.mit.edu>

mm/vmalloc: read vm_map_ram block regions from the requested offset

This commit updates vmap_ram_vread_iter() to skip vmap_block used regions
that end at or before the current read address and to compute the read
offset relative to the start of the region rather than the page offset.

> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index bea9f76ed7e74..34340c2490b82 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -4588,7 +4588,7 @@ static size_t vmap_ram_vread_iter(struct iov_iter *iter, const char *addr,
>  	struct xarray *xa;
>  	unsigned long offset;
>  	unsigned int rs, re;
> -	size_t remains, n;
> +	size_t remains, n, size;
>  
>  	/*
>  	 * If it's area created by vm_map_ram() interface directly, but
> @@ -4622,6 +4622,10 @@ static size_t vmap_ram_vread_iter(struct iov_iter *iter, const char *addr,
>  			goto finished;
>  
>  		start = vmap_block_vaddr(vb->va->va_start, rs);
> +		size = (re - rs + 1) << PAGE_SHIFT;
> +
> +		if (addr >= start + size)
> +			continue;

[Severity: High]
Is size off by one page here?

In include/linux/find.h, for_each_set_bitrange() sets the end index using
find_next_zero_bit():

#define for_each_set_bitrange(b, e, addr, size)			\
	for ((b) = 0;						\
	     (b) = find_next_bit((addr), (size), b),		\
	     (e) = find_next_zero_bit((addr), (size), (b) + 1),	\
	     (b) < (size);					\
	     (b) = (e) + 1)

Because re is the index of the first unset bit after rs (an exclusive upper
bound), the number of allocated pages in the bitrange [rs, re) is (re - rs)
rather than (re - rs + 1).

With size = (re - rs + 1) << PAGE_SHIFT, start + size points one page past
the end of the used region. When read_kcore_iter() -> vread_iter() ->
vmap_ram_vread_iter() reads across the end of a used region or starts at
the first unset page at index re, addr >= start + size does not skip the
preceding region, and page re is passed to aligned_vread_iter() instead of
zero_iter().

In mm/vmalloc.c:vb_free(), vb->used_map is cleared under vb->lock before
vunmap_range_noflush() tears down the PTEs outside the lock:

	spin_lock(&vb->lock);
	bitmap_clear(vb->used_map, offset, (1UL << order));
	spin_unlock(&vb->lock);

	vunmap_range_noflush(addr, addr + size);

Could treating the unset bit at re as part of the used region cause
aligned_vread_iter() to read a page that is concurrently being unmapped and
freed, or read one page past vb->va->va_end when re == VMAP_BBMAP_BITS?

>  
>  		if (addr < start) {
>  			size_t to_zero = min_t(size_t, start - addr, remains);
> @@ -4635,8 +4639,8 @@ static size_t vmap_ram_vread_iter(struct iov_iter *iter, const char *addr,
>  		}
>  
>  		/*it could start reading from the middle of used region*/
> -		offset = offset_in_page(addr);
> -		n = ((re - rs + 1) << PAGE_SHIFT) - offset;
> +		offset = addr - start;
> +		n = size - offset;
>  		if (n > remains)
>  			n = remains;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017154120.795844-1-nickolai@csail.mit.edu?part=1