[PATCH] mm/vmalloc: fix wrong behavior in vread

[PATCH] mm/vmalloc: fix wrong behavior in vread

[Chen Wandun]

commit f608788cd2d6 ("mm/vmalloc: use rb_tree instead of list for vread()
lookups") use rb_tree instread of list to speed up lookup, but function
__find_vmap_area is try to find a vmap_area that include target address,
if target address is smaller than the leftmost node in vmap_area_root,
it will return NULL, then vread will read nothing. This behavior is
different from the primitive semantics.

The correct way is find the first vmap_are that bigger than target addr,
that is what function find_vmap_area_exceed_addr does.

Fixes: f608788cd2d6 ("mm/vmalloc: use rb_tree instead of list for vread() lookups")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Chen Wandun <chenwandun@huawei.com>
---
 mm/vmalloc.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index d5cd52805149..47c3a551b6dc 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -787,6 +787,28 @@ unsigned long vmalloc_nr_pages(void)
 	return atomic_long_read(&nr_vmalloc_pages);
 }
 
+static struct vmap_area *find_vmap_area_exceed_addr(unsigned long addr)
+{
+	struct vmap_area *va = NULL;
+	struct rb_node *n = vmap_area_root.rb_node;
+
+	while (n) {
+		struct vmap_area *tmp;
+
+		tmp = rb_entry(n, struct vmap_area, rb_node);
+		if (tmp->va_end > addr) {
+			va = tmp;
+			if (tmp->va_start <= addr)
+				break;
+
+			n = n->rb_left;
+		} else
+			n = n->rb_right;
+	}
+
+	return va;
+}
+
 static struct vmap_area *__find_vmap_area(unsigned long addr)
 {
 	struct rb_node *n = vmap_area_root.rb_node;
@@ -3267,9 +3289,14 @@ long vread(char *buf, char *addr, unsigned long count)
 		count = -(unsigned long) addr;
 
 	spin_lock(&vmap_area_lock);
-	va = __find_vmap_area((unsigned long)addr);
+	va = find_vmap_area_exceed_addr((unsigned long)addr);
 	if (!va)
 		goto finished;
+
+	/* no intersects with alive vmap_area */
+	if ((unsigned long)addr + count <= va->va_start)
+		goto finished;
+
 	list_for_each_entry_from(va, &vmap_area_list, list) {
 		if (!count)
 			break;
-- 
2.25.1

Re: [PATCH] mm/vmalloc: fix wrong behavior in vread

[Andrew Morton]

On Wed, 14 Jul 2021 09:59:59 +0800 Chen Wandun <chenwandun@huawei.com> wrote:

> commit f608788cd2d6 ("mm/vmalloc: use rb_tree instead of list for vread()
> lookups") use rb_tree instread of list to speed up lookup, but function
> __find_vmap_area is try to find a vmap_area that include target address,
> if target address is smaller than the leftmost node in vmap_area_root,
> it will return NULL, then vread will read nothing. This behavior is
> different from the primitive semantics.

What is "the primitive semantics"?  Does this refer to behaviour prior
to f608788cd2d6?

> The correct way is find the first vmap_are that bigger than target addr,
> that is what function find_vmap_area_exceed_addr does.

Is this problem observable from userspace?  If so, what are the effects
and what must an application do to trigger it?

Re: [PATCH] mm/vmalloc: fix wrong behavior in vread

[Chen Wandun]

在 2021/7/15 7:29, Andrew Morton 写道:
> On Wed, 14 Jul 2021 09:59:59 +0800 Chen Wandun <chenwandun@huawei.com> wrote:
>
>> commit f608788cd2d6 ("mm/vmalloc: use rb_tree instead of list for vread()
>> lookups") use rb_tree instread of list to speed up lookup, but function
>> __find_vmap_area is try to find a vmap_area that include target address,
>> if target address is smaller than the leftmost node in vmap_area_root,
>> it will return NULL, then vread will read nothing. This behavior is
>> different from the primitive semantics.
> What is "the primitive semantics"?  Does this refer to behaviour prior
> to f608788cd2d6?

If address is smaller than the leftmost node in vmap_area_root and

the read size is big enough when reading [addr, addr + count),

"the primitive semantics" will fill holes by zero and copy valid vmap_area.

In such scenario,  f608788cd2d6 will read noting.

>
>> The correct way is find the first vmap_are that bigger than target addr,
>> that is what function find_vmap_area_exceed_addr does.
> Is this problem observable from userspace?  If so, what are the effects
> and what must an application do to trigger it?

Reading /proc/kcore will be affected.

Thanks,

Wandun

> .

Re: [PATCH] mm/vmalloc: fix wrong behavior in vread

[Uladzislau Rezki]

On Wed, Jul 14, 2021 at 09:59:59AM +0800, Chen Wandun wrote:
> commit f608788cd2d6 ("mm/vmalloc: use rb_tree instead of list for vread()
> lookups") use rb_tree instread of list to speed up lookup, but function
> __find_vmap_area is try to find a vmap_area that include target address,
> if target address is smaller than the leftmost node in vmap_area_root,
> it will return NULL, then vread will read nothing. This behavior is
> different from the primitive semantics.
> 
> The correct way is find the first vmap_are that bigger than target addr,
> that is what function find_vmap_area_exceed_addr does.
> 
> Fixes: f608788cd2d6 ("mm/vmalloc: use rb_tree instead of list for vread() lookups")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Chen Wandun <chenwandun@huawei.com>
> ---
>  mm/vmalloc.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index d5cd52805149..47c3a551b6dc 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -787,6 +787,28 @@ unsigned long vmalloc_nr_pages(void)
>  	return atomic_long_read(&nr_vmalloc_pages);
>  }
>  
> +static struct vmap_area *find_vmap_area_exceed_addr(unsigned long addr)
> +{
> +	struct vmap_area *va = NULL;
> +	struct rb_node *n = vmap_area_root.rb_node;
> +
> +	while (n) {
> +		struct vmap_area *tmp;
> +
> +		tmp = rb_entry(n, struct vmap_area, rb_node);
> +		if (tmp->va_end > addr) {
> +			va = tmp;
> +			if (tmp->va_start <= addr)
> +				break;
> +
> +			n = n->rb_left;
> +		} else
> +			n = n->rb_right;
> +	}
> +
> +	return va;
> +}
> +
Can we combine pvm_find_va_enclose_addr() with your new function
making it as one function? The aim is to reduce copy-paste and
not create such new "find" functions.

The pvm_find_va_enclose_addr() seems does the same but only in
reverse order. So something like:

find_vmap_area_exceed_addr(bool reverse)

Thanks!

--
Vlad Rezki

Re: [PATCH] mm/vmalloc: fix wrong behavior in vread

[Chen Wandun]

在 2021/7/15 18:58, Uladzislau Rezki 写道:
> On Wed, Jul 14, 2021 at 09:59:59AM +0800, Chen Wandun wrote:
>> commit f608788cd2d6 ("mm/vmalloc: use rb_tree instead of list for vread()
>> lookups") use rb_tree instread of list to speed up lookup, but function
>> __find_vmap_area is try to find a vmap_area that include target address,
>> if target address is smaller than the leftmost node in vmap_area_root,
>> it will return NULL, then vread will read nothing. This behavior is
>> different from the primitive semantics.
>>
>> The correct way is find the first vmap_are that bigger than target addr,
>> that is what function find_vmap_area_exceed_addr does.
>>
>> Fixes: f608788cd2d6 ("mm/vmalloc: use rb_tree instead of list for vread() lookups")
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: Chen Wandun <chenwandun@huawei.com>
>> ---
>>   mm/vmalloc.c | 29 ++++++++++++++++++++++++++++-
>>   1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>> index d5cd52805149..47c3a551b6dc 100644
>> --- a/mm/vmalloc.c
>> +++ b/mm/vmalloc.c
>> @@ -787,6 +787,28 @@ unsigned long vmalloc_nr_pages(void)
>>   	return atomic_long_read(&nr_vmalloc_pages);
>>   }
>>   
>> +static struct vmap_area *find_vmap_area_exceed_addr(unsigned long addr)
>> +{
>> +	struct vmap_area *va = NULL;
>> +	struct rb_node *n = vmap_area_root.rb_node;
>> +
>> +	while (n) {
>> +		struct vmap_area *tmp;
>> +
>> +		tmp = rb_entry(n, struct vmap_area, rb_node);
>> +		if (tmp->va_end > addr) {
>> +			va = tmp;
>> +			if (tmp->va_start <= addr)
>> +				break;
>> +
>> +			n = n->rb_left;
>> +		} else
>> +			n = n->rb_right;
>> +	}
>> +
>> +	return va;
>> +}
>> +
> Can we combine pvm_find_va_enclose_addr() with your new function
> making it as one function? The aim is to reduce copy-paste and
> not create such new "find" functions.
>
> The pvm_find_va_enclose_addr() seems does the same but only in
> reverse order. So something like:
>
> find_vmap_area_exceed_addr(bool reverse)

I dig into the two function, and found the lookup logic is some big difference between them.

pvm_find_va_enclose_addr is to find the highest vmap_area whose va_start is little than target address.

find_vmap_area_exceed_addr is to find the lowest vmap_area whose va_end that bigger than target address.

Two function maybe more clear.

But some refactor work can be done to make the code more readable and more common to use, I'm trying it.


Thanks for your suggestion.

Wandun


>
> Thanks!
>
> --
> Vlad Rezki
> .