Re: [PATCH 7/8] x86/mm: make code robust to future PAT changes

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 07/12/2022 09:41, Jan Beulich wrote:
> On 06.12.2022 18:55, Demi Marie Obenour wrote:
>> On Tue, Dec 06, 2022 at 12:06:24PM +0000, Andrew Cooper wrote:
>>> On 06/12/2022 04:33, Demi Marie Obenour wrote:
>>>> @@ -961,13 +1000,24 @@ get_page_from_l1e(
>>>>  
>>>>          switch ( l1f & PAGE_CACHE_ATTRS )
>>>>          {
>>>> -        case _PAGE_WB:
>>>> +        default:
>>>> +#ifndef NDEBUG
>>>> +            printk(XENLOG_G_WARNING
>>>> +                   "d%d: Guest tried to use bad cachability attribute %u 
>>>> for MFN %lx\n",
>>>> +                   l1e_owner->domain_id, l1f & PAGE_CACHE_ATTRS, mfn);
>>> %pd.  You absolutely want to convert the PTE bits to a PAT value before
>>> priniting (decimal on a PTE value is useless), and %PRI_mfn.
>> I’ll have to look at the rest of the Xen tree to see how to use this.
>>
>>>> +            pv_inject_hw_exception(TRAP_gp_fault, 0);
>>> As I said on IRC, we do want this, but I'm not certain if we can get
>>> away with just enabling it in debug builds.  _PAGE_GNTTAB was ok because
>>> it has always been like that, but there's a non-trivial chance that
>>> there are existing dom0 kernels which violate this constraint.
>> I chose this approach because it is very simple to implement.  Anything
>> more complex ought to be in a separate patch, IMO.
>>
>>>> +            return -EINVAL;
>>>> +#endif
> As to "complex": Just the "return -EINVAL" would be yet less complex.
> Irrespective of the remark my understanding of Andrew's response is that
> the concern extends to the error return as well.

It's a tradeoff.

From a debugging point of view, #GP is absolutely the way to go, because
you get a full backtrace out in Linux.  It doesn't matter in the
slightest which side of the SYSCALL instruction it points at, because
that's not the interesting information to look at.

Returning -EINVAL stops the problematic cache attributes from being
used, but is far more variable in terms of noticeable change inside
Linux.  It ranges from hitting a BUG(), to having the return code lost.


In this case, I'd err on the side of a #GP fault because it is a
definite error in the guest needing debugging and fixing.  But as I said
originally, it probably needs to be on-by-default with a knob to disable.

~Andrew