* Re: [PATCH 1/2] xen/hypercall: fix hypercall fallback code for very old hypervisors
@ 2012-10-30 15:44 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 11+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-10-30 15:44 UTC (permalink / raw)
To: linux-kernel, xen-devel; +Cc: Jan Beulich
On Mon, Oct 29, 2012 at 10:08:17AM -0400, Konrad Rzeszutek Wilk wrote:
> From: Jan Beulich <jbeulich@suse.com>
>
> 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().]
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
And it looks like I get
ERROR: "HYPERVISOR_event_channel_op_compat" [drivers/xen/xen-evtchn.ko] undefined!
when I build xen-evtchn as module. Jan did you encounter this issue on 2.6.18?
> ---
> arch/x86/include/asm/xen/hypercall.h | 21 +++------
> drivers/xen/Makefile | 2 +-
> drivers/xen/fallback.c | 78 ++++++++++++++++++++++++++++++++++
> 3 files changed, 86 insertions(+), 15 deletions(-)
> create mode 100644 drivers/xen/fallback.c
>
> diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
> index 59c226d..c812360 100644
> --- a/arch/x86/include/asm/xen/hypercall.h
> +++ b/arch/x86/include/asm/xen/hypercall.h
> @@ -359,18 +359,14 @@ HYPERVISOR_update_va_mapping(unsigned long va, pte_t new_val,
> return _hypercall4(int, update_va_mapping, va,
> new_val.pte, new_val.pte >> 32, flags);
> }
> +int __must_check HYPERVISOR_event_channel_op_compat(int, void *);
>
> static inline int
> HYPERVISOR_event_channel_op(int cmd, void *arg)
> {
> int rc = _hypercall2(int, event_channel_op, cmd, arg);
> - 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);
> return rc;
> }
>
> @@ -386,17 +382,14 @@ HYPERVISOR_console_io(int cmd, int count, char *str)
> return _hypercall3(int, console_io, cmd, count, str);
> }
>
> +int __must_check HYPERVISOR_physdev_op_compat(int, void *);
> +
> static inline int
> HYPERVISOR_physdev_op(int cmd, void *arg)
> {
> int rc = _hypercall2(int, physdev_op, cmd, arg);
> - 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);
> return rc;
> }
>
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index 0e863703..46de6cd 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -2,7 +2,7 @@ ifneq ($(CONFIG_ARM),y)
> obj-y += manage.o balloon.o
> obj-$(CONFIG_HOTPLUG_CPU) += cpu_hotplug.o
> endif
> -obj-y += grant-table.o features.o events.o
> +obj-y += grant-table.o features.o events.o fallback.o
> obj-y += xenbus/
>
> nostackp := $(call cc-option, -fno-stack-protector)
> diff --git a/drivers/xen/fallback.c b/drivers/xen/fallback.c
> new file mode 100644
> index 0000000..0bdc468
> --- /dev/null
> +++ b/drivers/xen/fallback.c
> @@ -0,0 +1,78 @@
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/bug.h>
> +#include <asm/hypervisor.h>
> +#include <asm/xen/hypercall.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;
> +}
> --
> 1.7.7.6
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] xen/hypercall: fix hypercall fallback code for very old hypervisors
2012-10-30 15:44 ` Konrad Rzeszutek Wilk
(?)
@ 2012-10-31 8:55 ` Jan Beulich
2012-11-02 16:44 ` Konrad Rzeszutek Wilk
2012-11-02 16:44 ` Konrad Rzeszutek Wilk
-1 siblings, 2 replies; 11+ messages in thread
From: Jan Beulich @ 2012-10-31 8:55 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: xen-devel, linux-kernel
>>> On 30.10.12 at 16:44, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Mon, Oct 29, 2012 at 10:08:17AM -0400, Konrad Rzeszutek Wilk wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>>
>> 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().]
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> And it looks like I get
>
> ERROR: "HYPERVISOR_event_channel_op_compat" [drivers/xen/xen-evtchn.ko]
> undefined!
>
> when I build xen-evtchn as module. Jan did you encounter this issue on
> 2.6.18?
No - the event channel driver there can't be built as a module, and
for the forward ported kernels I apparently never tried to build with
a compat setting of 3.0.2 or less (and I didn't care that much either
because the oldest we're actually concerned about is 3.0.4 to cover
some of those very old EC2 systems; I'll add the export at the right
point nevertheless).
Jan
>> ---
>> arch/x86/include/asm/xen/hypercall.h | 21 +++------
>> drivers/xen/Makefile | 2 +-
>> drivers/xen/fallback.c | 78
> ++++++++++++++++++++++++++++++++++
>> 3 files changed, 86 insertions(+), 15 deletions(-)
>> create mode 100644 drivers/xen/fallback.c
>>
>> diff --git a/arch/x86/include/asm/xen/hypercall.h
> b/arch/x86/include/asm/xen/hypercall.h
>> index 59c226d..c812360 100644
>> --- a/arch/x86/include/asm/xen/hypercall.h
>> +++ b/arch/x86/include/asm/xen/hypercall.h
>> @@ -359,18 +359,14 @@ HYPERVISOR_update_va_mapping(unsigned long va, pte_t
> new_val,
>> return _hypercall4(int, update_va_mapping, va,
>> new_val.pte, new_val.pte >> 32, flags);
>> }
>> +int __must_check HYPERVISOR_event_channel_op_compat(int, void *);
>>
>> static inline int
>> HYPERVISOR_event_channel_op(int cmd, void *arg)
>> {
>> int rc = _hypercall2(int, event_channel_op, cmd, arg);
>> - 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);
>> return rc;
>> }
>>
>> @@ -386,17 +382,14 @@ HYPERVISOR_console_io(int cmd, int count, char *str)
>> return _hypercall3(int, console_io, cmd, count, str);
>> }
>>
>> +int __must_check HYPERVISOR_physdev_op_compat(int, void *);
>> +
>> static inline int
>> HYPERVISOR_physdev_op(int cmd, void *arg)
>> {
>> int rc = _hypercall2(int, physdev_op, cmd, arg);
>> - 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);
>> return rc;
>> }
>>
>> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
>> index 0e863703..46de6cd 100644
>> --- a/drivers/xen/Makefile
>> +++ b/drivers/xen/Makefile
>> @@ -2,7 +2,7 @@ ifneq ($(CONFIG_ARM),y)
>> obj-y += manage.o balloon.o
>> obj-$(CONFIG_HOTPLUG_CPU) += cpu_hotplug.o
>> endif
>> -obj-y += grant-table.o features.o events.o
>> +obj-y += grant-table.o features.o events.o fallback.o
>> obj-y += xenbus/
>>
>> nostackp := $(call cc-option, -fno-stack-protector)
>> diff --git a/drivers/xen/fallback.c b/drivers/xen/fallback.c
>> new file mode 100644
>> index 0000000..0bdc468
>> --- /dev/null
>> +++ b/drivers/xen/fallback.c
>> @@ -0,0 +1,78 @@
>> +#include <linux/kernel.h>
>> +#include <linux/string.h>
>> +#include <linux/bug.h>
>> +#include <asm/hypervisor.h>
>> +#include <asm/xen/hypercall.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;
>> +}
>> --
>> 1.7.7.6
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] xen/hypercall: fix hypercall fallback code for very old hypervisors
2012-10-31 8:55 ` Jan Beulich
@ 2012-11-02 16:44 ` Konrad Rzeszutek Wilk
2012-11-02 16:44 ` Konrad Rzeszutek Wilk
1 sibling, 0 replies; 11+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-11-02 16:44 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, linux-kernel, Konrad Rzeszutek Wilk
On Wed, Oct 31, 2012 at 08:55:54AM +0000, Jan Beulich wrote:
> >>> On 30.10.12 at 16:44, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > On Mon, Oct 29, 2012 at 10:08:17AM -0400, Konrad Rzeszutek Wilk wrote:
> >> From: Jan Beulich <jbeulich@suse.com>
> >>
> >> 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().]
> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >
> > And it looks like I get
> >
> > ERROR: "HYPERVISOR_event_channel_op_compat" [drivers/xen/xen-evtchn.ko]
> > undefined!
> >
> > when I build xen-evtchn as module. Jan did you encounter this issue on
> > 2.6.18?
>
> No - the event channel driver there can't be built as a module, and
> for the forward ported kernels I apparently never tried to build with
> a compat setting of 3.0.2 or less (and I didn't care that much either
> because the oldest we're actually concerned about is 3.0.4 to cover
> some of those very old EC2 systems; I'll add the export at the right
> point nevertheless).
Ok, ended up with this version which I was thinking to for v3.7-rc5:
>From 4ae4c7658a7c0501521c422d618038587c3edeca Mon Sep 17 00:00:00 2001
From: Jan Beulich <jbeulich@suse.com>
Date: Fri, 19 Oct 2012 15:25:37 -0400
Subject: [PATCH] xen/hypercall: 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().]
[v3: Fix compile errors when modules use said hypercalls]
[v4: Add xen_ prefix to the HYPERCALL_..]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
arch/x86/include/asm/xen/hypercall.h | 21 ++++------
drivers/xen/Makefile | 2 +-
drivers/xen/fallback.c | 81 ++++++++++++++++++++++++++++++++++++
3 files changed, 89 insertions(+), 15 deletions(-)
create mode 100644 drivers/xen/fallback.c
diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index 59c226d..4055421 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -359,18 +359,14 @@ HYPERVISOR_update_va_mapping(unsigned long va, pte_t new_val,
return _hypercall4(int, update_va_mapping, va,
new_val.pte, new_val.pte >> 32, flags);
}
+extern int __must_check xen_HYPERVISOR_event_channel_op_compat(int, void *);
static inline int
HYPERVISOR_event_channel_op(int cmd, void *arg)
{
int rc = _hypercall2(int, event_channel_op, cmd, arg);
- 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 = xen_HYPERVISOR_event_channel_op_compat(cmd, arg);
return rc;
}
@@ -386,17 +382,14 @@ HYPERVISOR_console_io(int cmd, int count, char *str)
return _hypercall3(int, console_io, cmd, count, str);
}
+extern int __must_check xen_HYPERVISOR_physdev_op_compat(int, void *);
+
static inline int
HYPERVISOR_physdev_op(int cmd, void *arg)
{
int rc = _hypercall2(int, physdev_op, cmd, arg);
- 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 = xen_HYPERVISOR_physdev_op_compat(cmd, arg);
return rc;
}
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index 0e863703..46de6cd 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -2,7 +2,7 @@ ifneq ($(CONFIG_ARM),y)
obj-y += manage.o balloon.o
obj-$(CONFIG_HOTPLUG_CPU) += cpu_hotplug.o
endif
-obj-y += grant-table.o features.o events.o
+obj-y += grant-table.o features.o events.o fallback.o
obj-y += xenbus/
nostackp := $(call cc-option, -fno-stack-protector)
diff --git a/drivers/xen/fallback.c b/drivers/xen/fallback.c
new file mode 100644
index 0000000..b29ce32
--- /dev/null
+++ b/drivers/xen/fallback.c
@@ -0,0 +1,81 @@
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/bug.h>
+#include <linux/export.h>
+#include <asm/hypervisor.h>
+#include <asm/xen/hypercall.h>
+
+int xen_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;
+}
+EXPORT_SYMBOL_GPL(xen_HYPERVISOR_event_channel_op_compat);
+
+int xen_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;
+}
+EXPORT_SYMBOL_GPL(xen_HYPERVISOR_physdev_op_compat);
--
1.7.11.7
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] xen/hypercall: fix hypercall fallback code for very old hypervisors
2012-10-31 8:55 ` Jan Beulich
2012-11-02 16:44 ` Konrad Rzeszutek Wilk
@ 2012-11-02 16:44 ` Konrad Rzeszutek Wilk
2012-11-02 16:57 ` Jan Beulich
2012-11-02 16:57 ` Jan Beulich
1 sibling, 2 replies; 11+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-11-02 16:44 UTC (permalink / raw)
To: Jan Beulich; +Cc: Konrad Rzeszutek Wilk, xen-devel, linux-kernel
On Wed, Oct 31, 2012 at 08:55:54AM +0000, Jan Beulich wrote:
> >>> On 30.10.12 at 16:44, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > On Mon, Oct 29, 2012 at 10:08:17AM -0400, Konrad Rzeszutek Wilk wrote:
> >> From: Jan Beulich <jbeulich@suse.com>
> >>
> >> 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().]
> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >
> > And it looks like I get
> >
> > ERROR: "HYPERVISOR_event_channel_op_compat" [drivers/xen/xen-evtchn.ko]
> > undefined!
> >
> > when I build xen-evtchn as module. Jan did you encounter this issue on
> > 2.6.18?
>
> No - the event channel driver there can't be built as a module, and
> for the forward ported kernels I apparently never tried to build with
> a compat setting of 3.0.2 or less (and I didn't care that much either
> because the oldest we're actually concerned about is 3.0.4 to cover
> some of those very old EC2 systems; I'll add the export at the right
> point nevertheless).
Ok, ended up with this version which I was thinking to for v3.7-rc5:
>From 4ae4c7658a7c0501521c422d618038587c3edeca Mon Sep 17 00:00:00 2001
From: Jan Beulich <jbeulich@suse.com>
Date: Fri, 19 Oct 2012 15:25:37 -0400
Subject: [PATCH] xen/hypercall: 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().]
[v3: Fix compile errors when modules use said hypercalls]
[v4: Add xen_ prefix to the HYPERCALL_..]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
arch/x86/include/asm/xen/hypercall.h | 21 ++++------
drivers/xen/Makefile | 2 +-
drivers/xen/fallback.c | 81 ++++++++++++++++++++++++++++++++++++
3 files changed, 89 insertions(+), 15 deletions(-)
create mode 100644 drivers/xen/fallback.c
diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index 59c226d..4055421 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -359,18 +359,14 @@ HYPERVISOR_update_va_mapping(unsigned long va, pte_t new_val,
return _hypercall4(int, update_va_mapping, va,
new_val.pte, new_val.pte >> 32, flags);
}
+extern int __must_check xen_HYPERVISOR_event_channel_op_compat(int, void *);
static inline int
HYPERVISOR_event_channel_op(int cmd, void *arg)
{
int rc = _hypercall2(int, event_channel_op, cmd, arg);
- 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 = xen_HYPERVISOR_event_channel_op_compat(cmd, arg);
return rc;
}
@@ -386,17 +382,14 @@ HYPERVISOR_console_io(int cmd, int count, char *str)
return _hypercall3(int, console_io, cmd, count, str);
}
+extern int __must_check xen_HYPERVISOR_physdev_op_compat(int, void *);
+
static inline int
HYPERVISOR_physdev_op(int cmd, void *arg)
{
int rc = _hypercall2(int, physdev_op, cmd, arg);
- 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 = xen_HYPERVISOR_physdev_op_compat(cmd, arg);
return rc;
}
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index 0e863703..46de6cd 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -2,7 +2,7 @@ ifneq ($(CONFIG_ARM),y)
obj-y += manage.o balloon.o
obj-$(CONFIG_HOTPLUG_CPU) += cpu_hotplug.o
endif
-obj-y += grant-table.o features.o events.o
+obj-y += grant-table.o features.o events.o fallback.o
obj-y += xenbus/
nostackp := $(call cc-option, -fno-stack-protector)
diff --git a/drivers/xen/fallback.c b/drivers/xen/fallback.c
new file mode 100644
index 0000000..b29ce32
--- /dev/null
+++ b/drivers/xen/fallback.c
@@ -0,0 +1,81 @@
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/bug.h>
+#include <linux/export.h>
+#include <asm/hypervisor.h>
+#include <asm/xen/hypercall.h>
+
+int xen_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;
+}
+EXPORT_SYMBOL_GPL(xen_HYPERVISOR_event_channel_op_compat);
+
+int xen_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;
+}
+EXPORT_SYMBOL_GPL(xen_HYPERVISOR_physdev_op_compat);
--
1.7.11.7
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] xen/hypercall: fix hypercall fallback code for very old hypervisors
2012-11-02 16:44 ` Konrad Rzeszutek Wilk
@ 2012-11-02 16:57 ` Jan Beulich
2012-11-02 16:57 ` Jan Beulich
1 sibling, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2012-11-02 16:57 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: xen-devel, Konrad Rzeszutek Wilk, linux-kernel
>>> On 02.11.12 at 17:44, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> --- a/arch/x86/include/asm/xen/hypercall.h
> +++ b/arch/x86/include/asm/xen/hypercall.h
> @@ -359,18 +359,14 @@ HYPERVISOR_update_va_mapping(unsigned long va, pte_t new_val,
> return _hypercall4(int, update_va_mapping, va,
> new_val.pte, new_val.pte >> 32, flags);
> }
> +extern int __must_check xen_HYPERVISOR_event_channel_op_compat(int, void *);
Why don't you drop the HYPERVISOR_ now that you added the xen_?
>...
> +EXPORT_SYMBOL_GPL(xen_HYPERVISOR_event_channel_op_compat);
While this export is necessary, ...
>...
> +EXPORT_SYMBOL_GPL(xen_HYPERVISOR_physdev_op_compat);
... I would recommend not adding this one without need.
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] xen/hypercall: fix hypercall fallback code for very old hypervisors
2012-11-02 16:44 ` Konrad Rzeszutek Wilk
2012-11-02 16:57 ` Jan Beulich
@ 2012-11-02 16:57 ` Jan Beulich
1 sibling, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2012-11-02 16:57 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: Konrad Rzeszutek Wilk, linux-kernel, xen-devel
>>> On 02.11.12 at 17:44, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> --- a/arch/x86/include/asm/xen/hypercall.h
> +++ b/arch/x86/include/asm/xen/hypercall.h
> @@ -359,18 +359,14 @@ HYPERVISOR_update_va_mapping(unsigned long va, pte_t new_val,
> return _hypercall4(int, update_va_mapping, va,
> new_val.pte, new_val.pte >> 32, flags);
> }
> +extern int __must_check xen_HYPERVISOR_event_channel_op_compat(int, void *);
Why don't you drop the HYPERVISOR_ now that you added the xen_?
>...
> +EXPORT_SYMBOL_GPL(xen_HYPERVISOR_event_channel_op_compat);
While this export is necessary, ...
>...
> +EXPORT_SYMBOL_GPL(xen_HYPERVISOR_physdev_op_compat);
... I would recommend not adding this one without need.
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] xen/hypercall: fix hypercall fallback code for very old hypervisors
2012-10-30 15:44 ` Konrad Rzeszutek Wilk
(?)
(?)
@ 2012-10-31 8:55 ` Jan Beulich
-1 siblings, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2012-10-31 8:55 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: linux-kernel, xen-devel
>>> On 30.10.12 at 16:44, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Mon, Oct 29, 2012 at 10:08:17AM -0400, Konrad Rzeszutek Wilk wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>>
>> 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().]
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> And it looks like I get
>
> ERROR: "HYPERVISOR_event_channel_op_compat" [drivers/xen/xen-evtchn.ko]
> undefined!
>
> when I build xen-evtchn as module. Jan did you encounter this issue on
> 2.6.18?
No - the event channel driver there can't be built as a module, and
for the forward ported kernels I apparently never tried to build with
a compat setting of 3.0.2 or less (and I didn't care that much either
because the oldest we're actually concerned about is 3.0.4 to cover
some of those very old EC2 systems; I'll add the export at the right
point nevertheless).
Jan
>> ---
>> arch/x86/include/asm/xen/hypercall.h | 21 +++------
>> drivers/xen/Makefile | 2 +-
>> drivers/xen/fallback.c | 78
> ++++++++++++++++++++++++++++++++++
>> 3 files changed, 86 insertions(+), 15 deletions(-)
>> create mode 100644 drivers/xen/fallback.c
>>
>> diff --git a/arch/x86/include/asm/xen/hypercall.h
> b/arch/x86/include/asm/xen/hypercall.h
>> index 59c226d..c812360 100644
>> --- a/arch/x86/include/asm/xen/hypercall.h
>> +++ b/arch/x86/include/asm/xen/hypercall.h
>> @@ -359,18 +359,14 @@ HYPERVISOR_update_va_mapping(unsigned long va, pte_t
> new_val,
>> return _hypercall4(int, update_va_mapping, va,
>> new_val.pte, new_val.pte >> 32, flags);
>> }
>> +int __must_check HYPERVISOR_event_channel_op_compat(int, void *);
>>
>> static inline int
>> HYPERVISOR_event_channel_op(int cmd, void *arg)
>> {
>> int rc = _hypercall2(int, event_channel_op, cmd, arg);
>> - 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);
>> return rc;
>> }
>>
>> @@ -386,17 +382,14 @@ HYPERVISOR_console_io(int cmd, int count, char *str)
>> return _hypercall3(int, console_io, cmd, count, str);
>> }
>>
>> +int __must_check HYPERVISOR_physdev_op_compat(int, void *);
>> +
>> static inline int
>> HYPERVISOR_physdev_op(int cmd, void *arg)
>> {
>> int rc = _hypercall2(int, physdev_op, cmd, arg);
>> - 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);
>> return rc;
>> }
>>
>> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
>> index 0e863703..46de6cd 100644
>> --- a/drivers/xen/Makefile
>> +++ b/drivers/xen/Makefile
>> @@ -2,7 +2,7 @@ ifneq ($(CONFIG_ARM),y)
>> obj-y += manage.o balloon.o
>> obj-$(CONFIG_HOTPLUG_CPU) += cpu_hotplug.o
>> endif
>> -obj-y += grant-table.o features.o events.o
>> +obj-y += grant-table.o features.o events.o fallback.o
>> obj-y += xenbus/
>>
>> nostackp := $(call cc-option, -fno-stack-protector)
>> diff --git a/drivers/xen/fallback.c b/drivers/xen/fallback.c
>> new file mode 100644
>> index 0000000..0bdc468
>> --- /dev/null
>> +++ b/drivers/xen/fallback.c
>> @@ -0,0 +1,78 @@
>> +#include <linux/kernel.h>
>> +#include <linux/string.h>
>> +#include <linux/bug.h>
>> +#include <asm/hypervisor.h>
>> +#include <asm/xen/hypercall.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;
>> +}
>> --
>> 1.7.7.6
^ permalink raw reply [flat|nested] 11+ messages in thread