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

[Kevin Brodsky <kevin.brodsky@arm.com>]

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/