[v1] lib/test_hmm: Check alloc_page_vma() return value

[PATCH] lib/test_hmm: Check alloc_page_vma() return value

Posted by liuqiangneo@163.com 4 weeks, 1 day ago

From: Qiang Liu <liuqiang@kylinos.cn>

Return VM_FAULT_OOM if page allocation fails, which
avoids a NULL pointer dereference when calling lock_page().

Signed-off-by: Qiang Liu <liuqiang@kylinos.cn>
---
 lib/test_hmm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/test_hmm.c b/lib/test_hmm.c
index 213504915737..f8b43d6eb261 100644
--- a/lib/test_hmm.c
+++ b/lib/test_hmm.c
@@ -1063,6 +1063,8 @@ static vm_fault_t dmirror_devmem_fault_alloc_and_copy(struct migrate_vma *args,
 			/* Try with smaller pages if large allocation fails */
 			if (!dpage && order) {
 				dpage = alloc_page_vma(GFP_HIGHUSER_MOVABLE, args->vma, addr);
+				if (!dpage)
+					return VM_FAULT_OOM;
 				lock_page(dpage);
 				dst[i] = migrate_pfn(page_to_pfn(dpage));
 				dst_page = pfn_to_page(page_to_pfn(dpage));
-- 
2.43.0

Re: [PATCH] lib/test_hmm: Check alloc_page_vma() return value

Posted by Alistair Popple 4 weeks ago

On 2026-05-14 at 13:23 +1000, liuqiangneo@163.com wrote...
> From: Qiang Liu <liuqiang@kylinos.cn>
> 
> Return VM_FAULT_OOM if page allocation fails, which
> avoids a NULL pointer dereference when calling lock_page().

Thanks, I agree the NULL dereference is a bug and this avoids it but what
happens to the pages that may have already been allocated and locked in previous
iterations of the loop? I think the subsequent migrate_vma_pages()/finalize()
calls will do the correct thing, but that would lead to a partial migration.

Given that's not what we're explicitly testing here I think it would be better
to just unlock and free the previously allocated pages before returning.

 - Alistair
 
> Signed-off-by: Qiang Liu <liuqiang@kylinos.cn>
> ---
>  lib/test_hmm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/test_hmm.c b/lib/test_hmm.c
> index 213504915737..f8b43d6eb261 100644
> --- a/lib/test_hmm.c
> +++ b/lib/test_hmm.c
> @@ -1063,6 +1063,8 @@ static vm_fault_t dmirror_devmem_fault_alloc_and_copy(struct migrate_vma *args,
>  			/* Try with smaller pages if large allocation fails */
>  			if (!dpage && order) {
>  				dpage = alloc_page_vma(GFP_HIGHUSER_MOVABLE, args->vma, addr);
> +				if (!dpage)
> +					return VM_FAULT_OOM;
>  				lock_page(dpage);
>  				dst[i] = migrate_pfn(page_to_pfn(dpage));
>  				dst_page = pfn_to_page(page_to_pfn(dpage));
> -- 
> 2.43.0
> 
>