David Hildenbrand david@redhat.com wrote:
On 21.11.24 14:44, David Hildenbrand wrote:
On 21.11.24 13:41, Jeongjun Park wrote:
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().
Cc: stable@vger.kernel.org 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?
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
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