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

[Alexander Gordeev <agordeev@linux.ibm.com>]

On Mon, Aug 24, 2026 at 04:22:27PM +0200, Heiko Carstens wrote:
> 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.

Since your version is complete, would you like to post it?
If yes, please find:

Reviewed-by: Alexander Gordeev <agordeev@linux.ibm.com>

>  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

Thanks!