[PATCH v2 0/3] mm: improve insert_pages() error handling

[PATCH v2 0/3] mm: improve insert_pages() error handling

[Avi Weiss]

Improve insert_pages() error handling following review of the minor
cleanup in v1.

Rename the helper that populates the upper page-table levels, set error
codes at their points of failure, and return -ENOMEM when that
population fails.

Thanks for the review. I'm new to Linux; most of my previous upstream
work has been on NASA cFS and RTEMS.

I agree that initializing the return value to zero and assigning each
relevant error code at its failure site is clearer and more idiomatic.
The second (middle) patch implements that cleanup.

The pte_alloc() macro returns a boolean, so patch 2 tests it and assigns
-ENOMEM explicitly rather than using its return value as an error code.

Changes in v2:

* rename walk_to_pmd() to populate_to_pmd()
* set error codes at their points of failure and rename ret to err
* split the cleanup and return-code change into separate patches
* drop the Fixes tag because this is a general cleanup

Avi Weiss (3):
  mm/memory.c: rename walk_to_pmd() to populate_to_pmd()
  mm/memory.c: simplify error handling in insert_pages()
  mm: return -ENOMEM for page-table allocation failure in insert_pages()

 mm/memory.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)


base-commit: 848acc8ffe1b7cd5f1bf427b93069becfebc2c9d
-- 
2.43.0

[PATCH v2 1/3] mm/memory.c: rename walk_to_pmd() to populate_to_pmd()

[Avi Weiss]

This helper allocates any missing page-table levels (it doesn't just
'walk'. Rename for clarity.

Signed-off-by: Avi Weiss <thnkslprpt@gmail.com>
---
 mm/memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index ff338c2abe92..8658feba8be9 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2268,7 +2268,7 @@ void zap_special_vma_range(struct vm_area_struct *vma, unsigned long address,
 }
 EXPORT_SYMBOL_GPL(zap_special_vma_range);
 
-static pmd_t *walk_to_pmd(struct mm_struct *mm, unsigned long addr)
+static pmd_t *populate_to_pmd(struct mm_struct *mm, unsigned long addr)
 {
 	pgd_t *pgd;
 	p4d_t *p4d;
@@ -2293,7 +2293,7 @@ static pmd_t *walk_to_pmd(struct mm_struct *mm, unsigned long addr)
 pte_t *get_locked_pte(struct mm_struct *mm, unsigned long addr,
 		      spinlock_t **ptl)
 {
-	pmd_t *pmd = walk_to_pmd(mm, addr);
+	pmd_t *pmd = populate_to_pmd(mm, addr);
 
 	if (!pmd)
 		return NULL;
@@ -2437,7 +2437,7 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,
 	int ret;
 more:
 	ret = -EFAULT;
-	pmd = walk_to_pmd(mm, addr);
+	pmd = populate_to_pmd(mm, addr);
 	if (!pmd)
 		goto out;
 
-- 
2.43.0

[PATCH v2 2/3] mm/memory.c: simplify error handling in insert_pages()

[Avi Weiss]

Initialize error return status to zero and then set it as needed at each
point of failure.

Assign -ENOMEM explicitly when pte_alloc() fails as the pte_alloc()
macro returns a boolean.

Signed-off-by: Avi Weiss <thnkslprpt@gmail.com>
---
 mm/memory.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 8658feba8be9..11bb4fb98761 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2434,20 +2434,22 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,
 	unsigned long curr_page_idx = 0;
 	unsigned long remaining_pages_total = *num;
 	unsigned long pages_to_write_in_pmd;
-	int ret;
+	int err = 0;
 more:
-	ret = -EFAULT;
 	pmd = populate_to_pmd(mm, addr);
-	if (!pmd)
+	if (!pmd) {
+		err = -EFAULT;
 		goto out;
+	}
 
 	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;
-	if (pte_alloc(mm, pmd))
+	if (pte_alloc(mm, pmd)) {
+		err = -ENOMEM;
 		goto out;
+	}
 
 	while (pages_to_write_in_pmd) {
 		int pte_idx = 0;
@@ -2455,15 +2457,14 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,
 
 		start_pte = pte_offset_map_lock(mm, pmd, addr, &pte_lock);
 		if (!start_pte) {
-			ret = -EFAULT;
+			err = -EFAULT;
 			goto out;
 		}
 		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);
+			err = insert_page_in_batch_locked(vma, pte, addr,
+							  pages[curr_page_idx], prot);
 			if (unlikely(err)) {
 				pte_unmap_unlock(start_pte, pte_lock);
-				ret = err;
 				remaining_pages_total -= pte_idx;
 				goto out;
 			}
@@ -2476,10 +2477,9 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,
 	}
 	if (remaining_pages_total)
 		goto more;
-	ret = 0;
 out:
 	*num = remaining_pages_total;
-	return ret;
+	return err;
 }
 
 /**
-- 
2.43.0

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

[Avi Weiss]

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

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 11bb4fb98761..16093479d341 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2438,7 +2438,7 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,
 more:
 	pmd = populate_to_pmd(mm, addr);
 	if (!pmd) {
-		err = -EFAULT;
+		err = -ENOMEM;
 		goto out;
 	}
 
-- 
2.43.0