diff mbox series

mm: kfence: fix PG_slab and memcg_data clearing

Message ID 20230320030059.20189-1-songmuchun@bytedance.com
State New
Headers
Series mm: kfence: fix PG_slab and memcg_data clearing |

Commit Message

Muchun Song March 20, 2023, 3 a.m. UTC 
  It does not reset PG_slab and memcg_data when KFENCE fails to initialize
kfence pool at runtime. It is reporting a "Bad page state" message when
kfence pool is freed to buddy. The checking of whether it is a compound
head page seems unnecessary sicne we already guarantee this when allocating
kfence pool, removing the check to simplify the code.

Fixes: 0ce20dd84089 ("mm: add Kernel Electric-Fence infrastructure")
Fixes: 8f0b36497303 ("mm: kfence: fix objcgs vector allocation")
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/kfence/core.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

Comments

Andrew Morton March 20, 2023, 9:29 p.m. UTC | #1 
On Mon, 20 Mar 2023 11:00:59 +0800 Muchun Song <songmuchun@bytedance.com> wrote:

> It does not reset PG_slab and memcg_data when KFENCE fails to initialize
> kfence pool at runtime. It is reporting a "Bad page state" message when
> kfence pool is freed to buddy. The checking of whether it is a compound
> head page seems unnecessary sicne we already guarantee this when allocating
> kfence pool, removing the check to simplify the code.
> 
> Fixes: 0ce20dd84089 ("mm: add Kernel Electric-Fence infrastructure")
> Fixes: 8f0b36497303 ("mm: kfence: fix objcgs vector allocation")
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>

I'm not sure how the -stable maintainers are to handle two Fixes: tags.
Can we narrow it down to one please?  I assume 8f0b36497303 triggered
the bad_page() warning?
Muchun Song March 21, 2023, 3:03 a.m. UTC | #2 
> On Mar 21, 2023, at 05:29, Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> On Mon, 20 Mar 2023 11:00:59 +0800 Muchun Song <songmuchun@bytedance.com> wrote:
> 
>> It does not reset PG_slab and memcg_data when KFENCE fails to initialize
>> kfence pool at runtime. It is reporting a "Bad page state" message when
>> kfence pool is freed to buddy. The checking of whether it is a compound
>> head page seems unnecessary sicne we already guarantee this when allocating
>> kfence pool, removing the check to simplify the code.
>> 
>> Fixes: 0ce20dd84089 ("mm: add Kernel Electric-Fence infrastructure")
>> Fixes: 8f0b36497303 ("mm: kfence: fix objcgs vector allocation")
>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> 
> I'm not sure how the -stable maintainers are to handle two Fixes: tags.
> Can we narrow it down to one please?  I assume 8f0b36497303 triggered
> the bad_page() warning?
> 

Actually, 0ce20dd84089 triggered the PG_slab warning and 8f0b36497303
triggered the "page still charged to cgroup" warning. This patch fixes
both warnings. Moreover, 8f0b36497303 fixes 0ce20dd84089 as well. So I think
we can narrow it down to 0ce20dd84089.

Thanks.
Peng Zhang March 21, 2023, 4:14 a.m. UTC | #3 
在 2023/3/20 11:00, Muchun Song 写道:
> It does not reset PG_slab and memcg_data when KFENCE fails to initialize
> kfence pool at runtime. It is reporting a "Bad page state" message when
> kfence pool is freed to buddy. The checking of whether it is a compound
> head page seems unnecessary sicne we already guarantee this when allocating
> kfence pool, removing the check to simplify the code.
>
> Fixes: 0ce20dd84089 ("mm: add Kernel Electric-Fence infrastructure")
> Fixes: 8f0b36497303 ("mm: kfence: fix objcgs vector allocation")
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>   mm/kfence/core.c | 30 +++++++++++++++---------------
>   1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> index 79c94ee55f97..d66092dd187c 100644
> --- a/mm/kfence/core.c
> +++ b/mm/kfence/core.c
> @@ -561,10 +561,6 @@ static unsigned long kfence_init_pool(void)
>   		if (!i || (i % 2))
>   			continue;
>   
> -		/* Verify we do not have a compound head page. */
> -		if (WARN_ON(compound_head(&pages[i]) != &pages[i]))
> -			return addr;
> -
>   		__folio_set_slab(slab_folio(slab));
>   #ifdef CONFIG_MEMCG
>   		slab->memcg_data = (unsigned long)&kfence_metadata[i / 2 - 1].objcg |
> @@ -597,12 +593,26 @@ static unsigned long kfence_init_pool(void)
>   
>   		/* Protect the right redzone. */
>   		if (unlikely(!kfence_protect(addr + PAGE_SIZE)))
> -			return addr;
> +			goto reset_slab;
>   
>   		addr += 2 * PAGE_SIZE;
>   	}
>   
>   	return 0;
> +
> +reset_slab:
> +	for (i = 0; i < KFENCE_POOL_SIZE / PAGE_SIZE; i++) {
> +		struct slab *slab = page_slab(&pages[i]);
> +
> +		if (!i || (i % 2))
> +			continue;
> +#ifdef CONFIG_MEMCG
> +		slab->memcg_data = 0;
> +#endif
> +		__folio_clear_slab(slab_folio(slab));
> +	}
Can this loop be simplified to this?

	for (i = 2; i < KFENCE_POOL_SIZE / PAGE_SIZE; i+=2) {
		struct slab *slab = page_slab(&pages[i]);
#ifdef CONFIG_MEMCG
		slab->memcg_data = 0;
#endif
		__folio_clear_slab(slab_folio(slab));
	}

> +
> +	return addr;
>   }
>   
>   static bool __init kfence_init_pool_early(void)
> @@ -632,16 +642,6 @@ static bool __init kfence_init_pool_early(void)
>   	 * fails for the first page, and therefore expect addr==__kfence_pool in
>   	 * most failure cases.
>   	 */
> -	for (char *p = (char *)addr; p < __kfence_pool + KFENCE_POOL_SIZE; p += PAGE_SIZE) {
> -		struct slab *slab = virt_to_slab(p);
> -
> -		if (!slab)
> -			continue;
> -#ifdef CONFIG_MEMCG
> -		slab->memcg_data = 0;
> -#endif
> -		__folio_clear_slab(slab_folio(slab));
> -	}
>   	memblock_free_late(__pa(addr), KFENCE_POOL_SIZE - (addr - (unsigned long)__kfence_pool));
>   	__kfence_pool = NULL;
>   	return false;
Muchun Song March 21, 2023, 7:04 a.m. UTC | #4 
On 2023/3/21 12:14, Peng Zhang wrote:
>
> 在 2023/3/20 11:00, Muchun Song 写道:
>> It does not reset PG_slab and memcg_data when KFENCE fails to initialize
>> kfence pool at runtime. It is reporting a "Bad page state" message when
>> kfence pool is freed to buddy. The checking of whether it is a compound
>> head page seems unnecessary sicne we already guarantee this when 
>> allocating
>> kfence pool, removing the check to simplify the code.
>>
>> Fixes: 0ce20dd84089 ("mm: add Kernel Electric-Fence infrastructure")
>> Fixes: 8f0b36497303 ("mm: kfence: fix objcgs vector allocation")
>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>> ---
>>   mm/kfence/core.c | 30 +++++++++++++++---------------
>>   1 file changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
>> index 79c94ee55f97..d66092dd187c 100644
>> --- a/mm/kfence/core.c
>> +++ b/mm/kfence/core.c
>> @@ -561,10 +561,6 @@ static unsigned long kfence_init_pool(void)
>>           if (!i || (i % 2))
>>               continue;
>>   -        /* Verify we do not have a compound head page. */
>> -        if (WARN_ON(compound_head(&pages[i]) != &pages[i]))
>> -            return addr;
>> -
>>           __folio_set_slab(slab_folio(slab));
>>   #ifdef CONFIG_MEMCG
>>           slab->memcg_data = (unsigned long)&kfence_metadata[i / 2 - 
>> 1].objcg |
>> @@ -597,12 +593,26 @@ static unsigned long kfence_init_pool(void)
>>             /* Protect the right redzone. */
>>           if (unlikely(!kfence_protect(addr + PAGE_SIZE)))
>> -            return addr;
>> +            goto reset_slab;
>>             addr += 2 * PAGE_SIZE;
>>       }
>>         return 0;
>> +
>> +reset_slab:
>> +    for (i = 0; i < KFENCE_POOL_SIZE / PAGE_SIZE; i++) {
>> +        struct slab *slab = page_slab(&pages[i]);
>> +
>> +        if (!i || (i % 2))
>> +            continue;
>> +#ifdef CONFIG_MEMCG
>> +        slab->memcg_data = 0;
>> +#endif
>> +        __folio_clear_slab(slab_folio(slab));
>> +    }
> Can this loop be simplified to this?
>
>     for (i = 2; i < KFENCE_POOL_SIZE / PAGE_SIZE; i+=2) {
>         struct slab *slab = page_slab(&pages[i]);
> #ifdef CONFIG_MEMCG
>         slab->memcg_data = 0;
> #endif
>         __folio_clear_slab(slab_folio(slab));
>     }
>

It's a good simplification. The loop setting Pg_slab before this
also can be simplified in the same way. However, I choose a
consistent way to fix this bug. I'd like to send a separate
simplification patch to simplify both two loops instead of
in a bugfix patch.

Thanks.
diff mbox series

Patch

diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index 79c94ee55f97..d66092dd187c 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -561,10 +561,6 @@  static unsigned long kfence_init_pool(void)
 		if (!i || (i % 2))
 			continue;
 
-		/* Verify we do not have a compound head page. */
-		if (WARN_ON(compound_head(&pages[i]) != &pages[i]))
-			return addr;
-
 		__folio_set_slab(slab_folio(slab));
 #ifdef CONFIG_MEMCG
 		slab->memcg_data = (unsigned long)&kfence_metadata[i / 2 - 1].objcg |
@@ -597,12 +593,26 @@  static unsigned long kfence_init_pool(void)
 
 		/* Protect the right redzone. */
 		if (unlikely(!kfence_protect(addr + PAGE_SIZE)))
-			return addr;
+			goto reset_slab;
 
 		addr += 2 * PAGE_SIZE;
 	}
 
 	return 0;
+
+reset_slab:
+	for (i = 0; i < KFENCE_POOL_SIZE / PAGE_SIZE; i++) {
+		struct slab *slab = page_slab(&pages[i]);
+
+		if (!i || (i % 2))
+			continue;
+#ifdef CONFIG_MEMCG
+		slab->memcg_data = 0;
+#endif
+		__folio_clear_slab(slab_folio(slab));
+	}
+
+	return addr;
 }
 
 static bool __init kfence_init_pool_early(void)
@@ -632,16 +642,6 @@  static bool __init kfence_init_pool_early(void)
 	 * fails for the first page, and therefore expect addr==__kfence_pool in
 	 * most failure cases.
 	 */
-	for (char *p = (char *)addr; p < __kfence_pool + KFENCE_POOL_SIZE; p += PAGE_SIZE) {
-		struct slab *slab = virt_to_slab(p);
-
-		if (!slab)
-			continue;
-#ifdef CONFIG_MEMCG
-		slab->memcg_data = 0;
-#endif
-		__folio_clear_slab(slab_folio(slab));
-	}
 	memblock_free_late(__pa(addr), KFENCE_POOL_SIZE - (addr - (unsigned long)__kfence_pool));
 	__kfence_pool = NULL;
 	return false;