linux-kernel - Re: [PATCH] mm,page_owner: Defer enablement of static branch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9e0de158-f7b3-4602-8552-f568a96c3c65@suse.cz>
Date: Tue, 9 Apr 2024 15:26:23 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Oscar Salvador <osalvador@...e.de>,
 Andrew Morton <akpm@...ux-foundation.org>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
 Kefeng Wang <wangkefeng.wang@...wei.com>
Subject: Re: [PATCH] mm,page_owner: Defer enablement of static branch

On 4/9/24 3:17 PM, 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@...wei.com>
> Closes: https://lore.kernel.org/linux-mm/74b147b0-718d-4d50-be75-d6afc801cd24@huawei.com/
> Tested-by: Kefeng Wang <wangkefeng.wang@...wei.com>
> Fixes: 4bedfb314bdd ("mm,page_owner: maintain own list of stack_records structs")

That's in rc1 so ideally mm-hotfixes.

> Signed-off-by: Oscar Salvador <osalvador@...e.de>

Acked-by: Vlastimil Babka <vbabka@...e.cz>

> ---
> 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.
> 
>  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 = {