Re: [PATCH] book3s64/radix : Align section vmemmap start address to PAGE_SIZE

[Donet Tom <donettom@linux.ibm.com>]

On 3/8/25 9:16 AM, Aneesh Kumar K.V wrote:
> Donet Tom <donettom@linux.ibm.com> writes:
>
>> A vmemmap altmap is a device-provided region used to provide
>> backing storage for struct pages. For each namespace, the altmap
>> should belong to that same namespace. If the namespaces are
>> created unaligned, there is a chance that the section vmemmap
>> start address could also be unaligned. If the section vmemmap
>> start address is unaligned, the altmap page allocated from the
>> current namespace might be used by the previous namespace also.
>> During the free operation, since the altmap is shared between two
>> namespaces, the previous namespace may detect that the page does
>> not belong to its altmap and incorrectly assume that the page is a
>> normal page. It then attempts to free the normal page, which leads
>> to a kernel crash.
>>
>> In this patch, we are aligning the section vmemmap start address
>> to PAGE_SIZE. After alignment, the start address will not be
>> part of the current namespace, and a normal page will be allocated
>> for the vmemmap mapping of the current section. For the remaining
>> sections, altmaps will be allocated. During the free operation,
>> the normal page will be correctly freed.
>>
>> Without this patch
>> ==================
>> NS1 start               NS2 start
>>   _________________________________________________________
>> |         NS1               |            NS2              |
>>   ---------------------------------------------------------
>> | Altmap| Altmap | .....|Altmap| Altmap | ...........
>> |  NS1  |  NS1   |      | NS2  |  NS2   |
>>
>> In the above scenario, NS1 and NS2 are two namespaces. The vmemmap
>> for NS1 comes from Altmap NS1, which belongs to NS1, and the
>> vmemmap for NS2 comes from Altmap NS2, which belongs to NS2.
>>
>> The vmemmap start for NS2 is not aligned, so Altmap NS2 is shared
>> by both NS1 and NS2. During the free operation in NS1, Altmap NS2
>> is not part of NS1's altmap, causing it to attempt to free an
>> invalid page.
>>
>> With this patch
>> ===============
>> NS1 start               NS2 start
>>   _________________________________________________________
>> |         NS1               |            NS2              |
>>   ---------------------------------------------------------
>> | Altmap| Altmap | .....| Normal | Altmap | Altmap |.......
>> |  NS1  |  NS1   |      |  Page  |  NS2   |  NS2   |
>>
>> If the vmemmap start for NS2 is not aligned then we are allocating
>> a normal page. NS1 and NS2 vmemmap will be freed correctly.
>>
>> Fixes: 368a0590d954("powerpc/book3s64/vmemmap: switch radix to use a different vmemmap handling function")
>> Co-developed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> Signed-off-by: Donet Tom <donettom@linux.ibm.com>
>> ---
>>   arch/powerpc/mm/book3s64/radix_pgtable.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
>> index 311e2112d782..b22d5f6147d2 100644
>> --- a/arch/powerpc/mm/book3s64/radix_pgtable.c
>> +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
>> @@ -1120,6 +1120,8 @@ int __meminit radix__vmemmap_populate(unsigned long start, unsigned long end, in
>>   	pmd_t *pmd;
>>   	pte_t *pte;
>>
>         /*
>          * Make sure we align the start vmemmap addr so that we calculate
>         the correct start_pfn in altmap boundary check to decided whether
>         we should use altmap or RAM based backing memory allocation. Also
>         the address need to be aligned for set_pte operation.
>
>         If the start addr is already PMD_SIZE aligned we will try to use
>         a pmd mapping. We don't want to be too aggressive here beacause
>         that will cause more allocations in RAM. So only if the namespace
>         vmemmap start addr is PMD_SIZE aligned we will use PMD mapping.
>
>         */
>
> May be with some comments as above?
>

Sure, I will update it and send a V3.


>
>> +	start = ALIGN_DOWN(start, PAGE_SIZE);
>> +
>>   	for (addr = start; addr < end; addr = next) {
>>   		next = pmd_addr_end(addr, end);
>>   
>> -- 
>> 2.43.5