Re: [PATCH] mm/huge_memory: Fix to make vma_adjust_trans_huge() use find_vma() correctly

[Jeongjun Park <aha310510@gmail.com>]

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
>