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

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.

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