Received-SPF: pass (google.com: domain of linux-kernel+bounces-136959-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <4311bf57-ea3e-4ea6-8f7b-370d9dd2dac1@huawei.com>
Date: Tue, 9 Apr 2024 21:27:59 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] mm,page_owner: Defer enablement of static branch
Content-Language: en-US
To: Oscar Salvador <osalvador@suse.de>, Andrew Morton
	<akpm@linux-foundation.org>
CC: <linux-kernel@vger.kernel.org>, <linux-mm@kvack.org>, Vlastimil Babka
	<vbabka@suse.cz>
References: <20240409131715.13632-1-osalvador@suse.de>
From: Kefeng Wang <wangkefeng.wang@huawei.com>
In-Reply-To: <20240409131715.13632-1-osalvador@suse.de>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 2024/4/9 21:17, Oscar Salvador wrote:
> Kefeng Wang reported that he was seeing some memory leaks with kmemleak
> with page_owner enabled.
> The reason behind is that we enable the page_owner_inited static branch
> and then proceed with the linking of stack_list struct to dummy_stack,
> which means that exists a race window between these two steps where we
> can have pages already being allocated calling add_stack_record_to_list(),
> allocating objects and linking them to stack_list, but then we set
> stack_list pointing to dummy_stack in init_page_owner.
> Which means that the objects that have been allocated during that time
> window are unreferenced and lost.
> 
> Fix this by deferring the enablement of the branch until we have properly
> set up the list.
> 
> Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> Closes: https://lore.kernel.org/linux-mm/74b147b0-718d-4d50-be75-d6afc801cd24@huawei.com/
> Tested-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> Fixes: 4bedfb314bdd ("mm,page_owner: maintain own list of stack_records structs")
> Signed-off-by: Oscar Salvador <osalvador@suse.de>
> ---
> Special thanks and kudos go to Kefeng Wang for helping me out chasing
> down this bug, as I could not reproduce it with any of my machines, and
> to Vlastimil to bring another pair of eyes, which was very helpful.
> 
The issue is found by accident when test my migrate changes, thanks for
your great job and quick response, this does pass my test, thanks ;)

>   mm/page_owner.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 9bef0b442863..742f432e5bf0 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -118,7 +118,6 @@ static __init void init_page_owner(void)
>   	register_dummy_stack();
>   	register_failure_stack();
>   	register_early_stack();
> -	static_branch_enable(&page_owner_inited);
>   	init_early_allocated_pages();
>   	/* Initialize dummy and failure stacks and link them to stack_list */
>   	dummy_stack.stack_record = __stack_depot_get_stack_record(dummy_handle);
> @@ -129,6 +128,7 @@ static __init void init_page_owner(void)
>   		refcount_set(&failure_stack.stack_record->count, 1);
>   	dummy_stack.next = &failure_stack;
>   	stack_list = &dummy_stack;
> +	static_branch_enable(&page_owner_inited);
>   }
>   
>   struct page_ext_operations page_owner_ops = {