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

[Chintan Pandya]

On 4/17/2018 8:39 AM, Anshuman Khandual wrote:

On 04/16/2018 05:39 PM, Chintan Pandya wrote:



On 4/13/2018 5:31 PM, Anshuman Khandual wrote:

On 04/13/2018 05:03 PM, Chintan Pandya 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.

Signed-off-by: Chintan Pandya 
---
   mm/vmalloc.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 9ff21a1..28034c55 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1526,8 +1526,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));


This kind of makes sense to me but I am not sure. We also have another
instance of this inside the function vm_unmap_ram() where we call for

Right, I missed it. I plan to add below stub in v2.

--- 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);


It should have been 'va->va_start' instead of 'mem' in here but as
said before it looks correct to me but I am not really sure.


vb_free() doesn't honor va->va_start. If mem is not va_start and
deliberate, one will provide proper size. And that should be okay
to do as per the code. So, I don't think this particular debug_check
should have passed va_start in args.





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 2/2] mm: vmalloc: Pass proper vm_start into debugobjects

[Chintan Pandya]

On 4/17/2018 8:39 AM, Anshuman Khandual wrote:

On 04/16/2018 05:39 PM, Chintan Pandya wrote:



On 4/13/2018 5:31 PM, Anshuman Khandual wrote:

On 04/13/2018 05:03 PM, Chintan Pandya 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.

Signed-off-by: Chintan Pandya 
---
   mm/vmalloc.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 9ff21a1..28034c55 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1526,8 +1526,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));


This kind of makes sense to me but I am not sure. We also have another
instance of this inside the function vm_unmap_ram() where we call for

Right, I missed it. I plan to add below stub in v2.

--- 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);


It should have been 'va->va_start' instead of 'mem' in here but as
said before it looks correct to me but I am not really sure.


vb_free() doesn't honor va->va_start. If mem is not va_start and
deliberate, one will provide proper size. And that should be okay
to do as per the code. So, I don't think this particular debug_check
should have passed va_start in args.





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 2/2] mm: vmalloc: Pass proper vm_start into debugobjects

[Anshuman Khandual]

On 04/16/2018 05:39 PM, Chintan Pandya wrote:
> 
> 
> On 4/13/2018 5:31 PM, Anshuman Khandual wrote:
>> On 04/13/2018 05:03 PM, Chintan Pandya 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.
>>>
>>> Signed-off-by: Chintan Pandya 
>>> ---
>>>   mm/vmalloc.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>>> index 9ff21a1..28034c55 100644
>>> --- a/mm/vmalloc.c
>>> +++ b/mm/vmalloc.c
>>> @@ -1526,8 +1526,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));
>>
>> This kind of makes sense to me but I am not sure. We also have another
>> instance of this inside the function vm_unmap_ram() where we call for
> Right, I missed it. I plan to add below stub in v2.
> 
> --- 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);

It should have been 'va->va_start' instead of 'mem' in here but as
said before it looks correct to me but I am not really sure.

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

[Anshuman Khandual]

On 04/16/2018 05:39 PM, Chintan Pandya wrote:
> 
> 
> On 4/13/2018 5:31 PM, Anshuman Khandual wrote:
>> On 04/13/2018 05:03 PM, Chintan Pandya 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.
>>>
>>> Signed-off-by: Chintan Pandya 
>>> ---
>>>   mm/vmalloc.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>>> index 9ff21a1..28034c55 100644
>>> --- a/mm/vmalloc.c
>>> +++ b/mm/vmalloc.c
>>> @@ -1526,8 +1526,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));
>>
>> This kind of makes sense to me but I am not sure. We also have another
>> instance of this inside the function vm_unmap_ram() where we call for
> Right, I missed it. I plan to add below stub in v2.
> 
> --- 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);

It should have been 'va->va_start' instead of 'mem' in here but as
said before it looks correct to me but I am not really sure.

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

[Chintan Pandya]

On 4/13/2018 5:31 PM, Anshuman Khandual wrote:

On 04/13/2018 05:03 PM, Chintan Pandya 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.

Signed-off-by: Chintan Pandya 
---
  mm/vmalloc.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 9ff21a1..28034c55 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1526,8 +1526,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));


This kind of makes sense to me but I am not sure. We also have another
instance of this inside the function vm_unmap_ram() where we call for

Right, I missed it. I plan to add below stub in v2.

--- 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);



debug on locks without even finding the vmap_area first. But it is true
that in both these functions the vmap_area gets freed eventually. Hence
the entire mapping [va->va_start --> va->va_end] gets unmapped. Sounds
like these debug functions should have the entire range as argument.
But I am not sure and will seek Michal's input on this.



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 2/2] mm: vmalloc: Pass proper vm_start into debugobjects

[Chintan Pandya]

On 4/13/2018 5:31 PM, Anshuman Khandual wrote:

On 04/13/2018 05:03 PM, Chintan Pandya 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.

Signed-off-by: Chintan Pandya 
---
  mm/vmalloc.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 9ff21a1..28034c55 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1526,8 +1526,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));


This kind of makes sense to me but I am not sure. We also have another
instance of this inside the function vm_unmap_ram() where we call for

Right, I missed it. I plan to add below stub in v2.

--- 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);



debug on locks without even finding the vmap_area first. But it is true
that in both these functions the vmap_area gets freed eventually. Hence
the entire mapping [va->va_start --> va->va_end] gets unmapped. Sounds
like these debug functions should have the entire range as argument.
But I am not sure and will seek Michal's input on this.



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 2/2] mm: vmalloc: Pass proper vm_start into debugobjects

[Anshuman Khandual]

On 04/13/2018 05:03 PM, Chintan Pandya 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.
> 
> Signed-off-by: Chintan Pandya 
> ---
>  mm/vmalloc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 9ff21a1..28034c55 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1526,8 +1526,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));

This kind of makes sense to me but I am not sure. We also have another
instance of this inside the function vm_unmap_ram() where we call for
debug on locks without even finding the vmap_area first. But it is true
that in both these functions the vmap_area gets freed eventually. Hence
the entire mapping [va->va_start --> va->va_end] gets unmapped. Sounds
like these debug functions should have the entire range as argument.
But I am not sure and will seek Michal's input on this.

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

[Anshuman Khandual]

On 04/13/2018 05:03 PM, Chintan Pandya 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.
> 
> Signed-off-by: Chintan Pandya 
> ---
>  mm/vmalloc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 9ff21a1..28034c55 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1526,8 +1526,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));

This kind of makes sense to me but I am not sure. We also have another
instance of this inside the function vm_unmap_ram() where we call for
debug on locks without even finding the vmap_area first. But it is true
that in both these functions the vmap_area gets freed eventually. Hence
the entire mapping [va->va_start --> va->va_end] gets unmapped. Sounds
like these debug functions should have the entire range as argument.
But I am not sure and will seek Michal's input on this.

[PATCH 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 
---
 mm/vmalloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 9ff21a1..28034c55 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1526,8 +1526,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

[PATCH 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 
---
 mm/vmalloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 9ff21a1..28034c55 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1526,8 +1526,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