[PATCH] mm: fixup validation of buddy pfn

[PATCH] mm: fixup validation of buddy pfn

[Xianting Tian]

For RISC-V arch the first 2MB RAM could be reserved for opensbi,
and the arch code may don't create pages for the first 2MB RAM,
so it would have pfn_base=512 and mem_map began with 512th PFN when
CONFIG_FLATMEM=y.

But __find_buddy_pfn algorithm thinks the start PFN 0, it could get
0 PFN or less than the pfn_base value, so page_is_buddy() can't
verify the page whose PFN is 0 ~ 511, actually we don't have valid
pages for PFN 0 ~ 511.

Actually, buddy system should not assume Arch cretaed pages for
reserved memory, Arch may don't know the implied limitation.
With this patch, we can gurantee a valid buddy no matter what we
have pages for reserved memory or not.

Fixes: 8170ac4700d26f65 ("mm: wrap __find_buddy_pfn() with a necessary buddy page validation")
Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com>
---
 mm/internal.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/internal.h b/mm/internal.h
index c0f8fbe0445b..0ec446caeb2e 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -322,7 +322,8 @@ __find_buddy_pfn(unsigned long page_pfn, unsigned int order)
  * The found buddy can be a non PageBuddy, out of @page's zone, or its order is
  * not the same as @page. The validation is necessary before use it.
  *
- * Return: the found buddy page or NULL if not found.
+ * Return: the found buddy page or NULL if not found or NULL if buddy pfn is
+ *         not valid.
  */
 static inline struct page *find_buddy_page_pfn(struct page *page,
 			unsigned long pfn, unsigned int order, unsigned long *buddy_pfn)
@@ -330,6 +331,9 @@ static inline struct page *find_buddy_page_pfn(struct page *page,
 	unsigned long __buddy_pfn = __find_buddy_pfn(pfn, order);
 	struct page *buddy;
 
+	if (!pfn_valid(__buddy_pfn))
+		return NULL;
+
 	buddy = page + (__buddy_pfn - pfn);
 	if (buddy_pfn)
 		*buddy_pfn = __buddy_pfn;
-- 
2.17.1

Re: [PATCH] mm: fixup validation of buddy pfn

[David Hildenbrand]

On 21.06.22 05:11, Xianting Tian wrote:
> For RISC-V arch the first 2MB RAM could be reserved for opensbi,
> and the arch code may don't create pages for the first 2MB RAM,
> so it would have pfn_base=512 and mem_map began with 512th PFN when
> CONFIG_FLATMEM=y.
> 
> But __find_buddy_pfn algorithm thinks the start PFN 0, it could get
> 0 PFN or less than the pfn_base value, so page_is_buddy() can't
> verify the page whose PFN is 0 ~ 511, actually we don't have valid
> pages for PFN 0 ~ 511.
> 
> Actually, buddy system should not assume Arch cretaed pages for
> reserved memory, Arch may don't know the implied limitation.

Ehm, sorry, no. Archs have to stick to the rules of the buddy, not the
other way around. Why should we add additional overhead to the buddy
just because arch XYZ wants to be special?

If at all, we should fail hard if an arch doesn't play with the rules
and make this a VM_BUG_ON().

> With this patch, we can gurantee a valid buddy no matter what we
> have pages for reserved memory or not.
> 
> Fixes: 8170ac4700d26f65 ("mm: wrap __find_buddy_pfn() with a necessary buddy page validation")
> Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com>
> ---
>  mm/internal.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index c0f8fbe0445b..0ec446caeb2e 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -322,7 +322,8 @@ __find_buddy_pfn(unsigned long page_pfn, unsigned int order)
>   * The found buddy can be a non PageBuddy, out of @page's zone, or its order is
>   * not the same as @page. The validation is necessary before use it.
>   *
> - * Return: the found buddy page or NULL if not found.
> + * Return: the found buddy page or NULL if not found or NULL if buddy pfn is
> + *         not valid.
>   */
>  static inline struct page *find_buddy_page_pfn(struct page *page,
>  			unsigned long pfn, unsigned int order, unsigned long *buddy_pfn)
> @@ -330,6 +331,9 @@ static inline struct page *find_buddy_page_pfn(struct page *page,
>  	unsigned long __buddy_pfn = __find_buddy_pfn(pfn, order);
>  	struct page *buddy;
>  
> +	if (!pfn_valid(__buddy_pfn))
> +		return NULL;
> +
>  	buddy = page + (__buddy_pfn - pfn);
>  	if (buddy_pfn)
>  		*buddy_pfn = __buddy_pfn;


-- 
Thanks,

David / dhildenb

Re: [PATCH] mm: fixup validation of buddy pfn

[Xianting Tian]

在 2022/6/21 下午4:01, David Hildenbrand 写道:
> On 21.06.22 05:11, Xianting Tian wrote:
>> For RISC-V arch the first 2MB RAM could be reserved for opensbi,
>> and the arch code may don't create pages for the first 2MB RAM,
>> so it would have pfn_base=512 and mem_map began with 512th PFN when
>> CONFIG_FLATMEM=y.
>>
>> But __find_buddy_pfn algorithm thinks the start PFN 0, it could get
>> 0 PFN or less than the pfn_base value, so page_is_buddy() can't
>> verify the page whose PFN is 0 ~ 511, actually we don't have valid
>> pages for PFN 0 ~ 511.
>>
>> Actually, buddy system should not assume Arch cretaed pages for
>> reserved memory, Arch may don't know the implied limitation.
> Ehm, sorry, no. Archs have to stick to the rules of the buddy, not the
> other way around. Why should we add additional overhead to the buddy
> just because arch XYZ wants to be special?

We ever sent a patch to create mapping for the first 2MB RAM for RISC-V, 
But it is not accetped.

But I am just wondering, if we have the RAM whose physical base address 
is not 0, for example, start with 0x200000(2Mb).

Then the base PFN is (0x200000 >> 12) = 512, Do we still need to create 
mapping for the non-existing first 2Mb RAM,

if not, the issue still exist under the case?

>
> If at all, we should fail hard if an arch doesn't play with the rules
> and make this a VM_BUG_ON().
>
>> With this patch, we can gurantee a valid buddy no matter what we
>> have pages for reserved memory or not.
>>
>> Fixes: 8170ac4700d26f65 ("mm: wrap __find_buddy_pfn() with a necessary buddy page validation")
>> Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com>
>> ---
>>   mm/internal.h | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/internal.h b/mm/internal.h
>> index c0f8fbe0445b..0ec446caeb2e 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -322,7 +322,8 @@ __find_buddy_pfn(unsigned long page_pfn, unsigned int order)
>>    * The found buddy can be a non PageBuddy, out of @page's zone, or its order is
>>    * not the same as @page. The validation is necessary before use it.
>>    *
>> - * Return: the found buddy page or NULL if not found.
>> + * Return: the found buddy page or NULL if not found or NULL if buddy pfn is
>> + *         not valid.
>>    */
>>   static inline struct page *find_buddy_page_pfn(struct page *page,
>>   			unsigned long pfn, unsigned int order, unsigned long *buddy_pfn)
>> @@ -330,6 +331,9 @@ static inline struct page *find_buddy_page_pfn(struct page *page,
>>   	unsigned long __buddy_pfn = __find_buddy_pfn(pfn, order);
>>   	struct page *buddy;
>>   
>> +	if (!pfn_valid(__buddy_pfn))
>> +		return NULL;
>> +
>>   	buddy = page + (__buddy_pfn - pfn);
>>   	if (buddy_pfn)
>>   		*buddy_pfn = __buddy_pfn;
>

Re: [PATCH] mm: fixup validation of buddy pfn

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 3148 bytes --]

On 21 Jun 2022, at 4:49, Xianting Tian wrote:

> 在 2022/6/21 下午4:01, David Hildenbrand 写道:
>> On 21.06.22 05:11, Xianting Tian wrote:
>>> For RISC-V arch the first 2MB RAM could be reserved for opensbi,
>>> and the arch code may don't create pages for the first 2MB RAM,
>>> so it would have pfn_base=512 and mem_map began with 512th PFN when
>>> CONFIG_FLATMEM=y.
>>>
>>> But __find_buddy_pfn algorithm thinks the start PFN 0, it could get
>>> 0 PFN or less than the pfn_base value, so page_is_buddy() can't
>>> verify the page whose PFN is 0 ~ 511, actually we don't have valid
>>> pages for PFN 0 ~ 511.
>>>
>>> Actually, buddy system should not assume Arch cretaed pages for
>>> reserved memory, Arch may don't know the implied limitation.
>> Ehm, sorry, no. Archs have to stick to the rules of the buddy, not the
>> other way around. Why should we add additional overhead to the buddy
>> just because arch XYZ wants to be special?
>
> We ever sent a patch to create mapping for the first 2MB RAM for RISC-V, But it is not accetped.
>
> But I am just wondering, if we have the RAM whose physical base address is not 0, for example, start with 0x200000(2Mb).
>
> Then the base PFN is (0x200000 >> 12) = 512, Do we still need to create mapping for the non-existing first 2Mb RAM,
>
> if not, the issue still exist under the case?
>

How does RISC-V get mem_map for 2MB-4MB in FLATMEM? alloc_node_mem_map() from
mm/page_alloc.c only allocate MAX_ORDER_NR_PAGES aligned struct page.

>>
>> If at all, we should fail hard if an arch doesn't play with the rules
>> and make this a VM_BUG_ON().
>>
>>> With this patch, we can gurantee a valid buddy no matter what we
>>> have pages for reserved memory or not.
>>>
>>> Fixes: 8170ac4700d26f65 ("mm: wrap __find_buddy_pfn() with a necessary buddy page validation")
>>> Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com>
>>> ---
>>>   mm/internal.h | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/internal.h b/mm/internal.h
>>> index c0f8fbe0445b..0ec446caeb2e 100644
>>> --- a/mm/internal.h
>>> +++ b/mm/internal.h
>>> @@ -322,7 +322,8 @@ __find_buddy_pfn(unsigned long page_pfn, unsigned int order)
>>>    * The found buddy can be a non PageBuddy, out of @page's zone, or its order is
>>>    * not the same as @page. The validation is necessary before use it.
>>>    *
>>> - * Return: the found buddy page or NULL if not found.
>>> + * Return: the found buddy page or NULL if not found or NULL if buddy pfn is
>>> + *         not valid.
>>>    */
>>>   static inline struct page *find_buddy_page_pfn(struct page *page,
>>>   			unsigned long pfn, unsigned int order, unsigned long *buddy_pfn)
>>> @@ -330,6 +331,9 @@ static inline struct page *find_buddy_page_pfn(struct page *page,
>>>   	unsigned long __buddy_pfn = __find_buddy_pfn(pfn, order);
>>>   	struct page *buddy;
>>>  +	if (!pfn_valid(__buddy_pfn))
>>> +		return NULL;
>>> +
>>>   	buddy = page + (__buddy_pfn - pfn);
>>>   	if (buddy_pfn)
>>>   		*buddy_pfn = __buddy_pfn;
>>


--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]