[PATCH] s390/mm: Add missing ctor/dtor on page table upgrade

[PATCH] s390/mm: Add missing ctor/dtor on page table upgrade

[Alexander Gordeev]

Commit 78966b550289 ("s390: pgtable: add statistics for PUD and P4D
level page table") misses the call to pagetable_p4d_ctor() against
a newly allocated P4D table in crst_table_upgrade();

Commit 68c601de75d8 ("mm: introduce ctor/dtor at PGD level") misses
the call to pagetable_pgd_ctor() against a newly allocated PGD and
the call to pagetable_dtor() against a newly allocated P4D that is
about to be freed on crst_table_upgrade() PGD upgrade fail path.

The missed constructors and destructor break (at least) the page
table accounting when a process memory space is upgraded.

Reported-by: Heiko Carstens <hca@linux.ibm.com>
Closes: https://lore.kernel.org/all/20250122074954.8685-A-hca@linux.ibm.com/
Suggested-by: Heiko Carstens <hca@linux.ibm.com>
Fixes: 78966b550289 ("s390: pgtable: add statistics for PUD and P4D level page table")
Fixes: 68c601de75d8 ("mm: introduce ctor/dtor at PGD level")
Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
---
The patch is against:

  git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git	next-20250123
  git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm			mm-stable
---
 arch/s390/mm/pgalloc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c
index a4e761902093..d33f55b7ee98 100644
--- a/arch/s390/mm/pgalloc.c
+++ b/arch/s390/mm/pgalloc.c
@@ -88,12 +88,14 @@ int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
 		if (unlikely(!p4d))
 			goto err_p4d;
 		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;
 		crst_table_init(pgd, _REGION1_ENTRY_EMPTY);
+		pagetable_pgd_ctor(virt_to_ptdesc(pgd));
 	}
 
 	spin_lock_bh(&mm->page_table_lock);
@@ -130,6 +132,7 @@ int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
 	return 0;
 
 err_pgd:
+	pagetable_dtor(virt_to_ptdesc(p4d));
 	crst_table_free(mm, p4d);
 err_p4d:
 	return -ENOMEM;
-- 
2.45.2

Re: [PATCH] s390/mm: Add missing ctor/dtor on page table upgrade

[Gerald Schaefer]

On Thu, 23 Jan 2025 17:03:49 +0100
Alexander Gordeev <agordeev@linux.ibm.com> wrote:

> Commit 78966b550289 ("s390: pgtable: add statistics for PUD and P4D
> level page table") misses the call to pagetable_p4d_ctor() against
> a newly allocated P4D table in crst_table_upgrade();
> 
> Commit 68c601de75d8 ("mm: introduce ctor/dtor at PGD level") misses
> the call to pagetable_pgd_ctor() against a newly allocated PGD and
> the call to pagetable_dtor() against a newly allocated P4D that is
> about to be freed on crst_table_upgrade() PGD upgrade fail path.
> 
> The missed constructors and destructor break (at least) the page
> table accounting when a process memory space is upgraded.
> 
> Reported-by: Heiko Carstens <hca@linux.ibm.com>
> Closes: https://lore.kernel.org/all/20250122074954.8685-A-hca@linux.ibm.com/
> Suggested-by: Heiko Carstens <hca@linux.ibm.com>
> Fixes: 78966b550289 ("s390: pgtable: add statistics for PUD and P4D level page table")
> Fixes: 68c601de75d8 ("mm: introduce ctor/dtor at PGD level")
> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> ---
> The patch is against:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git	next-20250123
>   git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm			mm-stable
> ---
>  arch/s390/mm/pgalloc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c
> index a4e761902093..d33f55b7ee98 100644
> --- a/arch/s390/mm/pgalloc.c
> +++ b/arch/s390/mm/pgalloc.c
> @@ -88,12 +88,14 @@ int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
>  		if (unlikely(!p4d))
>  			goto err_p4d;
>  		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;
>  		crst_table_init(pgd, _REGION1_ENTRY_EMPTY);
> +		pagetable_pgd_ctor(virt_to_ptdesc(pgd));
>  	}
>  
>  	spin_lock_bh(&mm->page_table_lock);
> @@ -130,6 +132,7 @@ int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
>  	return 0;
>  
>  err_pgd:
> +	pagetable_dtor(virt_to_ptdesc(p4d));
>  	crst_table_free(mm, p4d);
>  err_p4d:
>  	return -ENOMEM;

Thanks, looks good, and nice catch.

Reviewed-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com>

Re: [PATCH] s390/mm: Add missing ctor/dtor on page table upgrade

[Qi Zheng]

On 2025/1/24 00:03, Alexander Gordeev wrote:
> Commit 78966b550289 ("s390: pgtable: add statistics for PUD and P4D
> level page table") misses the call to pagetable_p4d_ctor() against
> a newly allocated P4D table in crst_table_upgrade();
> 
> Commit 68c601de75d8 ("mm: introduce ctor/dtor at PGD level") misses
> the call to pagetable_pgd_ctor() against a newly allocated PGD and
> the call to pagetable_dtor() against a newly allocated P4D that is
> about to be freed on crst_table_upgrade() PGD upgrade fail path.
> 
> The missed constructors and destructor break (at least) the page
> table accounting when a process memory space is upgraded.
> 
> Reported-by: Heiko Carstens <hca@linux.ibm.com>
> Closes: https://lore.kernel.org/all/20250122074954.8685-A-hca@linux.ibm.com/
> Suggested-by: Heiko Carstens <hca@linux.ibm.com>
> Fixes: 78966b550289 ("s390: pgtable: add statistics for PUD and P4D level page table")
> Fixes: 68c601de75d8 ("mm: introduce ctor/dtor at PGD level")
> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> ---
> The patch is against:
> 
>    git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git	next-20250123
>    git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm			mm-stable
> ---
>   arch/s390/mm/pgalloc.c | 3 +++
>   1 file changed, 3 insertions(+)
> 

Acked-by: Qi Zheng <zhengqi.arch@bytedance.com>

Thanks!

Re: [PATCH] s390/mm: Add missing ctor/dtor on page table upgrade

[Kevin Brodsky]

On 23/01/2025 17:03, Alexander Gordeev wrote:
> Commit 78966b550289 ("s390: pgtable: add statistics for PUD and P4D
> level page table") misses the call to pagetable_p4d_ctor() against
> a newly allocated P4D table in crst_table_upgrade();
>
> Commit 68c601de75d8 ("mm: introduce ctor/dtor at PGD level") misses
> the call to pagetable_pgd_ctor() against a newly allocated PGD and
> the call to pagetable_dtor() against a newly allocated P4D that is
> about to be freed on crst_table_upgrade() PGD upgrade fail path.
>
> The missed constructors and destructor break (at least) the page
> table accounting when a process memory space is upgraded.
>
> Reported-by: Heiko Carstens <hca@linux.ibm.com>
> Closes: https://lore.kernel.org/all/20250122074954.8685-A-hca@linux.ibm.com/
> Suggested-by: Heiko Carstens <hca@linux.ibm.com>
> Fixes: 78966b550289 ("s390: pgtable: add statistics for PUD and P4D level page table")
> Fixes: 68c601de75d8 ("mm: introduce ctor/dtor at PGD level")
> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> ---
> The patch is against:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git	next-20250123
>   git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm			mm-stable

Thank you for putting together this patch! I was completely unaware of
this "upgrade" path on s390.

> ---
>  arch/s390/mm/pgalloc.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c
> index a4e761902093..d33f55b7ee98 100644
> --- a/arch/s390/mm/pgalloc.c
> +++ b/arch/s390/mm/pgalloc.c
> @@ -88,12 +88,14 @@ int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
>  		if (unlikely(!p4d))
>  			goto err_p4d;
>  		crst_table_init(p4d, _REGION2_ENTRY_EMPTY);
> +		pagetable_p4d_ctor(virt_to_ptdesc(p4d));

This block seems to be equivalent to p4d_alloc_one(), maybe it would be
preferable to just call p4d_alloc_one() here to avoid further mismatches
in the future (and reduce duplication)?


>  	}
>  	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));

I was hoping this might be equivalent to pgd_alloc() but it does not
include a call to crst_table_init(). Since adding it would be apparently
undesirable (having read the other thread), it seems reasonable to add
the explicit constructor call.

>  	}
>  
>  	spin_lock_bh(&mm->page_table_lock);
> @@ -130,6 +132,7 @@ int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
>  	return 0;
>  
>  err_pgd:
> +	pagetable_dtor(virt_to_ptdesc(p4d));
>  	crst_table_free(mm, p4d);

Similarly, this could be a call to p4d_free().

- Kevin

>  err_p4d:
>  	return -ENOMEM;

Re: [PATCH] s390/mm: Add missing ctor/dtor on page table upgrade

[Alexander Gordeev]

On Fri, Jan 24, 2025 at 08:58:07AM +0100, Kevin Brodsky wrote:

Kevin,
...
> Thank you for putting together this patch! I was completely unaware of
> this "upgrade" path on s390.
...
> > diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c
> > index a4e761902093..d33f55b7ee98 100644
> > --- a/arch/s390/mm/pgalloc.c
> > +++ b/arch/s390/mm/pgalloc.c
> > @@ -88,12 +88,14 @@ int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
> >  		if (unlikely(!p4d))
> >  			goto err_p4d;
> >  		crst_table_init(p4d, _REGION2_ENTRY_EMPTY);
> > +		pagetable_p4d_ctor(virt_to_ptdesc(p4d));
> 
> This block seems to be equivalent to p4d_alloc_one(), maybe it would be
> preferable to just call p4d_alloc_one() here to avoid further mismatches
> in the future (and reduce duplication)?

Yes, I wanted to do exactly that (together with p4d_free() you noticed),
but then the whole thing with open coded pagetable_pgd_ctor() below gets
inconsistent. I would prefer to keep it this way as of now.

> >  	}
> >  	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));
> 
> I was hoping this might be equivalent to pgd_alloc() but it does not
> include a call to crst_table_init(). Since adding it would be apparently
> undesirable (having read the other thread), it seems reasonable to add
> the explicit constructor call.

We were thinking about a follow-up cleanup that addresses it all, but this
patch is a targeted fix to catch up your and Qi Zheng series in the still
open merge window.

> >  	}
> >  
> >  	spin_lock_bh(&mm->page_table_lock);
> > @@ -130,6 +132,7 @@ int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
> >  	return 0;
> >  
> >  err_pgd:
> > +	pagetable_dtor(virt_to_ptdesc(p4d));
> >  	crst_table_free(mm, p4d);
> 
> Similarly, this could be a call to p4d_free().
> 
> - Kevin
> 
> >  err_p4d:
> >  	return -ENOMEM;

Thanks for the review!

Re: [PATCH] s390/mm: Add missing ctor/dtor on page table upgrade

[Kevin Brodsky]

On 24/01/2025 09:24, Alexander Gordeev wrote:
>>>  	}
>>>  	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));
>> I was hoping this might be equivalent to pgd_alloc() but it does not
>> include a call to crst_table_init(). Since adding it would be apparently
>> undesirable (having read the other thread), it seems reasonable to add
>> the explicit constructor call.
> We were thinking about a follow-up cleanup that addresses it all, but this
> patch is a targeted fix to catch up your and Qi Zheng series in the still
> open merge window.

Sure that's fair, this patch does fix the issue and the cleanup can wait
until the next cycle.

Reviewed-by: Kevin Brodsky <kevin.brodsky@arm.com>

- Kevin

Re: [PATCH] s390/mm: Add missing ctor/dtor on page table upgrade

[Heiko Carstens]

On Fri, Jan 24, 2025 at 08:58:07AM +0100, Kevin Brodsky wrote:
> On 23/01/2025 17:03, Alexander Gordeev wrote:
> > Commit 78966b550289 ("s390: pgtable: add statistics for PUD and P4D
> > level page table") misses the call to pagetable_p4d_ctor() against
> > a newly allocated P4D table in crst_table_upgrade();
> >
> > Commit 68c601de75d8 ("mm: introduce ctor/dtor at PGD level") misses
> > the call to pagetable_pgd_ctor() against a newly allocated PGD and
> > the call to pagetable_dtor() against a newly allocated P4D that is
> > about to be freed on crst_table_upgrade() PGD upgrade fail path.
> >
> > The missed constructors and destructor break (at least) the page
> > table accounting when a process memory space is upgraded.
> >
> > Reported-by: Heiko Carstens <hca@linux.ibm.com>
> > Closes: https://lore.kernel.org/all/20250122074954.8685-A-hca@linux.ibm.com/
> > Suggested-by: Heiko Carstens <hca@linux.ibm.com>
> > Fixes: 78966b550289 ("s390: pgtable: add statistics for PUD and P4D level page table")
> > Fixes: 68c601de75d8 ("mm: introduce ctor/dtor at PGD level")
> > Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> > ---
> > The patch is against:
> >
> >   git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git	next-20250123
> >   git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm			mm-stable
> 
> Thank you for putting together this patch! I was completely unaware of
> this "upgrade" path on s390.

This whole thing is even worse than it looks after this patch. With page
table upgrade taken into account we still have the oddity that e.g. a
previous pgd becomes a pud or p4d, which means that ctor and dtor functions
might be called for different levels for the same page table. As of now
this is ok, since they do all the same.

As a quick fix this patch is ok, and most likely it will be ok for a long
time, but it doesn't give me good feeling :)

In addition, looking at [1] where page table accounting was introduced: it
is really meant to reflect the memory consumption used by page tables. This
might work for nearly all architectures which have the same page table size
for every level; but on s390 the lowest level comes with 4kb page tables
while all other levels come with 16kb page tables.

Therefore at least on s390 you really can't tell how much memory is
consumed by page tables by only looking at nr_page_table_pages. It _might_
make sense to introduce a factor of four for page table accounting for
higher levels, so those numbers make at least some sense; but not sure
about that.

[1] https://lore.kernel.org/all/20201130212541.2781790-3-shakeelb@google.com/T/#mb6c6f2a84ded27cd9b3d54140dde1d5a75c74735

Re: [PATCH] s390/mm: Add missing ctor/dtor on page table upgrade

[Heiko Carstens]

On Fri, Jan 24, 2025 at 10:29:17AM +0100, Heiko Carstens wrote:
> On Fri, Jan 24, 2025 at 08:58:07AM +0100, Kevin Brodsky wrote:
> In addition, looking at [1] where page table accounting was introduced: it
> is really meant to reflect the memory consumption used by page tables. This
> might work for nearly all architectures which have the same page table size
> for every level; but on s390 the lowest level comes with 4kb page tables
> while all other levels come with 16kb page tables.
> 
> Therefore at least on s390 you really can't tell how much memory is
> consumed by page tables by only looking at nr_page_table_pages. It _might_
> make sense to introduce a factor of four for page table accounting for
> higher levels, so those numbers make at least some sense; but not sure
> about that.

Ah, this is actually not true at all, since we have

static inline void __lruvec_stat_add_folio(struct folio *folio,
                                           enum node_stat_item idx)
{
	__lruvec_stat_mod_folio(folio, idx, folio_nr_pages(folio));
}

which will do exactly what we want. So this part is not a problem.

Re: [PATCH] s390/mm: Add missing ctor/dtor on page table upgrade

[Kevin Brodsky]

On 24/01/2025 10:29, Heiko Carstens wrote:
> On Fri, Jan 24, 2025 at 08:58:07AM +0100, Kevin Brodsky wrote:
>> On 23/01/2025 17:03, Alexander Gordeev wrote:
>>> Commit 78966b550289 ("s390: pgtable: add statistics for PUD and P4D
>>> level page table") misses the call to pagetable_p4d_ctor() against
>>> a newly allocated P4D table in crst_table_upgrade();
>>>
>>> Commit 68c601de75d8 ("mm: introduce ctor/dtor at PGD level") misses
>>> the call to pagetable_pgd_ctor() against a newly allocated PGD and
>>> the call to pagetable_dtor() against a newly allocated P4D that is
>>> about to be freed on crst_table_upgrade() PGD upgrade fail path.
>>>
>>> The missed constructors and destructor break (at least) the page
>>> table accounting when a process memory space is upgraded.
>>>
>>> Reported-by: Heiko Carstens <hca@linux.ibm.com>
>>> Closes: https://lore.kernel.org/all/20250122074954.8685-A-hca@linux.ibm.com/
>>> Suggested-by: Heiko Carstens <hca@linux.ibm.com>
>>> Fixes: 78966b550289 ("s390: pgtable: add statistics for PUD and P4D level page table")
>>> Fixes: 68c601de75d8 ("mm: introduce ctor/dtor at PGD level")
>>> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
>>> ---
>>> The patch is against:
>>>
>>>   git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git	next-20250123
>>>   git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm			mm-stable
>> Thank you for putting together this patch! I was completely unaware of
>> this "upgrade" path on s390.
> This whole thing is even worse than it looks after this patch. With page
> table upgrade taken into account we still have the oddity that e.g. a
> previous pgd becomes a pud or p4d, which means that ctor and dtor functions
> might be called for different levels for the same page table. As of now
> this is ok, since they do all the same.
>
> As a quick fix this patch is ok, and most likely it will be ok for a long
> time, but it doesn't give me good feeling :)

This could clearly cause trouble if the pud/p4d/pgd constructors start
to diverge. In principle this could be addressed when upgrading by
calling the dtor on the old pgd and then the ctor on the new pud/p4d.
That said since there is now only one dtor, and the ctor are all the
same, it's probably good enough.

I'll mention in passing that my own series (that introduced PGD-level
ctor/dtor) really aimed at supporting the kpkeys page table protection
RFC [1], where hooks are inserted into the ctor/dtor to modify the pkey
page tables are mapped with. No idea if that would work with this sort
of upgrade path, but since s390 does not support pkeys this is just a
theoretical issue :)

- Kevin

[1]
https://lore.kernel.org/linux-hardening/20250108103250.3188419-1-kevin.brodsky@arm.com/