[PATCH v2 0/2] vunmap and debug objects

[PATCH v2 0/2] vunmap and debug objects

[Chintan Pandya]

I'm not entirely sure, how debug objects are really
useful in vmalloc framework.

I'm assuming they are useful in some ways. So, there
are 2 issues in that. First patch is avoiding possible
race scenario and second patch passes _proper_ args
in debug object APIs. Both these patches can help
debug objects to be in consistent state.

We've observed some list corruptions in debug objects.
However, no claims that these patches will be fixing
them.

If one has an opinion that debug object has no use in
vmalloc framework, I would raise a patch to remove
them from the vunmap leg.

Below 2 patches are rebased over tip + my other patch in
review "[PATCH v2] mm: vmalloc: Clean up vunmap to avoid
pgtable ops twice"

Chintan Pandya (2):
  mm: vmalloc: Avoid racy handling of debugobjects in vunmap
  mm: vmalloc: Pass proper vm_start into debugobjects

>From V1->V2:
 - Incorporated Anshuman's comment about missing corrections
   in vm_unmap_ram()

 mm/vmalloc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project

[PATCH v2 1/2] mm: vmalloc: Avoid racy handling of debugobjects in vunmap

[Chintan Pandya]

Currently, __vunmap flow is,
 1) Release the VM area
 2) Free the debug objects corresponding to that vm area.

This leave some race window open.
 1) Release the VM area
 1.5) Some other client gets the same vm area
 1.6) This client allocates new debug objects on the same
      vm area
 2) Free the debug objects corresponding to this vm area.

Here, we actually free 'other' client's debug objects.

Fix this by freeing the debug objects first and then
releasing the VM area.

Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
---
 mm/vmalloc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 6729400..12d675c 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1500,7 +1500,7 @@ static void __vunmap(const void *addr, int deallocate_pages)
 			addr))
 		return;
 
-	area = remove_vm_area(addr);
+	area = find_vmap_area((unsigned long)addr)->vm;
 	if (unlikely(!area)) {
 		WARN(1, KERN_ERR "Trying to vfree() nonexistent vm area (%p)\n",
 				addr);
@@ -1510,6 +1510,7 @@ static void __vunmap(const void *addr, int deallocate_pages)
 	debug_check_no_locks_freed(addr, get_vm_area_size(area));
 	debug_check_no_obj_freed(addr, get_vm_area_size(area));
 
+	remove_vm_area(addr);
 	if (deallocate_pages) {
 		int i;
 
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project

[PATCH v2 2/2] mm: vmalloc: Pass proper vm_start into debugobjects

[Chintan Pandya]

Client can call vunmap with some intermediate 'addr'
which may not be the start of the VM area. Entire
unmap code works with vm->vm_start which is proper
but debug object API is called with 'addr'. This
could be a problem within debug objects.

Pass proper start address into debug object API.

Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
---
 mm/vmalloc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 12d675c..033c918 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1124,15 +1124,15 @@ void vm_unmap_ram(const void *mem, unsigned int count)
 	BUG_ON(addr > VMALLOC_END);
 	BUG_ON(!PAGE_ALIGNED(addr));
 
-	debug_check_no_locks_freed(mem, size);
-
 	if (likely(count <= VMAP_MAX_ALLOC)) {
+		debug_check_no_locks_freed(mem, size);
 		vb_free(mem, size);
 		return;
 	}
 
 	va = find_vmap_area(addr);
 	BUG_ON(!va);
+	debug_check_no_locks_freed(va->va_start, (va->va_end - va->va_start));
 	free_unmap_vmap_area(va);
 }
 EXPORT_SYMBOL(vm_unmap_ram);
@@ -1507,8 +1507,8 @@ static void __vunmap(const void *addr, int deallocate_pages)
 		return;
 	}
 
-	debug_check_no_locks_freed(addr, get_vm_area_size(area));
-	debug_check_no_obj_freed(addr, get_vm_area_size(area));
+	debug_check_no_locks_freed(area->addr, get_vm_area_size(area));
+	debug_check_no_obj_freed(area->addr, get_vm_area_size(area));
 
 	remove_vm_area(addr);
 	if (deallocate_pages) {
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project

Re: [PATCH v2 2/2] mm: vmalloc: Pass proper vm_start into debugobjects

[Andrew Morton]

On Tue, 17 Apr 2018 16:13:48 +0530 Chintan Pandya <cpandya@codeaurora.org> wrote:

> Client can call vunmap with some intermediate 'addr'
> which may not be the start of the VM area. Entire
> unmap code works with vm->vm_start which is proper
> but debug object API is called with 'addr'. This
> could be a problem within debug objects.

As far as I can tell this is indeed the case, but it's a pretty weird
thing for us to do.  I wonder if there is any code in the kernel which
is passing such an offset address into vunmap().  If so, perhaps we
should check for it and do a WARN_ONCE so it gets fixed.

> Pass proper start address into debug object API.
>
> ...
>
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1124,15 +1124,15 @@ void vm_unmap_ram(const void *mem, unsigned int count)
>  	BUG_ON(addr > VMALLOC_END);
>  	BUG_ON(!PAGE_ALIGNED(addr));
>  
> -	debug_check_no_locks_freed(mem, size);
> -
>  	if (likely(count <= VMAP_MAX_ALLOC)) {
> +		debug_check_no_locks_freed(mem, size);

But this still has the problem you described, no?  Shouldn't we be
doing yet another find_vmap_area()?

>  		vb_free(mem, size);
>  		return;
>  	}
>  
>  	va = find_vmap_area(addr);
>  	BUG_ON(!va);
> +	debug_check_no_locks_freed(va->va_start, (va->va_end - va->va_start));
>  	free_unmap_vmap_area(va);
>  }
>  EXPORT_SYMBOL(vm_unmap_ram);
> @@ -1507,8 +1507,8 @@ static void __vunmap(const void *addr, int deallocate_pages)
>  		return;
>  	}
>  
> -	debug_check_no_locks_freed(addr, get_vm_area_size(area));
> -	debug_check_no_obj_freed(addr, get_vm_area_size(area));
> +	debug_check_no_locks_freed(area->addr, get_vm_area_size(area));
> +	debug_check_no_obj_freed(area->addr, get_vm_area_size(area));
>  

Offtopic: it's a bit sad that __vunmap() does the find_vmap_area() then
calls remove_vm_area() which runs find_vmap_area() yet again...

Re: [PATCH v2 2/2] mm: vmalloc: Pass proper vm_start into debugobjects

[Chintan Pandya]

On 5/1/2018 4:34 AM, Andrew Morton wrote:
> should check for it and do a WARN_ONCE so it gets fixed.

Yes, that was an idea in discussion but I've been suggested that it
could be intentional. But since you are raising this, I will try to dig
once again and share a patch with WARN_ONCE if passing intermediate
'addr' is absolutely not right thing to do.

Chintan
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation
Collaborative Project

Re: [PATCH v2 2/2] mm: vmalloc: Pass proper vm_start into debugobjects

[Andrew Morton]

On Tue, 17 Apr 2018 16:13:48 +0530 Chintan Pandya <cpandya@codeaurora.org> wrote:

> Client can call vunmap with some intermediate 'addr'
> which may not be the start of the VM area. Entire
> unmap code works with vm->vm_start which is proper
> but debug object API is called with 'addr'. This
> could be a problem within debug objects.
> 
> Pass proper start address into debug object API.
> 
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1124,15 +1124,15 @@ void vm_unmap_ram(const void *mem, unsigned int count)
>  	BUG_ON(addr > VMALLOC_END);
>  	BUG_ON(!PAGE_ALIGNED(addr));
>  
> -	debug_check_no_locks_freed(mem, size);
> -
>  	if (likely(count <= VMAP_MAX_ALLOC)) {
> +		debug_check_no_locks_freed(mem, size);
>  		vb_free(mem, size);
>  		return;
>  	}
>  
>  	va = find_vmap_area(addr);
>  	BUG_ON(!va);
> +	debug_check_no_locks_freed(va->va_start, (va->va_end - va->va_start));
>  	free_unmap_vmap_area(va);
>  }
>  EXPORT_SYMBOL(vm_unmap_ram);

hm, how did this sneak through?

mm/vmalloc.c:1139:29: warning: passing argument 1 of debug_check_no_locks_freed makes pointer from integer without a cast [-Wint-conversion]
  debug_check_no_locks_freed(va->va_start, (va->va_end - va->va_start));

--- a/mm/vmalloc.c~mm-vmalloc-pass-proper-vm_start-into-debugobjects-fix
+++ a/mm/vmalloc.c
@@ -1136,7 +1136,8 @@ void vm_unmap_ram(const void *mem, unsig
 
 	va = find_vmap_area(addr);
 	BUG_ON(!va);
-	debug_check_no_locks_freed(va->va_start, (va->va_end - va->va_start));
+	debug_check_no_locks_freed((void *)va->va_start,
+				    (va->va_end - va->va_start));
 	free_unmap_vmap_area(va);
 }
 EXPORT_SYMBOL(vm_unmap_ram);

Re: [PATCH v2 2/2] mm: vmalloc: Pass proper vm_start into debugobjects

[Chintan Pandya]

On 5/4/2018 3:12 AM, Andrew Morton wrote:
> On Tue, 17 Apr 2018 16:13:48 +0530 Chintan Pandya <cpandya@codeaurora.org> wrote:
> 
>> Client can call vunmap with some intermediate 'addr'
>> which may not be the start of the VM area. Entire
>> unmap code works with vm->vm_start which is proper
>> but debug object API is called with 'addr'. This
>> could be a problem within debug objects.
>>
>> Pass proper start address into debug object API.
>>
>> --- a/mm/vmalloc.c
>> +++ b/mm/vmalloc.c
>> @@ -1124,15 +1124,15 @@ void vm_unmap_ram(const void *mem, unsigned int count)
>>   	BUG_ON(addr > VMALLOC_END);
>>   	BUG_ON(!PAGE_ALIGNED(addr));
>>   
>> -	debug_check_no_locks_freed(mem, size);
>> -
>>   	if (likely(count <= VMAP_MAX_ALLOC)) {
>> +		debug_check_no_locks_freed(mem, size);
>>   		vb_free(mem, size);
>>   		return;
>>   	}
>>   
>>   	va = find_vmap_area(addr);
>>   	BUG_ON(!va);
>> +	debug_check_no_locks_freed(va->va_start, (va->va_end - va->va_start));
>>   	free_unmap_vmap_area(va);
>>   }
>>   EXPORT_SYMBOL(vm_unmap_ram);
> 
> hm, how did this sneak through?
My bad. I had tested them but missed bringing these compile fixes to the
patch file. Will be careful next time.

> 
> mm/vmalloc.c:1139:29: warning: passing argument 1 of debug_check_no_locks_freed makes pointer from integer without a cast [-Wint-conversion]
>    debug_check_no_locks_freed(va->va_start, (va->va_end - va->va_start));
> 
> --- a/mm/vmalloc.c~mm-vmalloc-pass-proper-vm_start-into-debugobjects-fix
> +++ a/mm/vmalloc.c
> @@ -1136,7 +1136,8 @@ void vm_unmap_ram(const void *mem, unsig
>   
>   	va = find_vmap_area(addr);
>   	BUG_ON(!va);
> -	debug_check_no_locks_freed(va->va_start, (va->va_end - va->va_start));
> +	debug_check_no_locks_freed((void *)va->va_start,
> +				    (va->va_end - va->va_start));
>   	free_unmap_vmap_area(va);
>   }
>   EXPORT_SYMBOL(vm_unmap_ram);
> 

Chintan
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation
Collaborative Project