Re: [PATCH] s390/mm: Fix fail path in crst_table_upgrade()

On Wed, Aug 19, 2026 at 01:22:50PM +0200, Alexander Gordeev wrote:
> On Tue, Aug 11, 2026 at 04:12:03PM +0200, Heiko Carstens wrote:
> > Imho this makes the code even more uglier than it is/was. This is also yet
> > another bug which is/was only possible after loop unrolling happened with
> > [1]. Another bug, which would not have been possible without the rework back
> > then, is [2].
> > 
> > So I would _much_ rather prefer to go back to what we had with [1], of course
> > including the additional dtor/ctor code. But that code was much more readable
> > compared to what we have now, especially considering the additional fix which
> > is proposed now.
> 
> Frankly, to me personally the old version is more difficult to read.
> In addition, in case of 3-5 upgrade on 4-5 allocation failure it has
> (a) 4-level table leak and

Which leak? The old code never had leak where the allocated 4-level table
would become unreachable (== leaked).

> (b) useless 3-4 upgrade.

This was an intentional design choice to keep the code simple.

> Should all of those get fixed, I doubt the end result will look as
> simple as the original version. But I can try if you insist.

So "all of that" :) is at most b). But it really doesn't make any sense to do
a rollback, simply because all of this is completely irrelevant for production
kernels. Order-2 allocations can only fail with error injection, otherwise
they never fail.

Hence the partial upgrade is fine - this is not a leak, but a partial proper
upgrade of the address space size of the current task, with proper freeing of
the page table (no leak) on process exit.

Note: the below keeps the oddity that pagetable_p4d_ctor() is called in case
of only a 4-level upgrade, even though in such a case pagetable_pgd_ctor()
would be "more" correct. In a similar way we have the oddity that after a page
table upgrade on process exit for the third level pagetable_pmd_dtor() is
called, even though on process start pagetable_pgd_ctor() was called for that
level. The current common code doesn't care about this, but all of this
doesn't look nice.

Anyway, the ctor/dtor issue is something different. The below would simplify
the code to what we had before the unrolling.

---
 arch/s390/mm/pgalloc.c | 89 +++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 53 deletions(-)

diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c
index 5a3745cf7330..787e4d5fabea 100644
--- a/arch/s390/mm/pgalloc.c
+++ b/arch/s390/mm/pgalloc.c
@@ -53,63 +53,46 @@ static void __crst_table_upgrade(void *arg)
 
 int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
 {
-	unsigned long *pgd = NULL, *p4d = NULL, *__pgd;
-	unsigned long asce_limit = mm->context.asce_limit;
+	unsigned long *table, *pgd;
+	int rc, notify;
 
 	mmap_assert_write_locked(mm);
-
 	/* upgrade should only happen from 3 to 4, 3 to 5, or 4 to 5 levels */
-	VM_BUG_ON(asce_limit < _REGION2_SIZE);
-
-	if (end <= asce_limit)
-		return 0;
-
-	if (asce_limit == _REGION2_SIZE) {
-		p4d = crst_table_alloc(mm);
-		if (unlikely(!p4d))
-			goto err_p4d;
-		crst_table_init(p4d, _REGION2_ENTRY_EMPTY);
-		pagetable_p4d_ctor(virt_to_ptdesc(p4d));
+	VM_BUG_ON(mm->context.asce_limit < _REGION2_SIZE);
+	rc = 0;
+	notify = 0;
+	while (mm->context.asce_limit < end) {
+		table = crst_table_alloc(mm);
+		if (!table) {
+			rc = -ENOMEM;
+			break;
+		}
+		spin_lock_bh(&mm->page_table_lock);
+		pgd = (unsigned long *)mm->pgd;
+		if (mm->context.asce_limit == _REGION2_SIZE) {
+			crst_table_init(table, _REGION2_ENTRY_EMPTY);
+			p4d_populate(mm, (p4d_t *)table, (pud_t *)pgd);
+			pagetable_p4d_ctor(virt_to_ptdesc(table));
+			mm->pgd = (pgd_t *)table;
+			mm->context.asce_limit = _REGION1_SIZE;
+			mm->context.asce = __pa(mm->pgd) | _ASCE_TABLE_LENGTH |
+				_ASCE_USER_BITS | _ASCE_TYPE_REGION2;
+			mm_inc_nr_puds(mm);
+		} else {
+			crst_table_init(table, _REGION1_ENTRY_EMPTY);
+			pgd_populate(mm, (pgd_t *)table, (p4d_t *)pgd);
+			pagetable_pgd_ctor(virt_to_ptdesc(table));
+			mm->pgd = (pgd_t *)table;
+			mm->context.asce_limit = TASK_SIZE_MAX;
+			mm->context.asce = __pa(mm->pgd) | _ASCE_TABLE_LENGTH |
+				_ASCE_USER_BITS | _ASCE_TYPE_REGION1;
+		}
+		notify = 1;
+		spin_unlock_bh(&mm->page_table_lock);
 	}
-	if (end > _REGION1_SIZE) {
-		pgd = crst_table_alloc(mm);
-		if (unlikely(!pgd))
-			goto err_pgd;
-		crst_table_init(pgd, _REGION1_ENTRY_EMPTY);
-		pagetable_pgd_ctor(virt_to_ptdesc(pgd));
-	}
-
-	spin_lock_bh(&mm->page_table_lock);
-
-	if (p4d) {
-		__pgd = (unsigned long *) mm->pgd;
-		p4d_populate(mm, (p4d_t *) p4d, (pud_t *) __pgd);
-		mm->pgd = (pgd_t *) p4d;
-		mm->context.asce_limit = _REGION1_SIZE;
-		mm->context.asce = __pa(mm->pgd) | _ASCE_TABLE_LENGTH |
-			_ASCE_USER_BITS | _ASCE_TYPE_REGION2;
-		mm_inc_nr_puds(mm);
-	}
-	if (pgd) {
-		__pgd = (unsigned long *) mm->pgd;
-		pgd_populate(mm, (pgd_t *) pgd, (p4d_t *) __pgd);
-		mm->pgd = (pgd_t *) pgd;
-		mm->context.asce_limit = TASK_SIZE_MAX;
-		mm->context.asce = __pa(mm->pgd) | _ASCE_TABLE_LENGTH |
-			_ASCE_USER_BITS | _ASCE_TYPE_REGION1;
-	}
-
-	spin_unlock_bh(&mm->page_table_lock);
-
-	on_each_cpu(__crst_table_upgrade, mm, 0);
-
-	return 0;
-
-err_pgd:
-	pagetable_dtor(virt_to_ptdesc(p4d));
-	crst_table_free(mm, p4d);
-err_p4d:
-	return -ENOMEM;
+	if (notify)
+		on_each_cpu(__crst_table_upgrade, mm, 0);
+	return rc;
 }
 
 unsigned long *page_table_alloc_noprof(struct mm_struct *mm)
-- 
2.53.0