Re: [kvm-devel] [PATCH] virtio-balloon: do not attempt to release more than available pages
On Thu, Mar 06, 2008 at 09:39:48AM +1100, Rusty Russell wrote:
On Thursday 06 March 2008 03:28:32 Marcelo Tosatti wrote:
Handle the case where the balloon target is larger than total ram size.
BUG: unable to handle kernel paging request at 00100100
IP: [881970f9] :virtio_balloon:leak__balloon+0x2e/0xbe
Signed-off-by: Marcelo Tosatti [EMAIL PROTECTED]
Hi Marcelo,
My deepest concern is that we're papering over some real bug, if you
tripped this. Zeroing num_pages makes sense as a hack, but we're clearly in
trouble at this point and there should be some indication of that.
If I may ask, how did this happen?
Rusty,
QEMU allows the user to set a target larger than the memory size which
the guest was booted with:
dev-num_pages = (ram_size - target) TARGET_PAGE_BITS;
virtio_notify_config(dev-vdev);
num_pages goes negative, so driver attempts deflate back -num_pages from
its balloon.
There is no other bug here, its just a matter of guest driver being
fragile. You can avoid that condition in the host backend:
--- virtio-balloon.c.orig 2008-03-08 14:39:51.0 -0300
+++ virtio-balloon.c2008-03-08 14:41:08.0 -0300
@@ -113,6 +113,8 @@
VirtIOBalloon *dev = opaque;
if (target) {
+ if (target ram_size)
+ target = ram_size;
dev-num_pages = (ram_size - target) TARGET_PAGE_BITS;
virtio_notify_config(dev-vdev);
}
But making the driver robust against it seems sensate. I agree that
zeroing num_pages is hackish. What do you suggest?
Thanks
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH] QEMU support for virtio balloon driver
On Sat, Mar 08, 2008 at 04:27:13PM -0300, Marcelo Tosatti wrote: On Sat, Jan 26, 2008 at 08:47:28PM +0200, Avi Kivity wrote: Anthony Liguori wrote: Do others expect KVM to just cope with the virtual mapping being changed out from underneath of it? kvm should cope with both malicious guests and malicious (or buggy) host userspace. It's difficuly to analyze, but mmu notifiers might be necessary for the latter. The reason for the host crash with madvise is that the rmap code relies on the guest process virtual mappings from not disappearing while there are active shadow mappings. How to proceed now? Do we want to efficiently support ballooning without mmu notifiers? If so, an ioctl to zap the mmu is necessary as discussed before. Err, actually zapping the mmu is necessary to guarantee guest will not use stale shadow entries vs entries pointing to old unreacheable pages. - This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH] Mark kobjects as unitialized
On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
snip
Where exactly in the code does that happen? kobjects should not be
reused as that implies that they are static, and not dynamically
allocated, right?
Which kobject is this?
Yes, its static. Here's the code from virt/kvm_main.c:1269
static struct sys_device kvm_sysdev = {
.id = 0,
.cls = kvm_sysdev_class,
};
this sys_device is being registered/unregistered when kvm-intel is
loaded/unloaded.
Ah, ok. I'll add this patch then.
Ugh, is this the sys_device stuff? I hate that code...
Yes it is! But, why do you hate it ?
For reasons like this :)
kobjects should not be static. the sysdevice stuff was a hack when it
was originally created and never touched since the mid 2.5 days. It
needs to be fixed up a lot, and is on my TODO list, slowly getting
closer to the top...
Hi,
This patch does not fix it all! The problem is in fact more involved. I also
get these BUG reports when I reload kvm-intel.
BUG kmalloc-8: Object already free
[ 74.696570]
-
[ 74.696596]
[ 74.697310] INFO: Allocated in strndup_user+0x30/0x62 age=587 cpu=2 pid=1439
[ 74.697845] INFO: Freed in kobject_set_name_vargs+0x29/0x32 age=559 cpu=3
pid=1439
[ 74.698008] INFO: Slab 0xc16f93a0 used=10 fp=0xf7c9d2d8 flags=0x1c3
[ 74.698008] INFO: Object 0xf7c9d1f8 @offset=504 fp=0xf7c9d578
This happens because, sysdev_class_register assigns a name to the kobject, and
kfrees the old name if any. The
poisoned 'name' object persists in case of statically allocated kobjects and as
its passed to kfree again when re
registered, we get the above warning.
So, AFAICS the best way to solve this is by fixing the kobject users (kvm,
oprofile etc.) to use dynamic kobjects instead
of static ones or memset the kobject to zero before passing it to
sysdev_register.
Greg ?
--
regards,
balaji rao
3rd year,
Dept. of Mechanical Engineering,
National Institute of Technology, Karnataka
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
spin_lock(kvm_lock);
+if (--kvm-refcount) {
+spin_lock(kvm_lock);
obvious typo here...
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH] Mark kobjects as unitialized
On Sunday 09 March 2008 10:36:49 am Greg KH wrote:
On Sun, Mar 09, 2008 at 03:37:16AM +0530, Balaji Rao wrote:
On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
snip
Where exactly in the code does that happen? kobjects should not be
reused as that implies that they are static, and not dynamically
allocated, right?
Which kobject is this?
Yes, its static. Here's the code from virt/kvm_main.c:1269
static struct sys_device kvm_sysdev = {
.id = 0,
.cls = kvm_sysdev_class,
};
this sys_device is being registered/unregistered when kvm-intel is
loaded/unloaded.
Ah, ok. I'll add this patch then.
Ugh, is this the sys_device stuff? I hate that code...
Yes it is! But, why do you hate it ?
For reasons like this :)
kobjects should not be static. the sysdevice stuff was a hack when it
was originally created and never touched since the mid 2.5 days. It
needs to be fixed up a lot, and is on my TODO list, slowly getting
closer to the top...
Hi,
This patch does not fix it all! The problem is in fact more involved.
I also get these BUG reports when I reload kvm-intel.
BUG kmalloc-8: Object already free
[ 74.696570]
-
[ 74.696596]
[ 74.697310] INFO: Allocated in strndup_user+0x30/0x62 age=587 cpu=2
pid=1439
[ 74.697845] INFO: Freed in kobject_set_name_vargs+0x29/0x32 age=559
cpu=3 pid=1439
[ 74.698008] INFO: Slab 0xc16f93a0 used=10 fp=0xf7c9d2d8 flags=0x1c3
[ 74.698008] INFO: Object 0xf7c9d1f8 @offset=504 fp=0xf7c9d578
This happens because, sysdev_class_register assigns a name to the
kobject, and kfrees the old name if any. The poisoned 'name' object
persists in case of statically allocated kobjects and as its passed to
kfree again when re registered, we get the above warning.
Ugh, that's horrible. And people wonder why I hate the sysdev code :)
Does it also mean that when we do cleanup sysdev devices, we are freeing
a name that might not have been dynamically allocated? If so, we need
to fix that as well.
No, that doesn't happen. We free the name only if it is allocated..
A simple 'strdup' of the class name in sysdev_class_register() should
fix all of this, right?
So, AFAICS the best way to solve this is by fixing the kobject users
(kvm, oprofile etc.) to use dynamic kobjects instead of static ones or
memset the kobject to zero before passing it to sysdev_register.
Yes, that would be great to fix up, but probably unlikly so late in the
release cycle.
How about the patch below, does it work for you? (build tested only)
thanks,
greg k-h
---
drivers/base/sys.c |5 -
1 file changed, 4 insertions(+), 1 deletion(-)
--- a/drivers/base/sys.c
+++ b/drivers/base/sys.c
@@ -130,13 +130,16 @@ static struct kset *system_kset;
int sysdev_class_register(struct sysdev_class * cls)
{
+ char *name;
+
pr_debug(Registering sysdev class '%s'\n,
kobject_name(cls-kset.kobj));
INIT_LIST_HEAD(cls-drivers);
cls-kset.kobj.parent = system_kset-kobj;
cls-kset.kobj.ktype = ktype_sysdev_class;
cls-kset.kobj.kset = system_kset;
- kobject_set_name(cls-kset.kobj, cls-name);
+ name = kstrdup(cls-name, GFP_KERNEL);
+ kobject_set_name(cls-kset.kobj, name);
return kset_register(cls-kset);
}
--
regards,
balaji rao
3rd year,
Dept. of Mechanical Engineering,
National Institute of Technology, Karnataka
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH] Mark kobjects as unitialized
On Sunday 09 March 2008 12:03:08 pm Greg KH wrote:
On Sun, Mar 09, 2008 at 03:37:16AM +0530, Balaji Rao wrote:
On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
snip
Where exactly in the code does that happen? kobjects should not be
reused as that implies that they are static, and not dynamically
allocated, right?
Which kobject is this?
Yes, its static. Here's the code from virt/kvm_main.c:1269
static struct sys_device kvm_sysdev = {
.id = 0,
.cls = kvm_sysdev_class,
};
this sys_device is being registered/unregistered when kvm-intel is
loaded/unloaded.
Ah, ok. I'll add this patch then.
Ugh, is this the sys_device stuff? I hate that code...
Yes it is! But, why do you hate it ?
For reasons like this :)
kobjects should not be static. the sysdevice stuff was a hack when it
was originally created and never touched since the mid 2.5 days. It
needs to be fixed up a lot, and is on my TODO list, slowly getting
closer to the top...
Hi,
This patch does not fix it all! The problem is in fact more involved. I
also get these BUG reports when I reload
kvm-intel.
BUG kmalloc-8: Object already free
[ 74.696570]
-
[ 74.696596]
[ 74.697310] INFO: Allocated in strndup_user+0x30/0x62 age=587 cpu=2
pid=1439
[ 74.697845] INFO: Freed in kobject_set_name_vargs+0x29/0x32 age=559
cpu=3 pid=1439
[ 74.698008] INFO: Slab 0xc16f93a0 used=10 fp=0xf7c9d2d8 flags=0x1c3
[ 74.698008] INFO: Object 0xf7c9d1f8 @offset=504 fp=0xf7c9d578
This happens because, sysdev_class_register assigns a name to the
kobject, and kfrees the old name if any. The poisoned 'name' object
persists in case of statically allocated kobjects and as its passed to
kfree again when re registered, we get the above warning.
So, AFAICS the best way to solve this is by fixing the kobject users
(kvm, oprofile etc.) to use dynamic kobjects instead of static ones or
memset the kobject to zero before passing it to sysdev_register.
I like the memset idea, how about this patch instead?
thanks,
greg k-h
--- a/drivers/base/sys.c
+++ b/drivers/base/sys.c
@@ -133,6 +133,7 @@ int sysdev_class_register(struct sysdev_
pr_debug(Registering sysdev class '%s'\n,
kobject_name(cls-kset.kobj));
INIT_LIST_HEAD(cls-drivers);
+ memset(cls-kset.kobj, 0x00, sizeof(struct kobject));
cls-kset.kobj.parent = system_kset-kobj;
cls-kset.kobj.ktype = ktype_sysdev_class;
cls-kset.kobj.kset = system_kset;
This should work.But I am afraid if we zeroed it, the kfree poison test
wouldn't catch even the genuine cases. Isn't it ?
CMIIW.
A better fix according to me would be to zero the kobject in the places where
we know it is being re-registered (kvm,
oprofile) etc. This should do for now. But we should fix the sys_device users
later, for the next cycle.
--
regards,
balaji rao
3rd year,
Dept. of Mechanical Engineering,
National Institute of Technology, Karnataka
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH] Mark kobjects as unitialized
On Sun, Mar 09, 2008 at 12:06:02PM +0530, Balaji Rao wrote:
On Sunday 09 March 2008 12:03:08 pm Greg KH wrote:
On Sun, Mar 09, 2008 at 03:37:16AM +0530, Balaji Rao wrote:
On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
snip
Where exactly in the code does that happen? kobjects should not be
reused as that implies that they are static, and not dynamically
allocated, right?
Which kobject is this?
Yes, its static. Here's the code from virt/kvm_main.c:1269
static struct sys_device kvm_sysdev = {
.id = 0,
.cls = kvm_sysdev_class,
};
this sys_device is being registered/unregistered when kvm-intel is
loaded/unloaded.
Ah, ok. I'll add this patch then.
Ugh, is this the sys_device stuff? I hate that code...
Yes it is! But, why do you hate it ?
For reasons like this :)
kobjects should not be static. the sysdevice stuff was a hack when it
was originally created and never touched since the mid 2.5 days. It
needs to be fixed up a lot, and is on my TODO list, slowly getting
closer to the top...
Hi,
This patch does not fix it all! The problem is in fact more involved. I
also get these BUG reports when I reload
kvm-intel.
BUG kmalloc-8: Object already free
[ 74.696570]
-
[ 74.696596]
[ 74.697310] INFO: Allocated in strndup_user+0x30/0x62 age=587 cpu=2
pid=1439
[ 74.697845] INFO: Freed in kobject_set_name_vargs+0x29/0x32 age=559
cpu=3 pid=1439
[ 74.698008] INFO: Slab 0xc16f93a0 used=10 fp=0xf7c9d2d8 flags=0x1c3
[ 74.698008] INFO: Object 0xf7c9d1f8 @offset=504 fp=0xf7c9d578
This happens because, sysdev_class_register assigns a name to the
kobject, and kfrees the old name if any. The poisoned 'name' object
persists in case of statically allocated kobjects and as its passed to
kfree again when re registered, we get the above warning.
So, AFAICS the best way to solve this is by fixing the kobject users
(kvm, oprofile etc.) to use dynamic kobjects instead of static ones or
memset the kobject to zero before passing it to sysdev_register.
I like the memset idea, how about this patch instead?
thanks,
greg k-h
--- a/drivers/base/sys.c
+++ b/drivers/base/sys.c
@@ -133,6 +133,7 @@ int sysdev_class_register(struct sysdev_
pr_debug(Registering sysdev class '%s'\n,
kobject_name(cls-kset.kobj));
INIT_LIST_HEAD(cls-drivers);
+ memset(cls-kset.kobj, 0x00, sizeof(struct kobject));
cls-kset.kobj.parent = system_kset-kobj;
cls-kset.kobj.ktype = ktype_sysdev_class;
cls-kset.kobj.kset = system_kset;
This should work.But I am afraid if we zeroed it, the kfree poison
test wouldn't catch even the genuine cases. Isn't it ? CMIIW.
What genuine cases? kobjects should always be initialized to zero
before they are registered.
A better fix according to me would be to zero the kobject in the
places where we know it is being re-registered (kvm, oprofile) etc.
This should do for now. But we should fix the sys_device users later,
for the next cycle.
Are you sure you know all of the sysdev_class objects that can be
re-registered?
Can you test this patch out?
thanks,
greg k-h
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH] Mark kobjects as unitialized
On Sunday 09 March 2008 12:33:07 pm Greg KH wrote: snip Hi, This patch does not fix it all! The problem is in fact more involved. I also get these BUG reports when I reload kvm-intel. BUG kmalloc-8: Object already free [ 74.696570] - [ 74.696596] [ 74.697310] INFO: Allocated in strndup_user+0x30/0x62 age=587 cpu=2 pid=1439 [ 74.697845] INFO: Freed in kobject_set_name_vargs+0x29/0x32 age=559 cpu=3 pid=1439 [ 74.698008] INFO: Slab 0xc16f93a0 used=10 fp=0xf7c9d2d8 flags=0x1c3 [ 74.698008] INFO: Object 0xf7c9d1f8 @offset=504 fp=0xf7c9d578 This happens because, sysdev_class_register assigns a name to the kobject, and kfrees the old name if any. The poisoned 'name' object persists in case of statically allocated kobjects and as its passed to kfree again when re registered, we get the above warning. So, AFAICS the best way to solve this is by fixing the kobject users (kvm, oprofile etc.) to use dynamic kobjects instead of static ones or memset the kobject to zero before passing it to sysdev_register. I like the memset idea, how about this patch instead? thanks, greg k-h --- a/drivers/base/sys.c +++ b/drivers/base/sys.c @@ -133,6 +133,7 @@ int sysdev_class_register(struct sysdev_ pr_debug(Registering sysdev class '%s'\n, kobject_name(cls-kset.kobj)); INIT_LIST_HEAD(cls-drivers); + memset(cls-kset.kobj, 0x00, sizeof(struct kobject)); cls-kset.kobj.parent = system_kset-kobj; cls-kset.kobj.ktype = ktype_sysdev_class; cls-kset.kobj.kset = system_kset; This should work.But I am afraid if we zeroed it, the kfree poison test wouldn't catch even the genuine cases. Isn't it ? CMIIW. What genuine cases? kobjects should always be initialized to zero before they are registered. The cases in which a freed kobject is passed to the register function ? Probably they will be caught when do a memset.. A better fix according to me would be to zero the kobject in the places where we know it is being re-registered (kvm, oprofile) etc. This should do for now. But we should fix the sys_device users later, for the next cycle. Are you sure you know all of the sysdev_class objects that can be re-registered? Hmm.. Right. No, I only know that KVM and Oprofile use it. Can you test this patch out? Yes the idea works. One more memset is needed in sysdev_register. Here's the final patch. diff --git a/drivers/base/sys.c b/drivers/base/sys.c index 2f79c55..7c839d9 100644 --- a/drivers/base/sys.c +++ b/drivers/base/sys.c @@ -133,6 +133,7 @@ int sysdev_class_register(struct sysdev_class * cls) pr_debug(Registering sysdev class '%s'\n, kobject_name(cls-kset.kobj)); INIT_LIST_HEAD(cls-drivers); + memset(cls-kset.kobj, 0x00, sizeof(struct kobject)); cls-kset.kobj.parent = system_kset-kobj; cls-kset.kobj.ktype = ktype_sysdev_class; cls-kset.kobj.kset = system_kset; @@ -227,6 +228,7 @@ int sysdev_register(struct sys_device * sysdev) pr_debug(Registering sys device '%s'\n, kobject_name(sysdev-kobj)); + memset(sysdev-kobj, 0x00, sizeof(struct kobject)); /* Make sure the kset is set */ sysdev-kobj.kset = cls-kset; -- regards, balaji rao 3rd year, Dept. of Mechanical Engineering, National Institute of Technology, Karnataka - This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
