Received-SPF: pass (google.com: domain of linux-kernel+bounces-87674-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <05449435-008c-4d51-b21f-03df1fa58e77@intel.com>
Date: Fri, 1 Mar 2024 12:21:59 +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>
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <ZeEOC0mo8C4GL708@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?RmxLVWhVVHk3dVdQTWV4Ty8yNGNHalNtZS9YZWE2emVycm43UzdpZGhPOTFK?=
 =?utf-8?B?Q3hxNG9qaXRFeGZXLy9WZTV2UGpNUU1FM1lPZ09jdmtXa0h2RVpBL2xPbVpU?=
 =?utf-8?B?VXBwcEF6QXZiYytpTmd0NWI3S0FZSStPVjBZcU8xVDVVcU1mZytzVGx0VWZ2?=
 =?utf-8?B?THVnNHBwdzZmeXhUTkFsR2F6RHVMc0JSaUh4d3JNWEYzNmZ4Z0NwcHFvdjQv?=
 =?utf-8?B?UWo2dzc2ZTY1TDZCMklabjF2L3pCU1N2T2g2ZFVHOXJiUFZQbGs2UjUvVEpm?=
 =?utf-8?B?dlhhcjV1ZG8xTWxNWWxYckVWWHBmSnZiNDZ1SUpmQk90Y0VZZ1JPSTBON2RO?=
 =?utf-8?B?OE9WOTYyalVmUjcxamE3UTNsS3A4emUwcW8vTEVDbExXOVZlckdocVM3dmxr?=
 =?utf-8?B?aXBldDVJLzZYS1NsUmpUcVNLRkFvZU5sZjlKeG9majJPaDNYSStiemF6aHFi?=
 =?utf-8?B?VVlLWmg4M3hvOWdLTjBSYTA1clMyVjFBNEhBbk0yT1o5RG1QZ1ZQcUV4WC9m?=
 =?utf-8?B?QXFqdWRMQ2NZRklPWVd1WjloSm10ekRPT3pCc3JjOVdSZVRaRnl0T05ZcDhv?=
 =?utf-8?B?NGpZaVkvUWdpNnJIdldxZG10WlJ3cy91N1J4ZzBTSkNZdHMvMEpHTmUvSU54?=
 =?utf-8?B?TGIvWGhxSUhtdWlvMlZGYU8xM0E0bmFxRExtY3NtaEJCL0VjZ3BNSlV5WFVp?=
 =?utf-8?B?WlVqTDF3aUk3Q0tlUlM4WmFORG5lU2M1anJWOGtPMnlYY091ZzIzYk95MldX?=
 =?utf-8?B?TG4zWnkxYUVta2FXczQwM3pNcFVwNUd6RTZrdHY3WFd3MmU3Y0d0bUNoZERm?=
 =?utf-8?B?VytqYXpuY213eXRad1ZrSEtLUGpuZ1J0TXFDRVdiSE1uZjlqdVpRMlBuamRZ?=
 =?utf-8?B?N3hXbkZaVG5WemM3TFYyL2grZks4TW84U2ZQc1NxZXNqTm1jK0NFWTBIVUFq?=
 =?utf-8?B?Ty85Yk9BRUc3VHBWZklETW9YWExoVDFBd0twbkwrQVVlaTcyeHNmSEhjZTJT?=
 =?utf-8?B?dnBiRWNSS2t0R1BzdGwxRVVjM2lIQ0xNTWNkdk9QMnVHaDAwMFRTK3U5Y2p1?=
 =?utf-8?B?R1pscnF2cFhvdm9obUZOUzFZOFZyQ3RPK1luTTVDTi9WaldBU2JWWkVsREE1?=
 =?utf-8?B?Zlp3Mnh3NnNlUStJWHVOR1lpZngxR2VMaU5vbllBV3Fsb0ZrRlJhcjNaY0FR?=
 =?utf-8?B?Wk5sRi94cDNZNFVGd1VFcVZSZW9aOFNCU0ZLR0RJdURjb29jWnZpLzcwMXNG?=
 =?utf-8?B?dUhqTS9MVFVkWDFOd3Z3ZDNhMWJ5WUExM1RsRmdIR0krOTlxbDd3MzlZaHJw?=
 =?utf-8?B?NFdqR2c3RHRtblNiQ1ZwME9oVnFKeERxQ0dmMFVRWnFPR0FqaXJxY3dpRU5S?=
 =?utf-8?B?VUMxbU9rU3VTZStUdTBoOWFMVm9ZSXN0UFk0L3Y1T01TeGZNYW1ibXBGVm9D?=
 =?utf-8?B?cCtGVlVYSGhFWGtFUWdZVnNDUy9XR0MzMlF6Rm5Fb09EMGdQcnVIejZ0NTN3?=
 =?utf-8?B?d0Mwc2RkbnFWaWZMRDV1d2QvWE9qVVJBeWtKdVl6cXlJNkVIblBCVmtqQ251?=
 =?utf-8?B?Z2YrbTZVU0NEQldSWE5oaFdaOXJodjhseFZpSUc5dk55Y0VkdFM5M2Fucm4w?=
 =?utf-8?B?c3JFQjdSQy93TWl0SEp2eWlsTlFiaDF2cDBXMUZSUHFUUHFSblgxaEoyaEhE?=
 =?utf-8?B?cjF3ejhjZHNseUtGWC91UW9UV3BhUzVxaWZPSzVmSEswRndFK2tQT2djTmYx?=
 =?utf-8?B?WXlNWWZxZFM0MVFOUktSWmRLOWRFODVQUVpmSmtXYUVCNDBzb295amM3Q0Zk?=
 =?utf-8?B?OHpNbW41Q0hkK0I0aU9MQzVQelUrYXlpbkRxTXFXYzQ5eS9xSEFoUmQrNW0w?=
 =?utf-8?B?S0V1RXBZUlJXeDB0eC8vQzhSR2Ixc1VqSHUzaTZDS2JPUXRPL1ZveHJXcllv?=
 =?utf-8?B?TE9Sd0F6SGhSZ05XRlhYaXNaNkJoYXpLYXhnMW9vMFdSTG5PM09aWFFVOEJn?=
 =?utf-8?B?QUkrUEJyeGJlblorMlA1S3RodU85cmdMTlRNS0NlTnZ5Z2ZkeGhISnNYK001?=
 =?utf-8?B?azkyMXkyTFR4anhmSVJzVjJONmRSQkdWQmozR0F2aGk5Tk84Vk1Lcm5kNTFi?=
 =?utf-8?Q?h5606CkYPOS0yOX2aF2fTATsy?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f5ba20bd-b861-491b-3433-08dc397d3f61
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Feb 2024 23:22:07.8380
 (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: wQM+oa0zRvK1iIlxxdnvrrWETb6wNgO2eJmwwsGfeIaNZzqZ4xVEF+iIrmzGWcmdkcdZrRTUzPuDii0DPlABmw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5783
X-OriginatorOrg: intel.com


On 1/03/2024 12:06 pm, Sean Christopherson wrote:
> On Fri, Mar 01, 2024, Kai Huang wrote:
>>
>>
>> On 28/02/2024 3:41 pm, Sean Christopherson wrote:
>>> WARN and skip the emulated MMIO fastpath if a private, reserved page fault
>>> is encountered, as private+reserved should be an impossible combination
>>> (KVM should never create an MMIO SPTE for a private access).
>>>
>>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>>> ---
>>>    arch/x86/kvm/mmu/mmu.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
>>> index bd342ebd0809..9206cfa58feb 100644
>>> --- a/arch/x86/kvm/mmu/mmu.c
>>> +++ b/arch/x86/kvm/mmu/mmu.c
>>> @@ -5866,7 +5866,8 @@ 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)) {
>>> +	if (unlikely((error_code & PFERR_RSVD_MASK) &&
>>> +		     !WARN_ON_ONCE(error_code & PFERR_PRIVATE_ACCESS))) {
>>>    		r = handle_mmio_page_fault(vcpu, cr2_or_gpa, direct);
>>>    		if (r == RET_PF_EMULATE)
>>>    			goto emulate;
>>
>> It seems this will make KVM continue to call kvm_mmu_do_page_fault() when
>> such private+reserve error code actually happens (e.g., due to bug), because
>> @r is still RET_PF_INVALID in such case.
> 
> Yep.
> 
>> Is it better to just return error, e.g., -EINVAL, and give up?
> 
> As long as there is no obvious/immediate danger to the host, no obvious way for
> the "bad" behavior to cause data corruption for the guest, and continuing on has
> a plausible chance of working, then KVM should generally try to continue on and
> not terminate the VM.

Agreed.  But I think sometimes it is hard to tell whether there's any 
dangerous things waiting to happen, because that means we have to sanity 
check a lot of code, and when new patches arrive we need to keep that in 
mind too, which could be a nightmare in terms of maintenance.

> 
> 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?