linux-kernel - Re: [repost PATCH v2] x86/mm/ident_map: Use gbpages only where full GB page should be mapped.

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] [day] [month] [year] [list]

Message-ID: <d5fd6d49-b0bb-48eb-a07d-9cc9c0a9bb4b@intel.com>
Date: Wed, 24 Jan 2024 15:24:10 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Steve Wahl <steve.wahl@....com>, Dave Hansen
 <dave.hansen@...ux.intel.com>, Andy Lutomirski <luto@...nel.org>,
 Peter Zijlstra <peterz@...radead.org>, Thomas Gleixner <tglx@...utronix.de>,
 Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
 x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
 linux-kernel@...r.kernel.org
Cc: Russ Anderson <rja@....com>, Dimitri Sivanich <sivanich@....com>
Subject: Re: [repost PATCH v2] x86/mm/ident_map: Use gbpages only where full
 GB page should be mapped.

On 1/24/24 09:36, Steve Wahl wrote:
> Instead of using gbpages for all memory regions, which can include
> vast areas outside what's actually been requested, use them only when
> map creation requests include the full GB page of space; descend to
> using smaller 2M pages when only portions of a GB page are included in
> the request.

This kinda jumps immediately to the solution without stating the problem.

The problem is something like this:

	Right now, ident_pud_init() will use 1GB pages to map an area as
	long as 1G pages are supported.  It does not consider the size
	of the area being mapped.  Mapping 1G?  Use a 1GB mapping.
	Mapping 4k?  Also use a 1GB mapping.  On UV systems, this ends
	up mapping BIOS-reserved regions that will cause hardware to
	halt the system if accessed, even speculatively.

Right?

> +		/*
> +		 * To be eligible to use a gbpage:
> +		 *   - gbpages must be enabled
> +		 *   - addr must be gb aligned (start of region)
> +		 *   - next must be gb aligned (end of region)
> +		 *   - PUD must be empty (nothing already mapped in this region)
> +		 */

.. this also needs to say _why_.  As it stands, it kinda just rewrites
the code in English which isn't super helpful.  It's also awfully
awkward to write a multi-line comment above a multi-line if().

Why not refactor it to do something like:

		bool can_use_gbpages = info->direct_gbpages;

		/* Avoid using a gbpage when it would be too large: */
		can_use_gbpages &= (addr & ~PUD_MASK) ||
				   (next & ~PUD_MASK);

		/* Never overwrite existing mappings: */
		can_use_gbpages &= !pud_present(*pud);

		if (can_use_gbpages) {
			...