Message-ID: <974ef73e-ab4f-7b24-d070-c981654e8c22@bytedance.com>
Date:   Tue, 21 Mar 2023 12:14:08 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.8.0
Subject: Re: [PATCH] mm: kfence: fix PG_slab and memcg_data clearing
To:     Muchun Song <songmuchun@bytedance.com>, glider@google.com,
        elver@google.com, dvyukov@google.com, akpm@linux-foundation.org,
        sjpark@amazon.de, jannh@google.com, muchun.song@linux.dev,
        roman.gushchin@linux.dev
Cc:     kasan-dev@googlegroups.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
References: <20230320030059.20189-1-songmuchun@bytedance.com>
From:   Peng Zhang <zhangpeng.00@bytedance.com>
In-Reply-To: <20230320030059.20189-1-songmuchun@bytedance.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 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;