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

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

[liuqiangneo]

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>
---
v2:
- Add unlock and free allocated pages before return.
- https://lore.kernel.org/all/20260514032345.32256-1-liuqiangneo@163.com/
---
 lib/test_hmm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lib/test_hmm.c b/lib/test_hmm.c
index 213504915737..90aec4ca2e01 100644
--- a/lib/test_hmm.c
+++ b/lib/test_hmm.c
@@ -1063,6 +1063,17 @@ 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) {
+					/* Unlock and free pages already allocated. */
+					while (i > 0) {
+						struct page *fpage;
+
+						fpage = migrate_pfn_to_page(dst[--i]);
+						unlock_page(fpage);
+						__free_page(fpage);
+					}
+					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 v2] lib/test_hmm: Check alloc_page_vma() return value

[Andrew Morton]

On Mon, 18 May 2026 11:19:03 +0800 liuqiangneo@163.com wrote:

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

Patch looks good to me, but AI review is saying that two of
the calling functions are junk, and should be taught to handle
callee errors:
	https://sashiko.dev/#/patchset/20260518031903.63455-1-liuqiangneo@163.com

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

[Alistair Popple]

On 2026-05-18 at 13:19 +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().
> 
> Signed-off-by: Qiang Liu <liuqiang@kylinos.cn>
> ---
> v2:
> - Add unlock and free allocated pages before return.
> - https://lore.kernel.org/all/20260514032345.32256-1-liuqiangneo@163.com/
> ---
>  lib/test_hmm.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/lib/test_hmm.c b/lib/test_hmm.c
> index 213504915737..90aec4ca2e01 100644
> --- a/lib/test_hmm.c
> +++ b/lib/test_hmm.c
> @@ -1063,6 +1063,17 @@ 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) {
> +					/* Unlock and free pages already allocated. */
> +					while (i > 0) {
> +						struct page *fpage;

You will need to clear dst[i] as well to avoid subsequent calls to
migrate_vma_pages/finalize() trying to continue with the migration. But then the
whole error handling for this function already seems to be broken because not
all callers do this :-)

Looking at the two callers for this, one ignores the return code
(dmirror_migrate_to_system) and the other (dmirror_devmem_fault) gets the
error handling wrong because it skips calling migrate_vma_finalize() if
dmirror_devmem_fault_alloc_and_copy() failed.

What needs to happen is we propagate the error up to the test so the test fails,
but we still need to call migrate_vma_finalize() to remove the migration entries
created in the migrate_vma_setup() step.

 - Alistair

> +						fpage = migrate_pfn_to_page(dst[--i]);
> +						unlock_page(fpage);
> +						__free_page(fpage);
> +					}
> +					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
> 
>