Re: [RFT PATCH] x86/pat: Fix set_mce_nospec() for pmem

[Jane Chu <jane.chu@oracle.com>]

Hi, All,

Since I volunteered to add a patch in my series to captured the
idea discussed here about what set_mce_nospec() should do, I went
back reread the discussion.

The conclusion of the discussion is that set_mce_nospec() should
always do set_memory_np(), regardless 'whole_page', or
DRAM vs PMEM which the code cannot tell any way.

I'd like to voice concern about the risk that Dan raised about
this change of behavior to pmem users.  Yes, in theory, it's all
driver's problem, let the driver deal with it. But in reality,
that translates to many unhappy customers when their
mission-critical applications crash and things don't get fixed
the next day.

Is the risk unavoidable? If I'm not mistaken, I thought the
central point of the discussion was about clarity/simplicity
rather than correctness.  How about we address that without
raising risk to existing customers?  Here is my proposed
wording with the fix Dan sent earlier.

   /*
    * set_memory_nospec - make memory type marking in order to prevent
    * speculative access to poisoned page.
    *
    * @pfn - pfn of a page that is either poisoned in the whole, or 
partially
    *       poisoned,
    * @whole_page - indicates whether the entire page is poisoned or only
    *       part of the page is poisoned accoding to the MSi_MISC register.
    *
    * The page might be a DRAM or a PMEM page which the code cannot tell.
    * The page might have already been unmapped (when 'whole_page') is true
    * and may not be accessed in any case (e.g. guest page).
    *
    * The page might be partially poisoned and hence the non-poisoned
    * cachelines could be safely accessed _in theory_, although practically,
    * when a DRAM page is marked PageHWPoison, the mm-subsystem prevents
    * it from being accessed, but when a PMEM page is marked PageHWPoison,
    * it could practically be accessed as it is not entirely under the
    * mm-subsystem management.
    *
    * Setting mem_type of the page to either 'NP' or 'UC' prevents the
    * prefetcher from accessing the page, henec the rest of the decision
    * is based on minimizing the risk and maximizing the flexibility, 
that is,
    * in case of 'whole_page', set mem_type to 'NP', but for partial page
    * poisoning, set mem_type to 'UC', regardless the page is DRAM or PMEM.
    */
static inline int set_mce_nospec(unsigned long pfn, bool whole_page)
<snip>
         if (whole_page)
                 rc = set_memory_np(decoy_addr, 1);
         else {
                rc = _set_memory_uc(decoy_addr, 1);
         }
<snip>

Comments? Suggestions?

thanks,
-jane




On 10/2/2021 3:17 AM, Borislav Petkov wrote:
> On Fri, Oct 01, 2021 at 11:29:43AM -0700, Dan Williams wrote:
>> My read is that the guest gets virtual #MC on an access to that page.
>> When the guest tries to do set_memory_uc() and instructs cpa_flush()
>> to do clean caches that results in taking another fault / exception
>> perhaps because the VMM unmapped the page from the guest? If the guest
>> had flipped the page to NP then cpa_flush() says "oh, no caching
>> change, skip the clflush() loop".
> 
> ... and the CLFLUSH is the insn which caused the second MCE because it
> "appeared that the guest was accessing the bad page."
> 
> Uuf, that could be. Nasty.
> 
>> Yeah, I thought UC would make the PMEM driver's life easier, but if it
>> has to contend with an NP case at all, might as well make it handle
>> that case all the time.
>>
>> Safe to say this patch of mine is woefully insufficient and I need to
>> go look at how to make the guarantees needed by the PMEM driver so it
>> can handle NP and set up alias maps.
>>
>> This was a useful discussion.
> 
> Oh yeah, thanks for taking the time!
> 
>> It proves that my commit:
>>
>> 284ce4011ba6 x86/memory_failure: Introduce {set, clear}_mce_nospec()
>>
>> ...was broken from the outset.
> 
> Well, the problem with hw errors is that it is always very hard to test
> the code. But I hear injection works now soo... :-)
> 
> Thx.
>