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")
Signed-off-by: Jeongjun Park <aha310510@gmail.com>
---
 mm/huge_memory.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 5734d5d5060f..db55b8abae2e 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -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);
+		}
 	}
 }
 
--

On Thu, Nov 21, 2024 at 09:41:13PM +0900, 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().

I don't think we should change anything here.

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?

-- 
Cheers,

David / dhildenb

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.

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

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
>

On 21.11.24 15:18, Jeongjun Park wrote:
> 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.

If we're working on VMAs in that way (merging!) we need the mmap lock in 
write mode, so no races are possible.

> 
> 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.

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.

-- 
Cheers,

David / dhildenb