From: ebiederm@xmission.com (Eric W. Biederman)
To: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>,
Gleb Natapov <gleb@redhat.com>,
"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
"x86@kernel.org" <x86@kernel.org>,
"kexec@lists.infradead.org" <kexec@lists.infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v9 1/2] x86/kexec: VMCLEAR VMCSs loaded on all cpus if necessary
Date: Tue, 04 Dec 2012 12:14:15 -0800 [thread overview]
Message-ID: <87zk1t4lt4.fsf@xmission.com> (raw)
In-Reply-To: <50B432CA.70804@cn.fujitsu.com> (Zhang Yanfei's message of "Tue, 27 Nov 2012 11:26:02 +0800")
Zhang Yanfei <zhangyanfei@cn.fujitsu.com> writes:
> 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.
Apologies for the delay I have been travelling, and I wanted
to at least read through the code.
Overall I think this is good but I have one nit, and I see one real
problem with this code.
> +/*
> + * 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) = 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();
> +}
The nit is the use of emergency instead of crash in the name.
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 to
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.
Otherwise I have no objections to this code.
Eric
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
WARNING: multiple messages have this Message-ID (diff)
From: ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman)
To: Zhang Yanfei <zhangyanfei-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: Marcelo Tosatti
<mtosatti-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>,
Gleb Natapov <gleb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>,
"kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
"x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org"
<x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
"kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org"
<kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>,
"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH v9 1/2] x86/kexec: VMCLEAR VMCSs loaded on all cpus if necessary
Date: Tue, 04 Dec 2012 12:14:15 -0800 [thread overview]
Message-ID: <87zk1t4lt4.fsf@xmission.com> (raw)
In-Reply-To: <50B432CA.70804-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> (Zhang Yanfei's message of "Tue, 27 Nov 2012 11:26:02 +0800")
Zhang Yanfei <zhangyanfei-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> writes:
> 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.
Apologies for the delay I have been travelling, and I wanted
to at least read through the code.
Overall I think this is good but I have one nit, and I see one real
problem with this code.
> +/*
> + * 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) = 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();
> +}
The nit is the use of emergency instead of crash in the name.
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 to
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.
Otherwise I have no objections to this code.
Eric
WARNING: multiple messages have this Message-ID (diff)
From: ebiederm@xmission.com (Eric W. Biederman)
To: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
Cc: "x86\@kernel.org" <x86@kernel.org>,
Marcelo Tosatti <mtosatti@redhat.com>,
Gleb Natapov <gleb@redhat.com>,
"kexec\@lists.infradead.org" <kexec@lists.infradead.org>,
"kvm\@vger.kernel.org" <kvm@vger.kernel.org>,
"linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v9 1/2] x86/kexec: VMCLEAR VMCSs loaded on all cpus if necessary
Date: Tue, 04 Dec 2012 12:14:15 -0800 [thread overview]
Message-ID: <87zk1t4lt4.fsf@xmission.com> (raw)
In-Reply-To: <50B432CA.70804@cn.fujitsu.com> (Zhang Yanfei's message of "Tue, 27 Nov 2012 11:26:02 +0800")
Zhang Yanfei <zhangyanfei@cn.fujitsu.com> writes:
> 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.
Apologies for the delay I have been travelling, and I wanted
to at least read through the code.
Overall I think this is good but I have one nit, and I see one real
problem with this code.
> +/*
> + * 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) = 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();
> +}
The nit is the use of emergency instead of crash in the name.
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 to
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.
Otherwise I have no objections to this code.
Eric
next prev parent reply other threads:[~2012-12-04 20:14 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-11-27 3:25 [PATCH v9 0/2] x86: vmclear vmcss on all cpus when doing kdump if necessary Zhang Yanfei
2012-11-27 3:25 ` Zhang Yanfei
2012-11-27 3:26 ` [PATCH v9 1/2] x86/kexec: VMCLEAR VMCSs loaded on all cpus " Zhang Yanfei
2012-11-27 3:26 ` Zhang Yanfei
2012-11-27 12:18 ` Gleb Natapov
2012-11-27 12:18 ` Gleb Natapov
2012-12-03 13:46 ` Gleb Natapov
2012-12-03 13:46 ` Gleb Natapov
2012-12-03 13:46 ` Gleb Natapov
2012-12-04 20:14 ` Eric W. Biederman [this message]
2012-12-04 20:14 ` Eric W. Biederman
2012-12-04 20:14 ` Eric W. Biederman
2012-12-05 7:55 ` Zhang Yanfei
2012-12-05 7:55 ` Zhang Yanfei
2012-11-27 3:26 ` [PATCH v9 2/2] KVM-INTEL: provide the vmclear function and a bitmap to support VMCLEAR in kdump Zhang Yanfei
2012-11-27 3:26 ` Zhang Yanfei
2012-11-27 3:26 ` Zhang Yanfei
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87zk1t4lt4.fsf@xmission.com \
--to=ebiederm@xmission.com \
--cc=gleb@redhat.com \
--cc=kexec@lists.infradead.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mtosatti@redhat.com \
--cc=x86@kernel.org \
--cc=zhangyanfei@cn.fujitsu.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.