* [PATCH 0/2] vunmap and debug objects @ 2018-04-13 11:33 Chintan Pandya 2018-04-13 11:33 ` [PATCH 1/2] mm: vmalloc: Avoid racy handling of debugobjects in vunmap Chintan Pandya 2018-04-13 11:33 ` [PATCH 2/2] mm: vmalloc: Pass proper vm_start into debugobjects Chintan Pandya 0 siblings, 2 replies; 7+ messages in thread From: Chintan Pandya @ 2018-04-13 11:33 UTC (permalink / raw) To: vbabka, labbott, catalin.marinas, hannes, f.fainelli, xieyisheng1, ard.biesheuvel, richard.weiyang, byungchul.park Cc: linux-mm, linux-kernel, 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. Chintan Pandya (2): mm: vmalloc: Avoid racy handling of debugobjects in vunmap mm: vmalloc: Pass proper vm_start into debugobjects mm/vmalloc.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] mm: vmalloc: Avoid racy handling of debugobjects in vunmap 2018-04-13 11:33 [PATCH 0/2] vunmap and debug objects Chintan Pandya @ 2018-04-13 11:33 ` Chintan Pandya 2018-04-13 11:33 ` [PATCH 2/2] mm: vmalloc: Pass proper vm_start into debugobjects Chintan Pandya 1 sibling, 0 replies; 7+ messages in thread From: Chintan Pandya @ 2018-04-13 11:33 UTC (permalink / raw) To: vbabka, labbott, catalin.marinas, hannes, f.fainelli, xieyisheng1, ard.biesheuvel, richard.weiyang, byungchul.park Cc: linux-mm, linux-kernel, 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 ebff729..9ff21a1 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -1519,7 +1519,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); @@ -1529,6 +1529,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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] mm: vmalloc: Pass proper vm_start into debugobjects 2018-04-13 11:33 [PATCH 0/2] vunmap and debug objects Chintan Pandya 2018-04-13 11:33 ` [PATCH 1/2] mm: vmalloc: Avoid racy handling of debugobjects in vunmap Chintan Pandya @ 2018-04-13 11:33 ` Chintan Pandya 2018-04-13 12:01 ` Anshuman Khandual 1 sibling, 1 reply; 7+ messages in thread From: Chintan Pandya @ 2018-04-13 11:33 UTC (permalink / raw) To: vbabka, labbott, catalin.marinas, hannes, f.fainelli, xieyisheng1, ard.biesheuvel, richard.weiyang, byungchul.park Cc: linux-mm, linux-kernel, 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 | 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] mm: vmalloc: Pass proper vm_start into debugobjects 2018-04-13 11:33 ` [PATCH 2/2] mm: vmalloc: Pass proper vm_start into debugobjects Chintan Pandya @ 2018-04-13 12:01 ` Anshuman Khandual 2018-04-16 12:09 ` Chintan Pandya 0 siblings, 1 reply; 7+ messages in thread From: Anshuman Khandual @ 2018-04-13 12:01 UTC (permalink / raw) To: Chintan Pandya, vbabka, labbott, catalin.marinas, hannes, f.fainelli, xieyisheng1, ard.biesheuvel, richard.weiyang, byungchul.park Cc: linux-mm, linux-kernel 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 <cpandya@codeaurora.org> > --- > 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] mm: vmalloc: Pass proper vm_start into debugobjects 2018-04-13 12:01 ` Anshuman Khandual @ 2018-04-16 12:09 ` Chintan Pandya 2018-04-17 3:09 ` Anshuman Khandual 0 siblings, 1 reply; 7+ messages in thread From: Chintan Pandya @ 2018-04-16 12:09 UTC (permalink / raw) To: Anshuman Khandual, vbabka, labbott, catalin.marinas, hannes, f.fainelli, xieyisheng1, ard.biesheuvel, richard.weiyang, byungchul.park Cc: linux-mm, linux-kernel 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 <cpandya@codeaurora.org> >> --- >> 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] mm: vmalloc: Pass proper vm_start into debugobjects 2018-04-16 12:09 ` Chintan Pandya @ 2018-04-17 3:09 ` Anshuman Khandual 2018-04-17 5:10 ` Chintan Pandya 0 siblings, 1 reply; 7+ messages in thread From: Anshuman Khandual @ 2018-04-17 3:09 UTC (permalink / raw) To: Chintan Pandya, Anshuman Khandual, vbabka, labbott, catalin.marinas, hannes, f.fainelli, xieyisheng1, ard.biesheuvel, richard.weiyang, byungchul.park Cc: linux-mm, linux-kernel 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 <cpandya@codeaurora.org> >>> --- >>> 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] mm: vmalloc: Pass proper vm_start into debugobjects 2018-04-17 3:09 ` Anshuman Khandual @ 2018-04-17 5:10 ` Chintan Pandya 0 siblings, 0 replies; 7+ messages in thread From: Chintan Pandya @ 2018-04-17 5:10 UTC (permalink / raw) To: Anshuman Khandual, vbabka, labbott, catalin.marinas, hannes, f.fainelli, xieyisheng1, ard.biesheuvel, richard.weiyang, byungchul.park Cc: linux-mm, linux-kernel 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 <cpandya@codeaurora.org> >>>> --- >>>> 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-04-17 5:11 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-13 11:33 [PATCH 0/2] vunmap and debug objects Chintan Pandya 2018-04-13 11:33 ` [PATCH 1/2] mm: vmalloc: Avoid racy handling of debugobjects in vunmap Chintan Pandya 2018-04-13 11:33 ` [PATCH 2/2] mm: vmalloc: Pass proper vm_start into debugobjects Chintan Pandya 2018-04-13 12:01 ` Anshuman Khandual 2018-04-16 12:09 ` Chintan Pandya 2018-04-17 3:09 ` Anshuman Khandual 2018-04-17 5:10 ` Chintan Pandya
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.