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

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

[Alexander Gordeev]

On a 4-to-5 level upgrade (unlike 3-to-5) p4d local variable
stays NULL. If the pgd allocation then fails, the fault code
path calls pagetable_dtor(virt_to_ptdesc(p4d)) with NULL.

Fixes: 502269ab98b5 ("s390/mm: add missing ctor/dtor on page table upgrade")
Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
---
 arch/s390/mm/pgalloc.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c
index 9610770fcf6d..87a8bdb3e51e 100644
--- a/arch/s390/mm/pgalloc.c
+++ b/arch/s390/mm/pgalloc.c
@@ -69,14 +69,19 @@ int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
 	if (asce_limit == _REGION2_SIZE) {
 		p4d = crst_table_alloc(mm);
 		if (unlikely(!p4d))
-			goto err_p4d;
+			return -ENOMEM;
 		crst_table_init(p4d, _REGION2_ENTRY_EMPTY);
 		pagetable_p4d_ctor(virt_to_ptdesc(p4d));
 	}
 	if (end > _REGION1_SIZE) {
 		pgd = crst_table_alloc(mm);
-		if (unlikely(!pgd))
-			goto err_pgd;
+		if (unlikely(!pgd)) {
+			if (p4d) {
+				pagetable_dtor(virt_to_ptdesc(p4d));
+				crst_table_free(mm, p4d);
+			}
+			return -ENOMEM;
+		}
 		crst_table_init(pgd, _REGION1_ENTRY_EMPTY);
 		pagetable_pgd_ctor(virt_to_ptdesc(pgd));
 	}
@@ -106,12 +111,6 @@ int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
 	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;
 }
 
 unsigned long *page_table_alloc_noprof(struct mm_struct *mm)
-- 
2.53.0

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

[Heiko Carstens]

On Tue, Aug 11, 2026 at 02:13:16PM +0200, Alexander Gordeev wrote:
> On a 4-to-5 level upgrade (unlike 3-to-5) p4d local variable
> stays NULL. If the pgd allocation then fails, the fault code
> path calls pagetable_dtor(virt_to_ptdesc(p4d)) with NULL.
> 
> Fixes: 502269ab98b5 ("s390/mm: add missing ctor/dtor on page table upgrade")
> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> ---
>  arch/s390/mm/pgalloc.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c
> index 9610770fcf6d..87a8bdb3e51e 100644
> --- a/arch/s390/mm/pgalloc.c
> +++ b/arch/s390/mm/pgalloc.c
> @@ -69,14 +69,19 @@ int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
>  	if (asce_limit == _REGION2_SIZE) {
>  		p4d = crst_table_alloc(mm);
>  		if (unlikely(!p4d))
> -			goto err_p4d;
> +			return -ENOMEM;
>  		crst_table_init(p4d, _REGION2_ENTRY_EMPTY);
>  		pagetable_p4d_ctor(virt_to_ptdesc(p4d));
>  	}
>  	if (end > _REGION1_SIZE) {
>  		pgd = crst_table_alloc(mm);
> -		if (unlikely(!pgd))
> -			goto err_pgd;
> +		if (unlikely(!pgd)) {
> +			if (p4d) {
> +				pagetable_dtor(virt_to_ptdesc(p4d));
> +				crst_table_free(mm, p4d);
> +			}
> +			return -ENOMEM;
> +		}
>  		crst_table_init(pgd, _REGION1_ENTRY_EMPTY);
>  		pagetable_pgd_ctor(virt_to_ptdesc(pgd));
>  	}
> @@ -106,12 +111,6 @@ int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
>  	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;
>  }

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.

[1] 31932757c612 ("s390/mm: optimize page table upgrade routine")
[2] b5efb63acf7b ("s390/mm: Add NULL pointer check to crst_table_free() base_crst_free()")

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

[Alexander Gordeev]

On Tue, Aug 11, 2026 at 04:12:03PM +0200, Heiko Carstens wrote:
> On Tue, Aug 11, 2026 at 02:13:16PM +0200, Alexander Gordeev wrote:
> > On a 4-to-5 level upgrade (unlike 3-to-5) p4d local variable
> > stays NULL. If the pgd allocation then fails, the fault code
> > path calls pagetable_dtor(virt_to_ptdesc(p4d)) with NULL.
> > 
> > Fixes: 502269ab98b5 ("s390/mm: add missing ctor/dtor on page table upgrade")
> > Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> > ---
> >  arch/s390/mm/pgalloc.c | 17 ++++++++---------
> >  1 file changed, 8 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c
> > index 9610770fcf6d..87a8bdb3e51e 100644
> > --- a/arch/s390/mm/pgalloc.c
> > +++ b/arch/s390/mm/pgalloc.c
> > @@ -69,14 +69,19 @@ int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
> >  	if (asce_limit == _REGION2_SIZE) {
> >  		p4d = crst_table_alloc(mm);
> >  		if (unlikely(!p4d))
> > -			goto err_p4d;
> > +			return -ENOMEM;
> >  		crst_table_init(p4d, _REGION2_ENTRY_EMPTY);
> >  		pagetable_p4d_ctor(virt_to_ptdesc(p4d));
> >  	}
> >  	if (end > _REGION1_SIZE) {
> >  		pgd = crst_table_alloc(mm);
> > -		if (unlikely(!pgd))
> > -			goto err_pgd;
> > +		if (unlikely(!pgd)) {
> > +			if (p4d) {
> > +				pagetable_dtor(virt_to_ptdesc(p4d));
> > +				crst_table_free(mm, p4d);
> > +			}
> > +			return -ENOMEM;
> > +		}
> >  		crst_table_init(pgd, _REGION1_ENTRY_EMPTY);
> >  		pagetable_pgd_ctor(virt_to_ptdesc(pgd));
> >  	}
> > @@ -106,12 +111,6 @@ int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
> >  	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;
> >  }
> 
> 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 (b) useless 3-4 upgrade.

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.

> [1] 31932757c612 ("s390/mm: optimize page table upgrade routine")
> [2] b5efb63acf7b ("s390/mm: Add NULL pointer check to crst_table_free() base_crst_free()")

Thanks!

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

[Heiko Carstens]

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