linux-kernel - Re: [PATCH] x86/mm: Remove "INVPCID single" feature tracking

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: <92462889-16e9-5c7b-a524-fe53859f93c1@intel.com>
Date:   Fri, 14 Jul 2023 16:01:59 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     andrew.cooper3@...rix.com,
        Dave Hansen <dave.hansen@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org, jannh@...gle.com, x86@...nel.org,
        luto@...nel.org, peterz@...radead.org
Subject: Re: [PATCH] x86/mm: Remove "INVPCID single" feature tracking

On 7/14/23 13:27, andrew.cooper3@...rix.com wrote:
>> +	/* If PTI is off there is no user PCID and nothing to flush. */
>>  	if (!static_cpu_has(X86_FEATURE_PTI))
>>  		return;
> 
> As a minor observation, the common case is for the function to exit
> here, but you've got both this_cpu_read()'s ahead of a full compiler
> memory barrier.
> 
> If you move them here, you'll drop the reads from the common case.
> But...

That's a good point.  I was depending on the generosity of the compiler
but the invlpg throws that out the window.  I'll move them around.

>>  	/*
>> -	 * Some platforms #GP if we call invpcid(type=1/2) before CR4.PCIDE=1.
>> -	 * Just use invalidate_user_asid() in case we are called early.
>> +	 * invpcid_flush_one(pcid>0) will #GP if CR4.PCIDE==0.  Check
>> +	 * 'cpu_pcide' to ensure that *this* CPU will not trigger those
>> +	 * #GP's even if called before CR4.PCIDE has been initialized.
>>  	 */
>> -	if (!this_cpu_has(X86_FEATURE_INVPCID_SINGLE))
>> -		invalidate_user_asid(loaded_mm_asid);
>> -	else
>> +	if (boot_cpu_has(X86_FEATURE_INVPCID) && cpu_pcide)
> ... why can't this just be && loaded_mm_asid ?
> 
> There's no plausible way the asid can be nonzero here without CR4.PCIDE
> being set, and that avoids looking at cr4 directly.

Except that 0 is a valid, normal 'loaded_mm_asid' value.  It would be
quite possible to have loaded_mm_asid==0 during normal runtime which
would drop down into the invalidate_user_asid() case.  It would work,
but it would be unnecessarily destructive to the TLB.

I guess we _could_ adjust the asids to go from 1=>TLB_NR_DYN_ASIDS
instead of 0=>TLB_NR_DYN_ASIDS-1.  *But*, PTI is slow path code these
days.  I'd rather read one more (presumably) cache hot variable that's
logically clear than go messing with the ASID allocation code making
ASID 0 even more special.