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

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

[Avi Weiss]

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()")
Signed-off-by: Avi Weiss <thnkslprpt@gmail.com>
---
 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);
 	if (!pmd)
 		goto out;
-- 
2.43.0

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@gmail.com>

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

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

[Lorenzo Stoakes (ARM)]

On Thu, Jul 30, 2026 at 08:42:55AM +0100, Lorenzo Stoakes (ARM) wrote:
> 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@gmail.com>
>
> This is correct, walk

Ugh incomplete thought :)

->

This is correct, the 'walk' can only fail due to failure to allocate.

Anyway I gave you a bunch of stuff to do here, respin once you've addressed all
that.

Cheers, Lorenzo

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

[David Hildenbrand (Arm)]

On 7/30/26 09:42, Lorenzo Stoakes (ARM) wrote:
> 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).

Agreed, I am missing the problem here?

-- 
Cheers,

David

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

[Lorenzo Stoakes (ARM)]

On Thu, Jul 30, 2026 at 10:27:20AM +0200, David Hildenbrand (Arm) wrote:
> On 7/30/26 09:42, Lorenzo Stoakes (ARM) wrote:
> > 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).
>
> Agreed, I am missing the problem here?

Seems just a cleanup, I gave Avi some ideas ;) this function could do with
it.

>
> --
> Cheers,
>
> David

Cheers, Lorenzo

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

[Andrew Morton]

On Thu, 30 Jul 2026 08:42:49 +0100 "Lorenzo Stoakes (ARM)" <ljs@kernel.org> wrote:

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

It used to be the preferred way because 

	ret = -ENOMEM;
	if (expr)
		goto out;

generated slightly better code than

	if (expr) {
		ret = -ENOMEM;
		goto out;
	}

Whether that is the case with current compilers I don't know.

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

[Lorenzo Stoakes (ARM)]

On Thu, Jul 30, 2026 at 09:15:58AM -0700, Andrew Morton wrote:
> On Thu, 30 Jul 2026 08:42:49 +0100 "Lorenzo Stoakes (ARM)" <ljs@kernel.org> wrote:
>
> > 	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.
>
> It used to be the preferred way because
>
> 	ret = -ENOMEM;
> 	if (expr)
> 		goto out;
>
> generated slightly better code than
>
> 	if (expr) {
> 		ret = -ENOMEM;
> 		goto out;
> 	}
>
> Whether that is the case with current compilers I don't know.

Thanks for the background on that!

I'd put money on there being no difference now :) but in any case that kind of
micro-optimisation isn't really a concern here even if it was the case.

--
Cheers, Lorenzo