Re: [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2622 bytes --]

On 21.08.22 23:41, Borislav Petkov wrote:
> On Sun, Aug 21, 2022 at 02:25:59PM +0200, Borislav Petkov wrote:
>>> Fix that by using percpu variables for saving the MSR contents.
>>>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> I thought adding a "Fixes:" tag for the kernel's initial git commit
>>> would maybe be entertaining, but without being really helpful.
>>> The percpu variables were preferred over on-stack ones in order to
>>> avoid more code churn in followup patches decoupling PAT from MTRR
>>> support.
>>
>> So if that thing has been broken for so long and no one noticed, we
>> could just as well not backport to stable at all...
> 
> Yeah, you can't do that.
> 
> The whole day today I kept thinking that something's wrong with this
> here. As in, why hasn't it been reported until now.
> 
> You say above:
> 
> "... for all cpus is racy in case the MSR contents differ across cpus."
> 
> But they don't differ:
> 
> "7.7.5 MTRRs in Multi-Processing Environments
> 
> In multi-processing environments, the MTRRs located in all processors
> must characterize memory in the same way. Generally, this means that
> identical values are written to the MTRRs used by the processors. This
> also means that values CR0.CD and the PAT must be consistent across
> processors. Failure to do so may result in coherency violations or loss
> of atomicity. Processor implementations do not check the MTRR settings
> in other processors to ensure consistency. It is the responsibility of
> system software to initialize and maintain MTRR consistency across all
> processors."
> 
> And you can't have different fixed MTRR type on each CPU - that would
> lead to all kinds of nasty bugs.
> 
> And here's from a big fat box:
> 
> $ rdmsr -a 0x2ff | uniq -c
>      256 c00
> 
> All 256 CPUs have the def type set to the same thing.
> 
> Now, if all CPUs go write that same deftype_lo variable in the
> rendezvous handler, the only issue that could happen is if a read
> sees a partial write. BUT, AFAIK, x86 doesn't tear 32-bit writes so I
> *think* all CPUs see the same value being corrected by using mtrr_state
> previously saved on the BSP.
> 
> As I said, we should've seen this exploding left and right otherwise...

And then there is mtrr_state_warn() in arch/x86/kernel/cpu/mtrr/generic.c
which has a comment saying:

/* Some BIOS's are messed up and don't set all MTRRs the same! */

Yes, the chances are slim to hit such a box, but your reasoning suggests
I should remove the related code?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]