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

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 <[email protected]>
> > ---
> >  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!