linux-kernel - Re: [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set

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: <aff049cb-ebdd-68ad-5597-d22f87026297@suse.com>
Date:   Thu, 25 Aug 2022 12:41:05 +0200
From:   Juergen Gross <jgross@...e.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     xen-devel@...ts.xenproject.org, x86@...nel.org,
        linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function

On 25.08.22 12:31, Borislav Petkov wrote:
> On Sat, Aug 20, 2022 at 11:25:25AM +0200, Juergen Gross wrote:
>> The Cyrix cpu specific MTRR function cyrix_set_all() will never be
>> called, as the struct mtrr_ops set_all() callback will only be called
>> in the use_intel() case, which would require the use_intel_if member
>> of struct mtrr_ops to be set, which isn't the case for Cyrix.
> 
> Doing some git archeology:
> 
> So the commit which added mtrr_aps_delayed_init is
> 
>    d0af9eed5aa9 ("x86, pat/mtrr: Rendezvous all the cpus for MTRR/PAT init")
> 
> from 2009.
> 
> The IPI callback before it, looked like this:
> 
> static void ipi_handler(void *info)
> {
> #ifdef CONFIG_SMP
> 	struct set_mtrr_data *data = info;
> 	unsigned long flags;
> 
> 	local_irq_save(flags);
> 
> 	atomic_dec(&data->count);
> 	while (!atomic_read(&data->gate))
> 		cpu_relax();
> 
> 	/*  The master has cleared me to execute  */
> 	if (data->smp_reg != ~0U) {
> 		mtrr_if->set(data->smp_reg, data->smp_base,
> 			     data->smp_size, data->smp_type);
> 	} else {
> 		mtrr_if->set_all();
> 		^^^^^^^^^
> 
> and that else branch would call ->set_all() on Cyrix too.
> 
> Suresh's patch changed it to do:
> 
> -	} else {
> +	} else if (mtrr_aps_delayed_init) {
> +		/*
> +		 * Initialize the MTRRs inaddition to the synchronisation.
> +		 */
>   		mtrr_if->set_all();
> 
> BUT below in the set_mtrr() call, it did:
> 
>          /*
>           * HACK!
>           * We use this same function to initialize the mtrrs on boot.
>           * The state of the boot cpu's mtrrs has been saved, and we want
>           * to replicate across all the APs.
>           * If we're doing that @reg is set to something special...
>           */
>          if (reg != ~0U)
>                  mtrr_if->set(reg, base, size, type);
>          else if (!mtrr_aps_delayed_init)
>                  mtrr_if->set_all();
> 		^^^
> 
> and that would be the Cyrix case.
> 
> But then
> 
>    192d8857427d ("x86, mtrr: use stop_machine APIs for doing MTRR rendezvous")
> 
> came and "cleaned" all up by removing the "HACK" and doing ->set_all()
> only in the rendezvous handler:
> 
> +       } else if (mtrr_aps_delayed_init || !cpu_online(smp_processor_id())) {
>                  mtrr_if->set_all();
>          }
> 
> Which begs the question: why doesn't the second part of the else test
> match on Cyrix? The "|| !cpu_online(smp_processor_id())" case.
> 
> If only we had a Cyrix machine somewhere...
> 

Maybe the alternative reasoning is much faster to understand: if the
Cyrix set_all() could be called, the AMD and Centaur ones would be callable,
too. Those being called would result in a NULL deref, so why should we keep
the Cyrix one?


Juergen

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3099 bytes)

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (496 bytes)