Re: [PATCH] mm: return -ENOMEM for page-table allocation failure in insert_pages()

[Lorenzo Stoakes (ARM)]

On Thu, Jul 30, 2026 at 10:13:23AM +0300, Avi Weiss wrote:
> walk_to_pmd() returns NULL only when p4d_alloc(), pud_alloc(), or
> pmd_alloc() fails. These are page-table allocation failures, but
> insert_pages() currently reports them as -EFAULT.
>
> Return -ENOMEM instead, consistent with the subsequent pte_alloc()
> failure and with the single-page insert_page() path, which reports
> failure of the same page-table allocation chain as -ENOMEM.
>
> Address and range validation failures in vm_insert_pages() continue to
> return -EFAULT. Keep the later -EFAULT return for
> pte_offset_map_lock(), which is not an allocation failure.
>
> Fixes: 8cd3984d81d5 ("mm/memory.c: add vm_insert_pages()")

Hmm :)

This isn't really a fix. Anybody relying on this returning -ENOMEM
vs. -EFAULT here is in a state of sin anyway (unless you can point to
specific users who are broken).

Drop the tag.

> Signed-off-by: Avi Weiss <thnkslprpt@xxxxxxxxx>

This is correct, walk

> ---
>  mm/memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index ff338c2abe92..d8eddca8e251 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2436,7 +2436,7 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,
>  	unsigned long pages_to_write_in_pmd;
>  	int ret;
>  more:
> -	ret = -EFAULT;
> +	ret = -ENOMEM;
>  	pmd = walk_to_pmd(mm, addr);

walk_to_pmd() is horribly named, it's allocating... populate_to_pmd() would
be better can you rename it?

>  	if (!pmd)
>  		goto out;

Looking down:

	pages_to_write_in_pmd = min_t(unsigned long,
		remaining_pages_total, PTRS_PER_PTE - pte_index(addr));

	/* Allocate the PTE if necessary; takes PMD lock once only. */
	ret = -ENOMEM; <------------------------------ set it again?
	if (pte_alloc(mm, pmd))
		goto out;

The way this function is written is horrible in general, I hate 'preset default
return value' as a pattern.

So could you instead change it so the ret is set at the point of error, and
while you're at it rename ret to err, e.g.:

	int err = 0;

	...

	pmd = populate_to_pmd(mm, addr);
	if (!pmd) {
		err = -ENOMEM;
		goto out;
	}

etc.

Also ignoring pte_alloc()'s error code (which will be -ENOMEM anyway) and
setting manually is stupid further down so:

-	ret = -ENOMEM;
-	if (pte_alloc(mm, pmd))
-		goto out;
+	err = pte_alloc(mm, pmd);
+	if (err)
+		goto out;

Obviously:

		for (pte = start_pte; pte_idx < batch_size; ++pte, ++pte_idx) {
			int err = insert_page_in_batch_locked(vma, pte,
				addr, pages[curr_page_idx], prot);

->

		for (pte = start_pte; pte_idx < batch_size; ++pte, ++pte_idx) {
			err = ...

Remove horrible ret = err and ret = 0 assignment later.

All this would improve it a lot thanks! :)

> --
> 2.43.0
>

Cheers, Lorenzo