linux-kernel - Re: [PATCH v3 3/3] mm,page_owner: Fix accounting of pages when migrating

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]

Date: Tue, 2 Apr 2024 12:26:51 +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,
 Michal Hocko <mhocko@...e.com>, Marco Elver <elver@...gle.com>,
 Andrey Konovalov <andreyknvl@...il.com>,
 Alexander Potapenko <glider@...gle.com>
Subject: Re: [PATCH v3 3/3] mm,page_owner: Fix accounting of pages when
 migrating

On 3/26/24 7:30 AM, Oscar Salvador wrote:
> Upon migration, new allocated pages are being given the handle of the old
> pages. This is problematic because it means that for the stack which
> allocated the old page, we will be substracting the old page + the new one
> when that page is freed, creating an accounting imbalance.
> 
> There is an interest in keeping it that way, as otherwise the output will
> biased towards migration stacks should those operations occur often, but
> that is not really helpful.
> The link from the new page to the old stack is being performed by calling
> __update_page_owner_handle() in __folio_copy_owner().
> The only thing that is left is to link the migrate stack to the old
> page, so the old page will be subtracted from the migrate stack,
> avoiding by doing so any possible imbalance.
> 
> Fixes: 217b2119b9e2 ("mm,page_owner: implement the tracking of the stacks count")
> Signed-off-by: Oscar Salvador <osalvador@...e.de>
> ---
>  mm/page_owner.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 5df0d6892bdc..b4476f45b376 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -366,9 +366,12 @@ void __split_page_owner(struct page *page, int old_order, int new_order)
>  
>  void __folio_copy_owner(struct folio *newfolio, struct folio *old)
>  {
> +	int i;
>  	struct page_ext *old_ext;
>  	struct page_ext *new_ext;
>  	struct page_owner *old_page_owner;
> +	struct page_owner *new_page_owner;
> +	depot_stack_handle_t migrate_handle;
>  
>  	old_ext = page_ext_get(&old->page);
>  	if (unlikely(!old_ext))
> @@ -381,6 +384,8 @@ void __folio_copy_owner(struct folio *newfolio, struct folio *old)
>  	}
>  
>  	old_page_owner = get_page_owner(old_ext);
> +	new_page_owner = get_page_owner(new_ext);
> +	migrate_handle = new_page_owner->handle;
>  	__update_page_owner_handle(new_ext, old_page_owner->handle,
>  				   old_page_owner->order, old_page_owner->gfp_mask,
>  				   old_page_owner->last_migrate_reason,
> @@ -395,6 +400,16 @@ void __folio_copy_owner(struct folio *newfolio, struct folio *old)
>  					old_page_owner->free_pid,
>  					old_page_owner->free_tgid,
>  					old_page_owner->free_ts_nsec);
> +	/*
> +	 * We linked the original stack to the new folio, we need to do the same
> +	 * for the new one and the old folio otherwise there will be an imbalance
> +	 * when subtracting those pages from the stack.
> +	 */
> +	for (i = 0; i < (1 << new_page_owner->order); i++) {
> +		old_page_owner->handle = migrate_handle;
> +		old_ext = page_ext_next(old_ext);
> +		old_page_owner = get_page_owner(old_ext);
> +	}

Can the migration still fail after __folio_copy_owner()? That goes again to
the comment you changed in patch 1/3. If it can, this will kinda create a
mess with the old folio's handles not reflecting reality? (although
refcounts will be ok)

So if that case is possible, could we instead just dec_stack_record_count()
for the handle that allocated the new folio (IIUC "migrate_handle" here) and
inc_stack_record_count() for the original handle that we duplicated from the
old to new. Then if either old is freed (successful migration) or new is
freed (failed migration), we'll have the correct refcounts.

>  
>  	page_ext_put(new_ext);
>  	page_ext_put(old_ext);