Re: [PATCH 2/3] kvm: don't register wildcard MMIO EVENTFD on two buses
On 08/25/2015 11:04 AM, Jason Wang wrote:
[...]
@@ -900,10 +899,11 @@ kvm_deassign_ioeventfd(struct kvm *kvm, struct
kvm_ioeventfd *args)
if (!p-wildcard p-datamatch != args-datamatch)
continue;
-kvm_io_bus_unregister_dev(kvm, bus_idx, p-dev);
if (!p-length) {
kvm_io_bus_unregister_dev(kvm,
KVM_FAST_MMIO_BUS,
p-dev);
+} else {
+kvm_io_bus_unregister_dev(kvm, bus_idx,
p-dev);
}
Similar comments here... do you want to check for bus_idx ==
KVM_MMIO_BUS as well?
Good catch. I think keep the original code as is will be also ok to
solve this. (with changing the bus_idx to KVM_FAST_MMIO_BUS during
registering if it was an wildcard mmio).
Do you need to handle the ioeventfd_count changes on the fast mmio bus
as well?
Yes. So actually, it needs some changes: checking the return value of
kvm_io_bus_unregister_dev() and decide which bus does the device belongs to.
Looks like it will be more cleaner by just changing
ioeventfd_bus_from_flags() to return KVM_FAST_MMIO_BUS accordingly. Will
post V2 soon.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] kvm: don't register wildcard MMIO EVENTFD on two buses
On Mon, 24 Aug 2015 11:29:29 +0800
Jason Wang jasow...@redhat.com wrote:
On 08/21/2015 05:29 PM, Cornelia Huck wrote:
On Fri, 21 Aug 2015 16:03:52 +0800
Jason Wang jasow...@redhat.com wrote:
@@ -850,9 +845,15 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct
kvm_ioeventfd *args)
Unfortunately snipped by diff, but the check here is on !len !PIO,
which only does the desired thing as VIRTIO_CCW always uses len == 8.
Should the check be for !len MMIO instead?
I think the answer depends on whether len == 0 is valid for ccw. If not
we can fail the assign earlier. Since even without this patch, if
userspace tries to register a dev with len equals to zero, it will also
be registered to KVM_FAST_MMIO_BUS. If yes, we need check as you
suggested here.
I don't think len != 8 makes much sense for the way ioeventfd is
defined for ccw (we handle hypercalls with a payload specifying the
device), but we currently don't actively fence it.
But regardless, I'd prefer to decide directly upon whether userspace
actually tried to register for the mmio bus.
ret = kvm_io_bus_register_dev(kvm, KVM_FAST_MMIO_BUS,
p-addr, 0, p-dev);
if (ret 0)
- goto register_fail;
+ goto unlock_fail;
+ } else {
+ ret = kvm_io_bus_register_dev(kvm, bus_idx, p-addr, p-length,
+p-dev);
+ if (ret 0)
+ goto unlock_fail;
}
Hm... maybe the following would be more obvious:
my_bus = (p-length == 0) (bus_idx == KVM_MMIO_BUS) ? KVM_FAST_MMIO_BUS
: bus_idx;
ret = kvm_io_bus_register_dev(kvm, my_bus, p-addr, p-length, pdev-dev);
+
kvm-buses[bus_idx]-ioeventfd_count++;
list_add_tail(p-list, kvm-ioeventfds);
(...)
@@ -900,10 +899,11 @@ kvm_deassign_ioeventfd(struct kvm *kvm, struct
kvm_ioeventfd *args)
if (!p-wildcard p-datamatch != args-datamatch)
continue;
- kvm_io_bus_unregister_dev(kvm, bus_idx, p-dev);
if (!p-length) {
kvm_io_bus_unregister_dev(kvm, KVM_FAST_MMIO_BUS,
p-dev);
+ } else {
+ kvm_io_bus_unregister_dev(kvm, bus_idx, p-dev);
}
Similar comments here... do you want to check for bus_idx ==
KVM_MMIO_BUS as well?
Good catch. I think keep the original code as is will be also ok to
solve this. (with changing the bus_idx to KVM_FAST_MMIO_BUS during
registering if it was an wildcard mmio).
Do you need to handle the ioeventfd_count changes on the fast mmio bus
as well?
kvm-buses[bus_idx]-ioeventfd_count--;
ioeventfd_release(p);
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] kvm: don't register wildcard MMIO EVENTFD on two buses
On 08/24/2015 10:05 PM, Cornelia Huck wrote:
On Mon, 24 Aug 2015 11:29:29 +0800
Jason Wang jasow...@redhat.com wrote:
On 08/21/2015 05:29 PM, Cornelia Huck wrote:
On Fri, 21 Aug 2015 16:03:52 +0800
Jason Wang jasow...@redhat.com wrote:
@@ -850,9 +845,15 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct
kvm_ioeventfd *args)
Unfortunately snipped by diff, but the check here is on !len !PIO,
which only does the desired thing as VIRTIO_CCW always uses len == 8.
Should the check be for !len MMIO instead?
I think the answer depends on whether len == 0 is valid for ccw. If not
we can fail the assign earlier. Since even without this patch, if
userspace tries to register a dev with len equals to zero, it will also
be registered to KVM_FAST_MMIO_BUS. If yes, we need check as you
suggested here.
I don't think len != 8 makes much sense for the way ioeventfd is
defined for ccw (we handle hypercalls with a payload specifying the
device), but we currently don't actively fence it.
But regardless, I'd prefer to decide directly upon whether userspace
actually tried to register for the mmio bus.
Ok.
ret = kvm_io_bus_register_dev(kvm, KVM_FAST_MMIO_BUS,
p-addr, 0, p-dev);
if (ret 0)
- goto register_fail;
+ goto unlock_fail;
+ } else {
+ ret = kvm_io_bus_register_dev(kvm, bus_idx, p-addr, p-length,
+p-dev);
+ if (ret 0)
+ goto unlock_fail;
}
Hm... maybe the following would be more obvious:
my_bus = (p-length == 0) (bus_idx == KVM_MMIO_BUS) ? KVM_FAST_MMIO_BUS
: bus_idx;
ret = kvm_io_bus_register_dev(kvm, my_bus, p-addr, p-length, pdev-dev);
+
kvm-buses[bus_idx]-ioeventfd_count++;
list_add_tail(p-list, kvm-ioeventfds);
(...)
@@ -900,10 +899,11 @@ kvm_deassign_ioeventfd(struct kvm *kvm, struct
kvm_ioeventfd *args)
if (!p-wildcard p-datamatch != args-datamatch)
continue;
- kvm_io_bus_unregister_dev(kvm, bus_idx, p-dev);
if (!p-length) {
kvm_io_bus_unregister_dev(kvm, KVM_FAST_MMIO_BUS,
p-dev);
+ } else {
+ kvm_io_bus_unregister_dev(kvm, bus_idx, p-dev);
}
Similar comments here... do you want to check for bus_idx ==
KVM_MMIO_BUS as well?
Good catch. I think keep the original code as is will be also ok to
solve this. (with changing the bus_idx to KVM_FAST_MMIO_BUS during
registering if it was an wildcard mmio).
Do you need to handle the ioeventfd_count changes on the fast mmio bus
as well?
Yes. So actually, it needs some changes: checking the return value of
kvm_io_bus_unregister_dev() and decide which bus does the device belongs to.
kvm-buses[bus_idx]-ioeventfd_count--;
ioeventfd_release(p);
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] kvm: don't register wildcard MMIO EVENTFD on two buses
On 08/21/2015 05:29 PM, Cornelia Huck wrote:
On Fri, 21 Aug 2015 16:03:52 +0800
Jason Wang jasow...@redhat.com wrote:
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 9ff4193..834a409 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -838,11 +838,6 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct
kvm_ioeventfd *args)
kvm_iodevice_init(p-dev, ioeventfd_ops);
-ret = kvm_io_bus_register_dev(kvm, bus_idx, p-addr, p-length,
- p-dev);
-if (ret 0)
-goto unlock_fail;
-
/* When length is ignored, MMIO is also put on a separate bus, for
* faster lookups.
You probably want to change this comment as well?
Yes.
*/
@@ -850,9 +845,15 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct
kvm_ioeventfd *args)
Unfortunately snipped by diff, but the check here is on !len !PIO,
which only does the desired thing as VIRTIO_CCW always uses len == 8.
Should the check be for !len MMIO instead?
I think the answer depends on whether len == 0 is valid for ccw. If not
we can fail the assign earlier. Since even without this patch, if
userspace tries to register a dev with len equals to zero, it will also
be registered to KVM_FAST_MMIO_BUS. If yes, we need check as you
suggested here.
ret = kvm_io_bus_register_dev(kvm, KVM_FAST_MMIO_BUS,
p-addr, 0, p-dev);
if (ret 0)
-goto register_fail;
+goto unlock_fail;
+} else {
+ret = kvm_io_bus_register_dev(kvm, bus_idx, p-addr, p-length,
+ p-dev);
+if (ret 0)
+goto unlock_fail;
}
Hm... maybe the following would be more obvious:
my_bus = (p-length == 0) (bus_idx == KVM_MMIO_BUS) ? KVM_FAST_MMIO_BUS :
bus_idx;
ret = kvm_io_bus_register_dev(kvm, my_bus, p-addr, p-length, pdev-dev);
+
kvm-buses[bus_idx]-ioeventfd_count++;
list_add_tail(p-list, kvm-ioeventfds);
(...)
@@ -900,10 +899,11 @@ kvm_deassign_ioeventfd(struct kvm *kvm, struct
kvm_ioeventfd *args)
if (!p-wildcard p-datamatch != args-datamatch)
continue;
-kvm_io_bus_unregister_dev(kvm, bus_idx, p-dev);
if (!p-length) {
kvm_io_bus_unregister_dev(kvm, KVM_FAST_MMIO_BUS,
p-dev);
+} else {
+kvm_io_bus_unregister_dev(kvm, bus_idx, p-dev);
}
Similar comments here... do you want to check for bus_idx ==
KVM_MMIO_BUS as well?
Good catch. I think keep the original code as is will be also ok to
solve this. (with changing the bus_idx to KVM_FAST_MMIO_BUS during
registering if it was an wildcard mmio).
kvm-buses[bus_idx]-ioeventfd_count--;
ioeventfd_release(p);
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] kvm: don't register wildcard MMIO EVENTFD on two buses
We register wildcard mmio eventfd on two buses, one for KVM_MMIO_BUS
and another is KVM_FAST_MMIO_BUS. This leads to issue:
- kvm_io_bus_destroy() knows nothing about the devices on two buses
points to a single dev. Which will lead double free [1] during exit.
- wildcard eventfd ignores data len, so it was registered as a
kvm_io_range with zero length. This will fail the binary search in
kvm_io_bus_get_first_dev() when we try to emulate through
KVM_MMIO_BUS. This will cause userspace io emulation request instead
of a eventfd notification (virtqueue kick will be trapped by qemu
instead of vhost in this case).
Fixing this by don't register wildcard mmio eventfd on two
buses. Instead, only register it in KVM_FAST_MMIO_BUS. This fixes the
double free issue of kvm_io_bus_destroy(). For the arch/setups that
does not utilize KVM_FAST_MMIO_BUS, before search KVM_MMIO_BUS, try
KVM_FAST_MMIO_BUS first to see it it has a match.
[1] Panic caused by double free:
CPU: 1 PID: 2894 Comm: qemu-system-x86 Not tainted 3.19.0-26-generic #28-Ubuntu
Hardware name: LENOVO 2356BG6/2356BG6, BIOS G7ET96WW (2.56 ) 09/12/2013
task: 88009ae0c4b0 ti: 88020e7f task.ti: 88020e7f
RIP: 0010:[c07e25d8] [c07e25d8]
ioeventfd_release+0x28/0x60 [kvm]
RSP: 0018:88020e7f3bc8 EFLAGS: 00010292
RAX: dead00200200 RBX: 8801ec19c900 RCX: 00018200016d
RDX: 8801ec19cf80 RSI: ea0008bf1d40 RDI: 8801ec19c900
RBP: 88020e7f3bd8 R08: 2fc75a01 R09: 00018200016d
R10: c07df6ae R11: 88022fc75a98 R12: 88021e7cc000
R13: 88021e7cca48 R14: 88021e7cca50 R15: 8801ec19c880
FS: 7fc1ee3e6700() GS:88023e24() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 7f8f389d8000 CR3: 00023dc13000 CR4: 001427e0
Stack:
88021e7cc000 88020e7f3be8 c07e2622
88020e7f3c38 c07df69a 880232524160 88020e792d80
880219b78c00 0008 8802321686a8
Call Trace:
[c07e2622] ioeventfd_destructor+0x12/0x20 [kvm]
[c07df69a] kvm_put_kvm+0xca/0x210 [kvm]
[c07df818] kvm_vcpu_release+0x18/0x20 [kvm]
[811f69f7] __fput+0xe7/0x250
[811f6bae] fput+0xe/0x10
[81093f04] task_work_run+0xd4/0xf0
[81079358] do_exit+0x368/0xa50
[81082c8f] ? recalc_sigpending+0x1f/0x60
[81079ad5] do_group_exit+0x45/0xb0
[81085c71] get_signal+0x291/0x750
[810144d8] do_signal+0x28/0xab0
[810f3a3b] ? do_futex+0xdb/0x5d0
[810b7028] ? __wake_up_locked_key+0x18/0x20
[810f3fa6] ? SyS_futex+0x76/0x170
[81014fc9] do_notify_resume+0x69/0xb0
[817cb9af] int_signal+0x12/0x17
Code: 5d c3 90 0f 1f 44 00 00 55 48 89 e5 53 48 89 fb 48 83 ec 08 48 8b 7f 20
e8 06 d6 a5 c0 48 8b 43 08 48 8b 13 48 89 df 48 89 42 08 48 89 10 48 b8 00 01
10 00 00
RIP [c07e25d8] ioeventfd_release+0x28/0x60 [kvm]
RSP 88020e7f3bc8
Cc: Gleb Natapov g...@kernel.org
Cc: Paolo Bonzini pbonz...@redhat.com
Cc: Michael S. Tsirkin m...@redhat.com
Signed-off-by: Jason Wang jasow...@redhat.com
---
virt/kvm/eventfd.c | 18 +-
virt/kvm/kvm_main.c | 16 ++--
2 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 9ff4193..834a409 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -838,11 +838,6 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd
*args)
kvm_iodevice_init(p-dev, ioeventfd_ops);
- ret = kvm_io_bus_register_dev(kvm, bus_idx, p-addr, p-length,
- p-dev);
- if (ret 0)
- goto unlock_fail;
-
/* When length is ignored, MMIO is also put on a separate bus, for
* faster lookups.
*/
@@ -850,9 +845,15 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd
*args)
ret = kvm_io_bus_register_dev(kvm, KVM_FAST_MMIO_BUS,
p-addr, 0, p-dev);
if (ret 0)
- goto register_fail;
+ goto unlock_fail;
+ } else {
+ ret = kvm_io_bus_register_dev(kvm, bus_idx, p-addr, p-length,
+ p-dev);
+ if (ret 0)
+ goto unlock_fail;
}
+
kvm-buses[bus_idx]-ioeventfd_count++;
list_add_tail(p-list, kvm-ioeventfds);
@@ -860,8 +861,6 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd
*args)
return 0;
-register_fail:
- kvm_io_bus_unregister_dev(kvm, bus_idx, p-dev);
unlock_fail:
mutex_unlock(kvm-slots_lock);
@@ -900,10 +899,11 @@ kvm_deassign_ioeventfd(struct kvm *kvm, struct
kvm_ioeventfd *args)
if (!p-wildcard p-datamatch != args-datamatch)
continue;
-
Re: [PATCH 2/3] kvm: don't register wildcard MMIO EVENTFD on two buses
On Fri, 21 Aug 2015 16:03:52 +0800
Jason Wang jasow...@redhat.com wrote:
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 9ff4193..834a409 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -838,11 +838,6 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct
kvm_ioeventfd *args)
kvm_iodevice_init(p-dev, ioeventfd_ops);
- ret = kvm_io_bus_register_dev(kvm, bus_idx, p-addr, p-length,
- p-dev);
- if (ret 0)
- goto unlock_fail;
-
/* When length is ignored, MMIO is also put on a separate bus, for
* faster lookups.
You probably want to change this comment as well?
*/
@@ -850,9 +845,15 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct
kvm_ioeventfd *args)
Unfortunately snipped by diff, but the check here is on !len !PIO,
which only does the desired thing as VIRTIO_CCW always uses len == 8.
Should the check be for !len MMIO instead?
ret = kvm_io_bus_register_dev(kvm, KVM_FAST_MMIO_BUS,
p-addr, 0, p-dev);
if (ret 0)
- goto register_fail;
+ goto unlock_fail;
+ } else {
+ ret = kvm_io_bus_register_dev(kvm, bus_idx, p-addr, p-length,
+ p-dev);
+ if (ret 0)
+ goto unlock_fail;
}
Hm... maybe the following would be more obvious:
my_bus = (p-length == 0) (bus_idx == KVM_MMIO_BUS) ? KVM_FAST_MMIO_BUS :
bus_idx;
ret = kvm_io_bus_register_dev(kvm, my_bus, p-addr, p-length, pdev-dev);
+
kvm-buses[bus_idx]-ioeventfd_count++;
list_add_tail(p-list, kvm-ioeventfds);
(...)
@@ -900,10 +899,11 @@ kvm_deassign_ioeventfd(struct kvm *kvm, struct
kvm_ioeventfd *args)
if (!p-wildcard p-datamatch != args-datamatch)
continue;
- kvm_io_bus_unregister_dev(kvm, bus_idx, p-dev);
if (!p-length) {
kvm_io_bus_unregister_dev(kvm, KVM_FAST_MMIO_BUS,
p-dev);
+ } else {
+ kvm_io_bus_unregister_dev(kvm, bus_idx, p-dev);
}
Similar comments here... do you want to check for bus_idx ==
KVM_MMIO_BUS as well?
kvm-buses[bus_idx]-ioeventfd_count--;
ioeventfd_release(p);
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
