mm/huge_memory: Fix to make vma_adjust_trans_huge() use find_vma() correctly
Subsystems (3)
PATCH 1/1 mm/huge_memory: Fix to make vma_adjust_trans_huge() use find_vma() correctly
vma_adjust_trans_huge() uses find_vma() to get the VMA, but find_vma() uses the returned pointer without any verification, even though it may return NULL. In this case, NULL pointer dereference may occur, so to prevent this, vma_adjust_trans_huge() should be fix to verify the return value of find_vma().
--- mm/huge_memory.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
mm/huge_memory.c +6 -3
@@ -2941,9 +2941,12 @@ void vma_adjust_trans_huge(struct vm_area_struct *vma,*/ if (adjust_next >0 ) {struct vm_area_struct *next =find_vma (vma->vm_mm, vma->vm_end); -unsigned long nstart = next->vm_start; - nstart += adjust_next; -split_huge_pmd_if_needed (next, nstart); + +if (likely (next)) { +unsigned long nstart = next->vm_start; + nstart += adjust_next; +split_huge_pmd_if_needed (next, nstart); + } } }
--
Quoted 7 lines · Jeongjun Park
vma_adjust_trans_huge()usesfind_vma()to get the VMA, butfind_vma()uses the returned pointer without any verification, even though it may return NULL. In this case, NULL pointer dereference may occur, so to prevent this,vma_adjust_trans_huge()should be fix to verify the return value offind_vma().Cc <[email protected]>Fixes 685405020b9f ("mm/khugepaged: stop using vma linked list")
If that's an issue, wouldn't it have predated that commit?
struct vm_area_struct *next = vma->vm_next;
unsigned long nstart = next->vm_start;
Would have also assumed that there is a next VMA that can be dereferenced, no?
Signature · 3 lines
Cheers, David / dhildenb
Quoted 9 lines · David Hildenbrand
Quoted 7 lines · Jeongjun Park
vma_adjust_trans_huge()usesfind_vma()to get the VMA, butfind_vma()uses the returned pointer without any verification, even though it may return NULL. In this case, NULL pointer dereference may occur, so to prevent this,vma_adjust_trans_huge()should be fix to verify the return value offind_vma().Cc <[email protected]>Fixes 685405020b9f ("mm/khugepaged: stop using vma linked list")
Quoted 8 lines
If that's an issue, wouldn't it have predated that commit?
struct
vm_area_struct*next =vma->vm_next;
unsigned long nstart =next->vm_start;Would have also assumed that there is a next VMA that can be dereferenced, no?
And looking into the details, we only assume that there is a next VMA if we are explicitly told to by the caller of vma_adjust_trans_huge() using "adjust_next".
There is only one such caller, vma_merge_existing_range()->commit_merge() where we set adj_start -> "adjust_next" where we seem to have a guarantee that there is a next VMA.
So I don't think there is an issue here (although the code does look confusing ...).
Not sure, though, if a
if (WARN_ON_ONCE(!next))
return;
would be reasonable.
Signature · 3 lines
Cheers, David / dhildenb
Quoted 20 lines · David Hildenbrand
Quoted 9 lines · David Hildenbrand
Quoted 7 lines · Jeongjun Park
vma_adjust_trans_huge()usesfind_vma()to get the VMA, butfind_vma()uses the returned pointer without any verification, even though it may return NULL. In this case, NULL pointer dereference may occur, so to prevent this,vma_adjust_trans_huge()should be fix to verify the return value offind_vma().Cc <[email protected]>Fixes 685405020b9f ("mm/khugepaged: stop using vma linked list")Quoted 8 lines
If that's an issue, wouldn't it have predated that commit?
struct
vm_area_struct*next =vma->vm_next;
unsigned long nstart =next->vm_start;Would have also assumed that there is a next VMA that can be dereferenced, no?
Quoted 7 lines
And looking into the details, we only assume that there is a next VMA if we are explicitly told to by the caller of
vma_adjust_trans_huge()using "adjust_next".There is only one such caller, vma_merge_existing_range()->commit_merge() where we set
adj_start-> "adjust_next" where we seem to have a guarantee that there is a next VMA.
I also thought that it would not be a problem in general cases, but I think that there may be a special case (for example, a race condition...?) that can occur in certain conditions, although I have not found it yet.
In addition, most functions except this one unconditionally check the return value of find_vma(), so I think it would be better to handle the return value of find_vma() consistently in this function as well, rather than taking the risk and leaving it alone just because it seems to be okay.
Regards,
Jeongjun Park
Quoted 16 lines
So I don't think there is an issue here (although the code does look confusing ...).
Not sure, though, if a
if (
WARN_ON_ONCE(!next))return;would be reasonable.
--
Cheers,David / dhildenb
Quoted 25 lines · Jeongjun Park
Quoted 24 lines · David Hildenbrand
Quoted 9 lines · David Hildenbrand
Quoted 7 lines · Jeongjun Park
vma_adjust_trans_huge()usesfind_vma()to get the VMA, butfind_vma()uses the returned pointer without any verification, even though it may return NULL. In this case, NULL pointer dereference may occur, so to prevent this,vma_adjust_trans_huge()should be fix to verify the return value offind_vma().Cc <[email protected]>Fixes 685405020b9f ("mm/khugepaged: stop using vma linked list")Quoted 8 lines
If that's an issue, wouldn't it have predated that commit?
struct
vm_area_struct*next =vma->vm_next;
unsigned long nstart =next->vm_start;Would have also assumed that there is a next VMA that can be dereferenced, no?
And looking into the details, we only assume that there is a next VMA if we are explicitly told to by the caller of
vma_adjust_trans_huge()using "adjust_next".
Quoted 7 lines
Quoted 3 lines
There is only one such caller, vma_merge_existing_range()->commit_merge() where we set
adj_start-> "adjust_next" where we seem to have a guarantee that there is a next VMA.I also thought that it would not be a problem in general cases, but I think that there may be a special case (for example, a race condition...?) that can occur in certain conditions, although I have not found it yet.
If we're working on VMAs in that way (merging!) we need the mmap lock in write mode, so no races are possible.
Quoted 5 lines
In addition, most functions except this one unconditionally check the return value of
find_vma(), so I think it would be better to handle the return value offind_vma()consistently in this function as well, rather than taking the risk and leaving it alone just because it seems to be okay.
Your patch is silently hiding something that should never happen such that we wouldn't handle our operation as intended. So no, that's even worse.
Signature · 3 lines
Cheers, David / dhildenb
Quoted 4 lines · Jeongjun Park
vma_adjust_trans_huge()usesfind_vma()to get the VMA, butfind_vma()uses the returned pointer without any verification, even though it may return NULL. In this case, NULL pointer dereference may occur, so to prevent this,vma_adjust_trans_huge()should be fix to verify the return value offind_vma().
I don't think we should change anything here.