From: Konrad Rzeszutek Wilk <konrad@kernel.org>
To: Jan Beulich <JBeulich@suse.com>
Cc: Ian Campbell <ian.campbell@citrix.com>,
Dan Carpenter <dan.carpenter@oracle.com>,
xen-devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH, v2] linux-2.6.18: fix hypercall fallback code for very old hypervisors
Date: Fri, 19 Oct 2012 15:23:01 -0400 [thread overview]
Message-ID: <20121019192300.GA20513@phenom.dumpdata.com> (raw)
In-Reply-To: <507D814D02000078000A1B51@nat28.tlf.novell.com>
On Tue, Oct 16, 2012 at 02:46:21PM +0100, Jan Beulich wrote:
> While copying the argument structures in HYPERVISOR_event_channel_op()
> and HYPERVISOR_physdev_op() into the local variable is sufficiently
> safe even if the actual structure is smaller than the container one,
> copying back eventual output values the same way isn't: This may
> collide with on-stack variables (particularly "rc") which may change
> between the first and second memcpy() (i.e. the second memcpy() could
> discard that change).
>
> Move the fallback code into out-of-line functions, and handle all of
> the operations known by this old a hypervisor individually: Some don't
> require copying back anything at all, and for the rest use the
> individual argument structures' sizes rather than the container's.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Reduce #define/#undef usage in HYPERVISOR_physdev_op_compat().
>
> --- a/drivers/xen/core/Makefile
> +++ b/drivers/xen/core/Makefile
> @@ -2,7 +2,7 @@
> # Makefile for the linux kernel.
> #
>
> -obj-y := evtchn.o gnttab.o features.o reboot.o machine_reboot.o
> +obj-y := evtchn.o gnttab.o features.o reboot.o machine_reboot.o fallback.o
>
> obj-$(CONFIG_PCI) += pci.o
> obj-$(CONFIG_XEN_PRIVILEGED_GUEST) += firmware.o
> --- /dev/null
> +++ b/drivers/xen/core/fallback.c
> @@ -0,0 +1,84 @@
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <asm/bug.h>
> +#include <asm/hypervisor.h>
> +
> +#if 1//temp CONFIG_XEN_COMPAT <= 0x030002
Um?
> +
> +#include <xen/interface/event_channel.h>
> +#include <xen/interface/physdev.h>
> +
> +int HYPERVISOR_event_channel_op_compat(int cmd, void *arg)
> +{
> + struct evtchn_op op;
> + int rc;
> +
> + op.cmd = cmd;
> + memcpy(&op.u, arg, sizeof(op.u));
> + rc = _hypercall1(int, event_channel_op_compat, &op);
> +
> + switch (cmd) {
> + case EVTCHNOP_close:
> + case EVTCHNOP_send:
> + case EVTCHNOP_bind_vcpu:
> + case EVTCHNOP_unmask:
> + /* no output */
> + break;
> +
> +#define COPY_BACK(eop) \
> + case EVTCHNOP_##eop: \
> + memcpy(arg, &op.u.eop, sizeof(op.u.eop)); \
> + break
> +
> + COPY_BACK(bind_interdomain);
> + COPY_BACK(bind_virq);
> + COPY_BACK(bind_pirq);
> + COPY_BACK(status);
> + COPY_BACK(alloc_unbound);
> + COPY_BACK(bind_ipi);
> +#undef COPY_BACK
> +
> + default:
> + WARN_ON(rc != -ENOSYS);
> + break;
> + }
> +
> + return rc;
> +}
> +
> +int HYPERVISOR_physdev_op_compat(int cmd, void *arg)
> +{
> + struct physdev_op op;
> + int rc;
> +
> + op.cmd = cmd;
> + memcpy(&op.u, arg, sizeof(op.u));
> + rc = _hypercall1(int, physdev_op_compat, &op);
> +
> + switch (cmd) {
> + case PHYSDEVOP_IRQ_UNMASK_NOTIFY:
> + case PHYSDEVOP_set_iopl:
> + case PHYSDEVOP_set_iobitmap:
> + case PHYSDEVOP_apic_write:
> + /* no output */
> + break;
> +
> +#define COPY_BACK(pop, fld) \
> + case PHYSDEVOP_##pop: \
> + memcpy(arg, &op.u.fld, sizeof(op.u.fld)); \
> + break
> +
> + COPY_BACK(irq_status_query, irq_status_query);
> + COPY_BACK(apic_read, apic_op);
> + COPY_BACK(ASSIGN_VECTOR, irq_op);
> +#undef COPY_BACK
> +
> + default:
> + WARN_ON(rc != -ENOSYS);
> + break;
> + }
> +
> + return rc;
> +}
> +
> +#endif /* CONFIG_XEN_COMPAT <= 0x030002 */
> --- a/include/asm-i386/mach-xen/asm/hypercall.h
> +++ b/include/asm-i386/mach-xen/asm/hypercall.h
> @@ -33,7 +33,6 @@
> #ifndef __HYPERCALL_H__
> #define __HYPERCALL_H__
>
> -#include <linux/string.h> /* memcpy() */
> #include <linux/stringify.h>
>
> #ifndef __HYPERVISOR_H__
> @@ -128,6 +127,11 @@
> __res; \
> })
>
> +#if CONFIG_XEN_COMPAT <= 0x030002
> +int __must_check HYPERVISOR_event_channel_op_compat(int, void *);
> +int __must_check HYPERVISOR_physdev_op_compat(int, void *);
> +#endif
> +
> static inline int __must_check
> HYPERVISOR_set_trap_table(
> const trap_info_t *table)
> @@ -267,13 +271,8 @@ HYPERVISOR_event_channel_op(
> int rc = _hypercall2(int, event_channel_op, cmd, arg);
>
> #if CONFIG_XEN_COMPAT <= 0x030002
> - if (unlikely(rc == -ENOSYS)) {
> - struct evtchn_op op;
> - op.cmd = cmd;
> - memcpy(&op.u, arg, sizeof(op.u));
> - rc = _hypercall1(int, event_channel_op_compat, &op);
> - memcpy(arg, &op.u, sizeof(op.u));
> - }
> + if (unlikely(rc == -ENOSYS))
> + rc = HYPERVISOR_event_channel_op_compat(cmd, arg);
> #endif
>
> return rc;
> @@ -300,13 +299,8 @@ HYPERVISOR_physdev_op(
> int rc = _hypercall2(int, physdev_op, cmd, arg);
>
> #if CONFIG_XEN_COMPAT <= 0x030002
> - if (unlikely(rc == -ENOSYS)) {
> - struct physdev_op op;
> - op.cmd = cmd;
> - memcpy(&op.u, arg, sizeof(op.u));
> - rc = _hypercall1(int, physdev_op_compat, &op);
> - memcpy(arg, &op.u, sizeof(op.u));
> - }
> + if (unlikely(rc == -ENOSYS))
> + rc = HYPERVISOR_physdev_op_compat(cmd, arg);
> #endif
>
> return rc;
> --- a/include/asm-i386/mach-xen/asm/hypervisor.h
> +++ b/include/asm-i386/mach-xen/asm/hypervisor.h
> @@ -39,8 +39,6 @@
> #include <linux/errno.h>
> #include <xen/interface/xen.h>
> #include <xen/interface/platform.h>
> -#include <xen/interface/event_channel.h>
> -#include <xen/interface/physdev.h>
> #include <xen/interface/sched.h>
> #include <xen/interface/nmi.h>
> #include <xen/interface/tmem.h>
>
>
> fix hypercall fallback code for very old hypervisors
>
> While copying the argument structures in HYPERVISOR_event_channel_op()
> and HYPERVISOR_physdev_op() into the local variable is sufficiently
> safe even if the actual structure is smaller than the container one,
> copying back eventual output values the same way isn't: This may
> collide with on-stack variables (particularly "rc") which may change
> between the first and second memcpy() (i.e. the second memcpy() could
> discard that change).
>
> Move the fallback code into out-of-line functions, and handle all of
> the operations known by this old a hypervisor individually: Some don't
> require copying back anything at all, and for the rest use the
> individual argument structures' sizes rather than the container's.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Reduce #define/#undef usage in HYPERVISOR_physdev_op_compat().
>
> --- a/drivers/xen/core/Makefile
> +++ b/drivers/xen/core/Makefile
> @@ -2,7 +2,7 @@
> # Makefile for the linux kernel.
> #
>
> -obj-y := evtchn.o gnttab.o features.o reboot.o machine_reboot.o
> +obj-y := evtchn.o gnttab.o features.o reboot.o machine_reboot.o fallback.o
>
> obj-$(CONFIG_PCI) += pci.o
> obj-$(CONFIG_XEN_PRIVILEGED_GUEST) += firmware.o
> --- /dev/null
> +++ b/drivers/xen/core/fallback.c
> @@ -0,0 +1,84 @@
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <asm/bug.h>
> +#include <asm/hypervisor.h>
> +
> +#if 1//temp CONFIG_XEN_COMPAT <= 0x030002
> +
> +#include <xen/interface/event_channel.h>
> +#include <xen/interface/physdev.h>
> +
> +int HYPERVISOR_event_channel_op_compat(int cmd, void *arg)
> +{
> + struct evtchn_op op;
> + int rc;
> +
> + op.cmd = cmd;
> + memcpy(&op.u, arg, sizeof(op.u));
> + rc = _hypercall1(int, event_channel_op_compat, &op);
> +
> + switch (cmd) {
> + case EVTCHNOP_close:
> + case EVTCHNOP_send:
> + case EVTCHNOP_bind_vcpu:
> + case EVTCHNOP_unmask:
> + /* no output */
> + break;
> +
> +#define COPY_BACK(eop) \
> + case EVTCHNOP_##eop: \
> + memcpy(arg, &op.u.eop, sizeof(op.u.eop)); \
> + break
> +
> + COPY_BACK(bind_interdomain);
> + COPY_BACK(bind_virq);
> + COPY_BACK(bind_pirq);
> + COPY_BACK(status);
> + COPY_BACK(alloc_unbound);
> + COPY_BACK(bind_ipi);
> +#undef COPY_BACK
> +
> + default:
> + WARN_ON(rc != -ENOSYS);
> + break;
> + }
> +
> + return rc;
> +}
> +
> +int HYPERVISOR_physdev_op_compat(int cmd, void *arg)
> +{
> + struct physdev_op op;
> + int rc;
> +
> + op.cmd = cmd;
> + memcpy(&op.u, arg, sizeof(op.u));
> + rc = _hypercall1(int, physdev_op_compat, &op);
> +
> + switch (cmd) {
> + case PHYSDEVOP_IRQ_UNMASK_NOTIFY:
> + case PHYSDEVOP_set_iopl:
> + case PHYSDEVOP_set_iobitmap:
> + case PHYSDEVOP_apic_write:
> + /* no output */
> + break;
> +
> +#define COPY_BACK(pop, fld) \
> + case PHYSDEVOP_##pop: \
> + memcpy(arg, &op.u.fld, sizeof(op.u.fld)); \
> + break
> +
> + COPY_BACK(irq_status_query, irq_status_query);
> + COPY_BACK(apic_read, apic_op);
> + COPY_BACK(ASSIGN_VECTOR, irq_op);
> +#undef COPY_BACK
> +
> + default:
> + WARN_ON(rc != -ENOSYS);
> + break;
> + }
> +
> + return rc;
> +}
> +
> +#endif /* CONFIG_XEN_COMPAT <= 0x030002 */
> --- a/include/asm-i386/mach-xen/asm/hypercall.h
> +++ b/include/asm-i386/mach-xen/asm/hypercall.h
> @@ -33,7 +33,6 @@
> #ifndef __HYPERCALL_H__
> #define __HYPERCALL_H__
>
> -#include <linux/string.h> /* memcpy() */
> #include <linux/stringify.h>
>
> #ifndef __HYPERVISOR_H__
> @@ -128,6 +127,11 @@
> __res; \
> })
>
> +#if CONFIG_XEN_COMPAT <= 0x030002
> +int __must_check HYPERVISOR_event_channel_op_compat(int, void *);
> +int __must_check HYPERVISOR_physdev_op_compat(int, void *);
> +#endif
> +
> static inline int __must_check
> HYPERVISOR_set_trap_table(
> const trap_info_t *table)
> @@ -267,13 +271,8 @@ HYPERVISOR_event_channel_op(
> int rc = _hypercall2(int, event_channel_op, cmd, arg);
>
> #if CONFIG_XEN_COMPAT <= 0x030002
> - if (unlikely(rc == -ENOSYS)) {
> - struct evtchn_op op;
> - op.cmd = cmd;
> - memcpy(&op.u, arg, sizeof(op.u));
> - rc = _hypercall1(int, event_channel_op_compat, &op);
> - memcpy(arg, &op.u, sizeof(op.u));
> - }
> + if (unlikely(rc == -ENOSYS))
> + rc = HYPERVISOR_event_channel_op_compat(cmd, arg);
> #endif
>
> return rc;
> @@ -300,13 +299,8 @@ HYPERVISOR_physdev_op(
> int rc = _hypercall2(int, physdev_op, cmd, arg);
>
> #if CONFIG_XEN_COMPAT <= 0x030002
> - if (unlikely(rc == -ENOSYS)) {
> - struct physdev_op op;
> - op.cmd = cmd;
> - memcpy(&op.u, arg, sizeof(op.u));
> - rc = _hypercall1(int, physdev_op_compat, &op);
> - memcpy(arg, &op.u, sizeof(op.u));
> - }
> + if (unlikely(rc == -ENOSYS))
> + rc = HYPERVISOR_physdev_op_compat(cmd, arg);
> #endif
>
> return rc;
> --- a/include/asm-i386/mach-xen/asm/hypervisor.h
> +++ b/include/asm-i386/mach-xen/asm/hypervisor.h
> @@ -39,8 +39,6 @@
> #include <linux/errno.h>
> #include <xen/interface/xen.h>
> #include <xen/interface/platform.h>
> -#include <xen/interface/event_channel.h>
> -#include <xen/interface/physdev.h>
> #include <xen/interface/sched.h>
> #include <xen/interface/nmi.h>
> #include <xen/interface/tmem.h>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2012-10-19 19:23 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-16 12:29 [PATCH] linux-2.6.18: fix hypercall fallback code for very old hypervisors Jan Beulich
2012-10-16 13:02 ` Ian Campbell
2012-10-16 13:46 ` [PATCH, v2] " Jan Beulich
2012-10-19 19:23 ` Konrad Rzeszutek Wilk [this message]
2012-10-22 6:47 ` Jan Beulich
2012-10-19 19:52 ` Konrad Rzeszutek Wilk
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=20121019192300.GA20513@phenom.dumpdata.com \
--to=konrad@kernel.org \
--cc=JBeulich@suse.com \
--cc=dan.carpenter@oracle.com \
--cc=ian.campbell@citrix.com \
--cc=xen-devel@lists.xen.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).