[PATCH] mm/huge_memory: ensure huge_zero_folio won't have large_rmappable flag set

[PATCH] mm/huge_memory: ensure huge_zero_folio won't have large_rmappable flag set

[Miaohe Lin]

Ensure huge_zero_folio won't have large_rmappable flag set. So it can be
reported as thp,zero correctly through stable_page_flags().

Fixes: 5691753d73a2 ("mm: convert huge_zero_page to huge_zero_folio")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/huge_memory.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 2a73efea02d7..4e34b7f89daf 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -218,6 +218,8 @@ static bool get_huge_zero_page(void)
 		count_vm_event(THP_ZERO_PAGE_ALLOC_FAILED);
 		return false;
 	}
+	/* Ensure zero folio won't have large_rmappable flag set. */
+	folio_clear_large_rmappable(zero_folio);
 	preempt_disable();
 	if (cmpxchg(&huge_zero_folio, NULL, zero_folio)) {
 		preempt_enable();
-- 
2.33.0

Re: [PATCH] mm/huge_memory: ensure huge_zero_folio won't have large_rmappable flag set

[David Hildenbrand]

On 14.09.24 03:53, Miaohe Lin wrote:
> Ensure huge_zero_folio won't have large_rmappable flag set. So it can be
> reported as thp,zero correctly through stable_page_flags().
> 
> Fixes: 5691753d73a2 ("mm: convert huge_zero_page to huge_zero_folio")
 > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>> ---
>   mm/huge_memory.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 2a73efea02d7..4e34b7f89daf 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -218,6 +218,8 @@ static bool get_huge_zero_page(void)
>   		count_vm_event(THP_ZERO_PAGE_ALLOC_FAILED);
>   		return false;
>   	}
> +	/* Ensure zero folio won't have large_rmappable flag set. */
> +	folio_clear_large_rmappable(zero_folio);
>   	preempt_disable();
>   	if (cmpxchg(&huge_zero_folio, NULL, zero_folio)) {
>   		preempt_enable();

Doesn't that rather fix

commit 4c8763e84aae4d04d94b35aca9f7db6a8930ad77
Author: Ran Xiaokai <ran.xiaokai@zte.com.cn>
Date:   Fri Jul 5 10:43:43 2024 +0000

     kpageflags: detect isolated KPF_THP folios


?

We could fix it simply by changing the order of checks in there.

It makes sense, though. The huge zeropage is not tracked via the rmap 
... ever. Mapcounts etc are unused. But clearing that flag is just ugly.

I wonder if the real problem lies in using folio_alloc() here, and that 
we should be never setting the flag in the first place .... Yes, we want 
a folio, but not really an rmappable one.

... Willy, what would be your take?

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/huge_memory: ensure huge_zero_folio won't have large_rmappable flag set

[Miaohe Lin]

On 2024/9/17 19:59, David Hildenbrand wrote:
> On 14.09.24 03:53, Miaohe Lin wrote:
>> Ensure huge_zero_folio won't have large_rmappable flag set. So it can be
>> reported as thp,zero correctly through stable_page_flags().
>>
>> Fixes: 5691753d73a2 ("mm: convert huge_zero_page to huge_zero_folio")
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>> ---
>>   mm/huge_memory.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 2a73efea02d7..4e34b7f89daf 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -218,6 +218,8 @@ static bool get_huge_zero_page(void)
>>           count_vm_event(THP_ZERO_PAGE_ALLOC_FAILED);
>>           return false;
>>       }
>> +    /* Ensure zero folio won't have large_rmappable flag set. */
>> +    folio_clear_large_rmappable(zero_folio);
>>       preempt_disable();
>>       if (cmpxchg(&huge_zero_folio, NULL, zero_folio)) {
>>           preempt_enable();
> 
> Doesn't that rather fix
> 
> commit 4c8763e84aae4d04d94b35aca9f7db6a8930ad77
> Author: Ran Xiaokai <ran.xiaokai@zte.com.cn>
> Date:   Fri Jul 5 10:43:43 2024 +0000
> 
>     kpageflags: detect isolated KPF_THP folios
> 
> 
> ?
> 
> We could fix it simply by changing the order of checks in there.

Sure, we can fix it like above but huge zero folio shouldn't have large_rmappable flag set anyway.

> 
> It makes sense, though. The huge zeropage is not tracked via the rmap ... ever. Mapcounts etc are unused. But clearing that flag is just ugly.
> 
> I wonder if the real problem lies in using folio_alloc() here, and that we should be never setting the flag in the first place .... Yes, we want a folio, but not really an rmappable one.

There was a similar problem. Hugetlb folio has large_rmappable flag set through using folio_alloc. And that cause kernel panic when migrating hugetlb folio.

Please see commit 1390a3334a48 ("mm/hugetlb: fix kernel NULL pointer dereference when migrating hugetlb folio").

Thanks.
.

> 
> ... Willy, what would be your take?
>