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:13:37 +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 1/3] mm,page_owner: Update metada for tail pages


Subject: metada -> metadata

On 3/26/24 7:30 AM, Oscar Salvador wrote:
> __set_page_owner_handle() and __reset_page_owner() update the metadata
> of all pages when the page is of a higher-order, but we miss to do the
> same when the pages are migrated.
> __folio_copy_owner() only updates the metadata of the head page, meaning
> that the information stored in the first page and the tail pages will not
> match.
> 
> Strictly speaking that is not a big problem because 1) we do not print
> tail pages and 2) upon splitting all tail pages will inherit the
> metada of the head page, but it is better to have all metadata in check

  metadata

> should there be any problem, so it can ease debugging.
> 
> For that purpose, a couple of helpers are created
> __update_page_owner_handle() which updates the metadata on allocation,
> and __update_page_owner_free_handle() which does the same when the page
> is freed.
> 
> __folio_copy_owner() will make use of both as it needs to entirely replace
> the page_owner metadata for the new page.
> 
> Signed-off-by: Oscar Salvador <osalvador@...e.de>

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

Also I think this series should move to mm-hotfixes due to fixing bugs from rc1.

Some more nits:

> @@ -355,31 +375,21 @@ 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);
> -	new_page_owner->order = old_page_owner->order;
> -	new_page_owner->gfp_mask = old_page_owner->gfp_mask;
> -	new_page_owner->last_migrate_reason =
> -		old_page_owner->last_migrate_reason;
> -	new_page_owner->handle = old_page_owner->handle;
> -	new_page_owner->pid = old_page_owner->pid;
> -	new_page_owner->tgid = old_page_owner->tgid;
> -	new_page_owner->free_pid = old_page_owner->free_pid;
> -	new_page_owner->free_tgid = old_page_owner->free_tgid;
> -	new_page_owner->ts_nsec = old_page_owner->ts_nsec;
> -	new_page_owner->free_ts_nsec = old_page_owner->ts_nsec;
> -	strcpy(new_page_owner->comm, old_page_owner->comm);
> -
> +	__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,
> +				   old_page_owner->ts_nsec, old_page_owner->pid,
> +				   old_page_owner->tgid, old_page_owner->comm);
>  	/*
> -	 * We don't clear the bit on the old folio as it's going to be freed
> -	 * after migration. Until then, the info can be useful in case of
> -	 * a bug, and the overall stats will be off a bit only temporarily.
> -	 * Also, migrate_misplaced_transhuge_page() can still fail the
> -	 * migration and then we want the old folio to retain the info. But
> -	 * in that case we also don't need to explicitly clear the info from
> -	 * the new page, which will be freed.
> +	 * Do not proactively clear PAGE_EXT_OWNER{_ALLOCATED} bits as the folio
> +	 * will be freed after migration. Keep them until then as they may be
> +	 * useful.
>  	 */

The full old comment made sense, the new one sounds like it's talking about
the old folio ("will be freed after migration") but we're modifying the new
folio here. IIUC it means the case of migration failing and then the new
folio MIGHT be freed. So I think you made the comment too much concise to be
immediately clear?

> -	__set_bit(PAGE_EXT_OWNER, &new_ext->flags);
> -	__set_bit(PAGE_EXT_OWNER_ALLOCATED, &new_ext->flags);
> +	__update_page_owner_free_handle(new_ext, 0, old_page_owner->order,
> +					old_page_owner->free_pid,
> +					old_page_owner->free_tgid,
> +					old_page_owner->free_ts_nsec);
> +
>  	page_ext_put(new_ext);
>  	page_ext_put(old_ext);
>  }
> @@ -787,8 +797,9 @@ static void init_pages_in_zone(pg_data_t *pgdat, struct zone *zone)
>  				goto ext_put_continue;
>  
>  			/* Found early allocated page */
> -			__set_page_owner_handle(page_ext, early_handle,
> -						0, 0);
> +			__update_page_owner_handle(page_ext, early_handle, 0, 0,
> +						   -1, local_clock(), current->pid,
> +						   current->tgid, current->comm);
>  			count++;
>  ext_put_continue:
>  			page_ext_put(page_ext);


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ