From: Zhang Yanfei Subject: Re: [PATCH v9 1/2] x86/kexec: VMCLEAR VMCSs loaded on all cpus if necessary Date: Wed, 05 Dec 2012 15:55:37 +0800 Message-ID: <50BEFDF9.9080601@cn.fujitsu.com> References: <50B43299.9030409@cn.fujitsu.com> <50B432CA.70804@cn.fujitsu.com> <87zk1t4lt4.fsf@xmission.com> Mime-Version: 1.0 Content-Type: text/plain; charset=GB2312 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: "x86@kernel.org" , Marcelo Tosatti , Gleb Natapov , "kexec@lists.infradead.org" , "kvm@vger.kernel.org" , "linux-kernel@vger.kernel.org" To: "Eric W. Biederman" Return-path: Received: from cn.fujitsu.com ([222.73.24.84]:19726 "EHLO song.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1751585Ab2LEH5g convert rfc822-to-8bit (ORCPT ); Wed, 5 Dec 2012 02:57:36 -0500 In-Reply-To: <87zk1t4lt4.fsf@xmission.com> Sender: kvm-owner@vger.kernel.org List-ID: =D3=DA 2012=C4=EA12=D4=C205=C8=D5 04:14, Eric W. Biederman =D0=B4=B5=C0= : > Zhang Yanfei writes: >=20 >> This patch provides a way to VMCLEAR VMCSs related to guests >> on all cpus before executing the VMXOFF when doing kdump. This >> is used to ensure the VMCSs in the vmcore updated and >> non-corrupted. >=20 > Apologies for the delay I have been travelling, and I wanted > to at least read through the code. >=20 > Overall I think this is good but I have one nit, and I see one real > problem with this code. >=20 >> +/* >> + * This is used to VMCLEAR all VMCSs loaded on the >> + * processor. And when loading kvm_intel module, the >> + * callback function pointer will be assigned. >> + */ >> +void (*crash_vmclear_loaded_vmcss)(void) =3D NULL; >> +EXPORT_SYMBOL_GPL(crash_vmclear_loaded_vmcss); >> + >> +static inline void cpu_emergency_vmclear_loaded_vmcss(void) >> +{ >> + if (crash_vmclear_loaded_vmcss) >> + crash_vmclear_loaded_vmcss(); >> +} >=20 > The nit is the use of emergency instead of crash in the name. ok, emergency -> crash >=20 > The problem is that this is potentially a NULL pointer dereference if > kvm-intel is removed. The easist fix would be in your second patch t= o > just make it impossible to unload the kvm-intel module. Otherwise > there the deference of crash_vmclear_loaded_vmcss needs to be rcu > protected, with a syncrhonize_rcu after the pointer is set to NULL in > the unload path. Ah, thanks for this comment. I think I will use the rcu machanism to solve the problem. >=20 > Otherwise I have no objections to this code. Thanks for your review. I will update the patch and resend it. Thanks Zhang Yanfei