Received-SPF: pass (google.com: domain of linux-kernel+bounces-93018-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <420cf8e8-88de-40b1-91a3-6660f7568494@intel.com>
Date: Wed, 6 Mar 2024 10:32:16 +1300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 08/16] KVM: x86/mmu: WARN and skip MMIO cache on private,
 reserved page faults
Content-Language: en-US
To: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, Yan Zhao <yan.y.zhao@intel.com>, "Isaku
 Yamahata" <isaku.yamahata@intel.com>, Michael Roth <michael.roth@amd.com>,
	"Yu Zhang" <yu.c.zhang@linux.intel.com>, Chao Peng
	<chao.p.peng@linux.intel.com>, Fuad Tabba <tabba@google.com>, David Matlack
	<dmatlack@google.com>
References: <20240228024147.41573-1-seanjc@google.com>
 <20240228024147.41573-9-seanjc@google.com>
 <2237d6b1-1c90-4acd-99e9-f051556dd6ac@intel.com>
 <ZeEOC0mo8C4GL708@google.com>
 <05449435-008c-4d51-b21f-03df1fa58e77@intel.com>
 <ZeXt6A_w4etYCYP7@google.com>
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <ZeXt6A_w4etYCYP7@google.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WVdYRGpkMlVvdk9tVDZqR0ZBMDRxb2p4VHJ4Sm1Ua0svbXlhREM0ME5rdm1J?=
 =?utf-8?B?ZjJzQkZ2czJLVFFxTzRLa05lVkdKMEh6NWYvMlBqdFVKVnZtTDB2Y1cyUjVk?=
 =?utf-8?B?eU9GMFB3SEhUdDdXVzBjSnpmNExSMVNsWm1TcUovT3Q2VUxTZzBOLzcveFhP?=
 =?utf-8?B?T3RjV0tSM2NLdjVmV2gybU5nMGplUWI2KzlkbUduaVR6aDNjSm9XMzk5WFBR?=
 =?utf-8?B?UXE4L1RBRzZzQTQwVFF3QkpoR1QwYUg5V0lSbW1McWl0WmRmMXNyVWNCVzNu?=
 =?utf-8?B?ZUVwSjl1d0V1WUI3ZDlLNkNGTW50Y1RTdEt0ZXRXc0dpbnB6ZCszL0NMR3lV?=
 =?utf-8?B?WDFKbzV2MDFJNm9sMXl4VnFjUVJLSzQ0T1k4R0RBZm1LcWNDQlg5cDN0dTdj?=
 =?utf-8?B?Yy85T1FTMTRjOWVZYXcwSFoyWXNJeW1MU2tyd3FnMWFLN3lFZzFrQVdHOEEz?=
 =?utf-8?B?anhLeUdYUi9uVlkwNWMxdHdPMHFxdHE5d2VqRWNsU0xvZ1hYa04xbVd6Q0ho?=
 =?utf-8?B?Vm1qR1RWek9pOHBWVnVScEJ6QnB3a1d3eHJRTnM5aEVPN1JnbUxTaHIwQ2ps?=
 =?utf-8?B?UStlUHVsVEpNckhVRTBkNjJqU3IwRzErOWhiY05EM2p6Z2J1M3ZIeDIvOSt6?=
 =?utf-8?B?Tk02ekE0N2llMVBPWktSR2FBWE9FeHQxb3hyYWFYSzBqNENmbk9tSFZMSDB0?=
 =?utf-8?B?WExWNzhVdkZJYnhVUnd6a2I3bzJGVGpnTGJCaFg4T3hMTmsxKzdESGNaVDZ1?=
 =?utf-8?B?U0JaSXg2UHF5NW9TaGN6S2J2bVo2c2RsTDBTWlBjTmtFTVVwMlI3WG9sbklY?=
 =?utf-8?B?dThYeng3N2RYYmI3U1h0MGNmUHJjM3NRSlhabzd5enFkbkV6a042NEk4ZnRr?=
 =?utf-8?B?WlpLdHBWSmdFU0lhckFlK3pTZkR5WFdhd1NNc1N1ZlVJUEI3RjA1TlRlVVdR?=
 =?utf-8?B?STVoN25iWDhsT0U0OG1hQ0VBcXNYY1ViT0lyM0Y5My9vYUNuMEEzWTgwcEVY?=
 =?utf-8?B?amZYd3l2WkhaYWVqYXVaRjlJTzIvSTdvTS83WmxYSTF6bXg3QzRXMGxtcVZI?=
 =?utf-8?B?ZDhZZzlwRkdZU0lwS2lKckdHeHNSeUpieVNIcTFwRUtEZ213dmY3RmI0cFp2?=
 =?utf-8?B?a3hOV0RkamcwYW9HbFgwa0JZVFlzQU9odktLSTZ5dVNQWW0zMUI4STBCTFBw?=
 =?utf-8?B?NG5OS1J4T2Z2bWtqYS9xVW1BNmFNaTlpeXZWakFtOGZPZ29LazZZVUxabnZs?=
 =?utf-8?B?WEpOZlVMdHVSZklEcTYvaE5FSFFSNXIzUVBhcnNKSkg1NGg1Tkp0NGxrem5F?=
 =?utf-8?B?bklDSmIvSEh3WmRQNWg1N01VU0pRc29hSmZLamZaemJicDUrVEZYNDBWY3Vr?=
 =?utf-8?B?OFMwV0pwSDlGRjN2byt2VmkzQzFFUW9HZExBY0RPZTlMektIMHhDbXJqNVJP?=
 =?utf-8?B?VlZ2SHE1RnVHWHBaVVEwZUZVRzZnY3liaUQ2VTYvZjUzNjdFREZ3VHovblZ0?=
 =?utf-8?B?YXZBMExSclZ2QXpnakIra3JWRXZTcXFiWUdNM25tZjFIalI2ZVo5OFVMTCtJ?=
 =?utf-8?B?OVp4ZTR4QUd6U0VianhSdldtRlZXMjcyc0VYWDNvbDhKTkVvek5tTDVCQmZL?=
 =?utf-8?B?T3d1V2hBTGV0U2ZQUkJVWTMxUThRU3UzRStDQ0E1VWV5cms5RnY5cmY5NkY0?=
 =?utf-8?B?cGJhc2U2eFAwMU5VSWZicHlPQ3BQL25ZRzlGVWR1bndsdzZGcTdpUW5Vc0pD?=
 =?utf-8?B?UEVYRjk3WEdZVE9hOUZDQi9hL05xSGlnbFhZbnJtYWh3c0hIYS8yNTE0Z3V3?=
 =?utf-8?B?UlBsKzNYN3BPbGZkWTlwdHJBT0l0bHVFYU83UXNzK0cxb3NwUXpYVndzMU92?=
 =?utf-8?B?dkpCNGcwdnZpOWlxeHVhZ0N0eXdtL1JrUHErWVVPTXNuZVFEWnF2U3ZZNGFS?=
 =?utf-8?B?VDExVTNDbFdlUm5maWNtTHowL1BSa0lxWWJWc2VpMjc5Q2RDTWpHakZLWUNQ?=
 =?utf-8?B?b3BVM28yeFJZWHA5RGRJVENtRXBQNkVYMS9PdW9oQnJBSlF0Q3BkaENXQm9T?=
 =?utf-8?B?WlRTRmk0aWpCTVJvTFNCaWdaaitxbWlJVEYyZ2l3RnluRkVIWFQ4V2hqd0Iv?=
 =?utf-8?Q?cBGCr/edQ6JOFmpl8MHDABigQ?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 35d44199-6677-4372-2bab-08dc3d5bbfe9
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Mar 2024 21:32:25.2555
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 0QIwFKPWyGWEiSxSZJAUtwyYdE6qA0ojMoh3ERX8JPeE7BwYk4f5QnyVPmW3+Pc8oZIduoX7Y8pCrY7mmWjsDA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB8049
X-OriginatorOrg: intel.com


On 5/03/2024 4:51 am, Sean Christopherson wrote:
> On Fri, Mar 01, 2024, Kai Huang wrote:
>> On 1/03/2024 12:06 pm, Sean Christopherson wrote:
>>> E.g. in this case, KVM will just skip various fast paths because of the RSVD flag,
>>> and treat the fault like a PRIVATE fault.  Hmm, but page_fault_handle_page_track()
>>> would skip write tracking, which could theoretically cause data corruption, so I
>>> guess arguably it would be safer to bail?
>>>
>>> Anyone else have an opinion?  This type of bug should never escape development,
>>> so I'm a-ok effectively killing the VM.  Unless someone has a good argument for
>>> continuing on, I'll go with Kai's suggestion and squash this:
>>>
>>> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
>>> index cedacb1b89c5..d796a162b2da 100644
>>> --- a/arch/x86/kvm/mmu/mmu.c
>>> +++ b/arch/x86/kvm/mmu/mmu.c
>>> @@ -5892,8 +5892,10 @@ int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
>>>                   error_code |= PFERR_PRIVATE_ACCESS;
>>>           r = RET_PF_INVALID;
>>> -       if (unlikely((error_code & PFERR_RSVD_MASK) &&
>>> -                    !WARN_ON_ONCE(error_code & PFERR_PRIVATE_ACCESS))) {
>>> +       if (unlikely(error_code & PFERR_RSVD_MASK)) {
>>> +               if (WARN_ON_ONCE(error_code & PFERR_PRIVATE_ACCESS))
>>> +                       return -EFAULT;
>>
>> -EFAULT is part of guest_memfd() memory fault ABI.  I didn't think over this
>> thoroughly but do you want to return -EFAULT here?
> 
> Yes, I/we do.  There are many existing paths that can return -EFAULT from KVM_RUN
> without setting run->exit_reason to KVM_EXIT_MEMORY_FAULT.  Userspace is responsible
> for checking run->exit_reason on -EFAULT (and -EHWPOISON), i.e. must be prepared
> to handle a "bare" -EFAULT, where for all intents and purposes "handle" means
> "terminate the guest".

Right.

> 
> That's actually one of the reasons why KVM_EXIT_MEMORY_FAULT exists, it'd require
> an absurd amount of work and churn in KVM to *safely* return useful information
> on *all* -EFAULTs.  FWIW, I had hopes and dreams of actually doing exactly this,
> but have long since abandoned those dreams.

I am not sure whether we need to do that.  Perhaps it made you feel so 
after we changed to use -EFAULT to carry KVM_EXIT_MEMORY_FAULT. :-)

> 
> In other words, KVM_EXIT_MEMORY_FAULT essentially communicates to userspace that
> (a) userspace can likely fix whatever badness triggered the -EFAULT, and (b) that
> KVM is in a state where fixing the underlying problem and resuming the guest is
> safe, e.g. won't corrupt the guest (because KVM is in a half-baked state).
> 

Sure.  One small issue might be that, in a later code check, we actually 
return KVM_EXIT_MEMORY_FAULT when private fault hits RET_PF_EMULATION -- 
see your patch:

[PATCH 01/16] KVM: x86/mmu: Exit to userspace with -EFAULT if private 
fault hits emulation

So here if we just return -EFAULT w/o reporting KVM_EXIT_MEMORY_FAULT 
when private+reserved is hit, it seems there's a little bit 
inconsistency here.

But you may have concern of corrupting guest here as you mentioned above.