Received-SPF: pass (google.com: domain of linux-kernel+bounces-93077-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <adbcdeaa-a780-49cb-823c-3980a4dfea12@intel.com>
Date: Wed, 6 Mar 2024 12:06:14 +1300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 09/16] KVM: x86/mmu: Move private vs. shared check above
 slot validity checks
Content-Language: en-US
To: Sean Christopherson <seanjc@google.com>, Paolo Bonzini
	<pbonzini@redhat.com>
CC: <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-10-seanjc@google.com>
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <20240228024147.41573-10-seanjc@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?bW1odDF2ajlKOTlIajBmL2k4blZLb2J3RXQyM0NkeGtQV21SWHlVSjlLS0dh?=
 =?utf-8?B?YXNhYWlqNDU2djFWT1JtTHRKMWdvcTNqTk00NU9QSG1nd0VTa0ZDR2oyMkFr?=
 =?utf-8?B?RXl1blNwQ3l0YXBVOHA1QmRrLzNDbzExVmlxRFM1aWZpUjV6STZlZ09sMXdm?=
 =?utf-8?B?S1c5TmpUTHlkeDI3aE5lYlRXMUFnL1BQU2tTZkZPenJPNFRCT3FqTCtncFNI?=
 =?utf-8?B?TXlhSW44azdzcklTOWV0SVZoT2M0bmtpRUxzemNIbk5nUStBd2paL0VmQXJI?=
 =?utf-8?B?dnRDRjlqclgyelBiMlJZaWtzZElyeEl5OUhEcUt5K0FSekxYci9TcFkvWXBa?=
 =?utf-8?B?RXdwSjVFODZ4UnhrRDVkTVN3VXRzREx6YlZEdkRQa0NKK1BmV2Z4ZEtRdkN2?=
 =?utf-8?B?dFAvai9TalZ4Q2dxbll0U0VQSjNwMDlVTDlGdWxCUjNXSFUrZUx5TnJxVko5?=
 =?utf-8?B?bWpxM2R0T2IvODNZcER4d2JLaWU4N1NvRUhrN0RSQWtmbTRxbjE3ZWkwTzJn?=
 =?utf-8?B?bHAxbkFCd2xYNFpLdmZlUmlxOVJkNmsrOXgyQ1VoSVEyL0Z2YzY1Qm5VRlpC?=
 =?utf-8?B?VlIzZGZZcXQxZjVuWUZvUWNjeURoNk1HYUlNQzNUM1FJQitJUGFHRTNFVmFs?=
 =?utf-8?B?RHEwU01TYzJqeDBtT2M2QmJFQTRoVS9DYlZHSGh3L2dkcGdXZ0p3cExzNjI3?=
 =?utf-8?B?VU40cFhyU2RhUkhBWnNmWGtvSlhwSG96UzllV3ZvdjlJTXRxSUJSeUVvWWdP?=
 =?utf-8?B?MjNDaXgvUWNBeVVMbkRsZDlHUndML2ZpVzBnMzFQYTJKSy9ZZ1U3VEtYYUEx?=
 =?utf-8?B?VG4zZ2IydVBQdUMvWEFDa1RGOUpaelFkUnNkNVJ1ZHVzenBrZHhTdDdyZi93?=
 =?utf-8?B?OFlQbzFGRjVKOEkwR1J1blU3WFZWajIwVUQwWmVXdEc3d0JkRGFqRzRubHkx?=
 =?utf-8?B?bzBrZjJaZFpqUUFxQ25MeWc1bEJWa3c3Q0NscC9CbnRYdU1nSzNXSTFiKzNa?=
 =?utf-8?B?TnhqcUlSOEtMM0xVbzlYRWlmNlVEWUlHbkZQY1MxZG84dlF6aDJnSUlWbGRu?=
 =?utf-8?B?U2JKWEpZUTBPWFUrTDAxV0JKRnhxVnVQWCtIY1JrV0ttR3BPSmNUdjRSdE1Z?=
 =?utf-8?B?eFZ2VkVRM2VvWXlHa3AxaENRZ3lSY3YyMSs2MmlXMEdQNUg0a0tyWGt5b1hK?=
 =?utf-8?B?cW0wSlRUOXY5blMzd1BjZjhLZ1oyZFNUU3BYYUtxcE5JcEtZQjFkMGJzVG5U?=
 =?utf-8?B?T1Y5Y2lxZ2tHRnVSTGFXRGg5dGg4MGM0bjR2VTVtbjRhaXV4UjJYUHZUcDlZ?=
 =?utf-8?B?VTYxbG9NUVNNTEJCOUNKaHpkaC9GNWI1UEFhbkRNKzNYTFhMR0txblhvL24v?=
 =?utf-8?B?UldOUGdVQmxTMnhodmV0VlZLT1RLeVZNZUV3b1hNWkY4SUhhMWhFSTl4WHJI?=
 =?utf-8?B?Um5QdUdES2xDOEhnNWczWnF6NXdPRk43MTlZbno0OHVpclRaMXVXdndHbXBz?=
 =?utf-8?B?a2JkVnMrYjBERXc4TUFremhTSitJdFNOaWdMaXE5bEx1WE1hQlRJSHV1aFg5?=
 =?utf-8?B?RnNpaDk4eVpiK1hqTTZCcVNNSVNtcGIxdzBXTFBSYmlLU3ppN29lS2tTNUc2?=
 =?utf-8?B?aFV4YXJXRndHcStFOWtIL1c4ZS82Zit1SEt4dGNtZGFBdVBHVnQ0eS9uZmFk?=
 =?utf-8?B?Y2tDdzN2aXcxRDhVV0E5OUhIYUVTQzJBVXZOazRkdG9WWjlPLzBTT2ltMllN?=
 =?utf-8?B?VkxFSFJDQXRPY0IvUEg3Tko0M3kwYW50eFAyNm1iSCtaSGhmaGtZWnBYc0JH?=
 =?utf-8?B?b1h4MTJwWFM1UEhqa21TZWdGbWd6ZkFXRy9OVHdGNi9qU2tRNnlpWmJlZ1Ur?=
 =?utf-8?B?SzE4a1RWeFFHbHNnTnROaWQzNWNJd3R4SGNJYVlYL1ZZMlI1aFhleHhZOHky?=
 =?utf-8?B?ZGlSbmpsaEhTdXhpQWk1VExTNXJMQ3VJRi9yRUJ2VHBKamEyMTcxd1RubWg4?=
 =?utf-8?B?dGhUL0FYaXhJTWpoejlpQjNwTC8xcnhrNCtqQ0pLaGRSeVI4NkxkZ24xZHV6?=
 =?utf-8?B?cHY4c2VvclpzdTkxcE9jcW1VN01IeVU4d3hZS3VjWUM4c3VHZE9pa3hjUTE1?=
 =?utf-8?Q?WO+FALqtS0UdAzn3oHFVBRTjf?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 76b6e376-1a37-4bf4-e239-08dc3d68e0f2
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB5983.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Mar 2024 23:06:24.2012
 (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: RewLnkA9Ri1c+HTpiPmvDugOUS5Z1yXuwZIgJViWWZGgNoQfMEv8oHVtCyWmQXULNRDgb0baqFOUYL+493eQNQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6355
X-OriginatorOrg: intel.com


On 28/02/2024 3:41 pm, Sean Christopherson wrote:
> Prioritize private vs. shared gfn attribute checks above slot validity
> checks to ensure a consistent userspace ABI.  E.g. as is, KVM will exit to
> userspace if there is no memslot, but emulate accesses to the APIC access
> page even if the attributes mismatch.

IMHO, it would be helpful to explicitly say that, in the later case 
(emulate APIC access page) we still want to report MEMORY_FAULT error 
first (so that userspace can have chance to fixup, IIUC) instead of 
emulating directly, which will unlikely work.

Reviewed-by: Kai Huang <kai.huang@intel.com>

> 
> Fixes: 8dd2eee9d526 ("KVM: x86/mmu: Handle page fault for private memory")
> Cc: Yu Zhang <yu.c.zhang@linux.intel.com>
> Cc: Chao Peng <chao.p.peng@linux.intel.com>
> Cc: Fuad Tabba <tabba@google.com>
> Cc: Michael Roth <michael.roth@amd.com>
> Cc: Isaku Yamahata <isaku.yamahata@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/mmu/mmu.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 9206cfa58feb..58c5ae8be66c 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4365,11 +4365,6 @@ static int __kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
>   			return RET_PF_EMULATE;
>   	}
>   
> -	if (fault->is_private != kvm_mem_is_private(vcpu->kvm, fault->gfn)) {
> -		kvm_mmu_prepare_memory_fault_exit(vcpu, fault);
> -		return -EFAULT;
> -	}
> -
>   	if (fault->is_private)
>   		return kvm_faultin_pfn_private(vcpu, fault);
>   
> @@ -4410,6 +4405,16 @@ static int kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault,
>   	fault->mmu_seq = vcpu->kvm->mmu_invalidate_seq;
>   	smp_rmb();
>   
> +	/*
> +	 * Check for a private vs. shared mismatch *after* taking a snapshot of
> +	 * mmu_invalidate_seq, as changes to gfn attributes are guarded by the
> +	 * invalidation notifier.
> +	 */
> +	if (fault->is_private != kvm_mem_is_private(vcpu->kvm, fault->gfn)) {
> +		kvm_mmu_prepare_memory_fault_exit(vcpu, fault);
> +		return -EFAULT;
> +	}
> +
>   	/*
>   	 * Check for a relevant mmu_notifier invalidation event before getting
>   	 * the pfn from the primary MMU, and before acquiring mmu_lock.