[Qemu-devel] [PATCH 5/7] balloon: Separate out stat and balloon handling

[Qemu-devel] [PATCH 5/7] balloon: Separate out stat and balloon handling

[Amit Shah]

Passing on '0' as ballooning target to indicate retrieval of stats is
bad API.  It also makes 'balloon 0' in the monitor cause a segfault.
Have two different functions handle the different functionality instead.

Reported-by: Mike Cao <bcao@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 balloon.c           |   17 ++++++++++-------
 balloon.h           |    8 +++++---
 hw/virtio-balloon.c |    7 ++-----
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/balloon.c b/balloon.c
index d40be39..8be3812 100644
--- a/balloon.c
+++ b/balloon.c
@@ -32,30 +32,33 @@
 
 
 static QEMUBalloonEvent *balloon_event_fn;
+static QEMUBalloonStatus *balloon_stat_fn;
 static void *balloon_opaque;
 
-void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque)
+void qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
+                              QEMUBalloonStatus *stat_func, void *opaque)
 {
-    balloon_event_fn = func;
+    balloon_event_fn = event_func;
+    balloon_stat_fn = stat_func;
     balloon_opaque = opaque;
 }
 
-static int qemu_balloon(ram_addr_t target, MonitorCompletion cb, void *opaque)
+static int qemu_balloon(ram_addr_t target)
 {
     if (!balloon_event_fn) {
         return 0;
     }
     trace_balloon_event(balloon_opaque, target);
-    balloon_event_fn(balloon_opaque, target, cb, opaque);
+    balloon_event_fn(balloon_opaque, target);
     return 1;
 }
 
 static int qemu_balloon_status(MonitorCompletion cb, void *opaque)
 {
-    if (!balloon_event_fn) {
+    if (!balloon_stat_fn) {
         return 0;
     }
-    balloon_event_fn(balloon_opaque, 0, cb, opaque);
+    balloon_stat_fn(balloon_opaque, cb, opaque);
     return 1;
 }
 
@@ -135,7 +138,7 @@ int do_balloon(Monitor *mon, const QDict *params,
         return -1;
     }
 
-    ret = qemu_balloon(qdict_get_int(params, "value"), cb, opaque);
+    ret = qemu_balloon(qdict_get_int(params, "value"));
     if (ret == 0) {
         qerror_report(QERR_DEVICE_NOT_ACTIVE, "balloon");
         return -1;
diff --git a/balloon.h b/balloon.h
index 06a8a46..a6c31d5 100644
--- a/balloon.h
+++ b/balloon.h
@@ -16,10 +16,12 @@
 
 #include "monitor.h"
 
-typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target,
-                                MonitorCompletion cb, void *cb_data);
+typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
+typedef void (QEMUBalloonStatus)(void *opaque, MonitorCompletion cb,
+                                 void *cb_data);
 
-void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque);
+void qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
+                              QEMUBalloonStatus *stat_func, void *opaque);
 
 void monitor_print_balloon(Monitor *mon, const QObject *data);
 int do_info_balloon(Monitor *mon, MonitorCompletion cb, void *opaque);
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 2f371f2..40b43b0 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -227,8 +227,7 @@ static void virtio_balloon_stat(void *opaque, MonitorCompletion cb,
     complete_stats_request(dev);
 }
 
-static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
-                                     MonitorCompletion cb, void *cb_data)
+static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
 {
     VirtIOBalloon *dev = opaque;
 
@@ -238,8 +237,6 @@ static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
     if (target) {
         dev->num_pages = (ram_size - target) >> VIRTIO_BALLOON_PFN_SHIFT;
         virtio_notify_config(&dev->vdev);
-    } else {
-        virtio_balloon_stat(opaque, cb, cb_data);
     }
 }
 
@@ -284,7 +281,7 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
     s->svq = virtio_add_queue(&s->vdev, 128, virtio_balloon_receive_stats);
 
     reset_stats(s);
-    qemu_add_balloon_handler(virtio_balloon_to_target, s);
+    qemu_add_balloon_handler(virtio_balloon_to_target, virtio_balloon_stat, s);
 
     register_savevm(dev, "virtio-balloon", -1, 1,
                     virtio_balloon_save, virtio_balloon_load, s);
-- 
1.7.6

Re: [Qemu-devel] [PATCH 5/7] balloon: Separate out stat and balloon handling

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> Passing on '0' as ballooning target to indicate retrieval of stats is
> bad API.  It also makes 'balloon 0' in the monitor cause a segfault.
> Have two different functions handle the different functionality instead.
>
> Reported-by: Mike Cao <bcao@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

Can you explain the fault?  It's not obvious to me...

> ---
>  balloon.c           |   17 ++++++++++-------
>  balloon.h           |    8 +++++---
>  hw/virtio-balloon.c |    7 ++-----
>  3 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/balloon.c b/balloon.c
> index d40be39..8be3812 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -32,30 +32,33 @@
>  
>  
>  static QEMUBalloonEvent *balloon_event_fn;
> +static QEMUBalloonStatus *balloon_stat_fn;
>  static void *balloon_opaque;
>  
> -void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque)
> +void qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
> +                              QEMUBalloonStatus *stat_func, void *opaque)
>  {
> -    balloon_event_fn = func;
> +    balloon_event_fn = event_func;
> +    balloon_stat_fn = stat_func;
>      balloon_opaque = opaque;
>  }
>  
> -static int qemu_balloon(ram_addr_t target, MonitorCompletion cb, void *opaque)
> +static int qemu_balloon(ram_addr_t target)
>  {
>      if (!balloon_event_fn) {
>          return 0;
>      }
>      trace_balloon_event(balloon_opaque, target);
> -    balloon_event_fn(balloon_opaque, target, cb, opaque);
> +    balloon_event_fn(balloon_opaque, target);
>      return 1;
>  }
>  
>  static int qemu_balloon_status(MonitorCompletion cb, void *opaque)
>  {
> -    if (!balloon_event_fn) {
> +    if (!balloon_stat_fn) {
>          return 0;
>      }
> -    balloon_event_fn(balloon_opaque, 0, cb, opaque);
> +    balloon_stat_fn(balloon_opaque, cb, opaque);
>      return 1;
>  }
>  
> @@ -135,7 +138,7 @@ int do_balloon(Monitor *mon, const QDict *params,
>          return -1;
>      }
>  
> -    ret = qemu_balloon(qdict_get_int(params, "value"), cb, opaque);
> +    ret = qemu_balloon(qdict_get_int(params, "value"));
>      if (ret == 0) {
>          qerror_report(QERR_DEVICE_NOT_ACTIVE, "balloon");
>          return -1;
> diff --git a/balloon.h b/balloon.h
> index 06a8a46..a6c31d5 100644
> --- a/balloon.h
> +++ b/balloon.h
> @@ -16,10 +16,12 @@
>  
>  #include "monitor.h"
>  
> -typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target,
> -                                MonitorCompletion cb, void *cb_data);
> +typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
> +typedef void (QEMUBalloonStatus)(void *opaque, MonitorCompletion cb,
> +                                 void *cb_data);
>  
> -void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque);
> +void qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
> +                              QEMUBalloonStatus *stat_func, void *opaque);
>  
>  void monitor_print_balloon(Monitor *mon, const QObject *data);
>  int do_info_balloon(Monitor *mon, MonitorCompletion cb, void *opaque);
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 2f371f2..40b43b0 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -227,8 +227,7 @@ static void virtio_balloon_stat(void *opaque, MonitorCompletion cb,
>      complete_stats_request(dev);
>  }
>  
> -static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
> -                                     MonitorCompletion cb, void *cb_data)
> +static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
>  {
>      VirtIOBalloon *dev = opaque;
>  
> @@ -238,8 +237,6 @@ static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
>      if (target) {
>          dev->num_pages = (ram_size - target) >> VIRTIO_BALLOON_PFN_SHIFT;
>          virtio_notify_config(&dev->vdev);
> -    } else {
> -        virtio_balloon_stat(opaque, cb, cb_data);
>      }
>  }
>  

Special case: do nothing when target == 0.  Is that necessary/

> @@ -284,7 +281,7 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
>      s->svq = virtio_add_queue(&s->vdev, 128, virtio_balloon_receive_stats);
>  
>      reset_stats(s);
> -    qemu_add_balloon_handler(virtio_balloon_to_target, s);
> +    qemu_add_balloon_handler(virtio_balloon_to_target, virtio_balloon_stat, s);
>  
>      register_savevm(dev, "virtio-balloon", -1, 1,
>                      virtio_balloon_save, virtio_balloon_load, s);

Re: [Qemu-devel] [PATCH 5/7] balloon: Separate out stat and balloon handling

[Amit Shah]

On (Fri) 22 Jul 2011 [16:45:55], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > Passing on '0' as ballooning target to indicate retrieval of stats is
> > bad API.  It also makes 'balloon 0' in the monitor cause a segfault.
> > Have two different functions handle the different functionality instead.
> >
> > Reported-by: Mike Cao <bcao@redhat.com>
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> 
> Can you explain the fault?  It's not obvious to me...

There's a bt at:

https://bugzilla.redhat.com/show_bug.cgi?id=694378

The callback is populated when called via 'info balloon', where some
detail is printed on the monitor after the guest responds with the
current balloon info.

On the other hand, 'balloon X' just updates the balloon size; with no
information to be printed.  When 'balloon 0' is issued,
virtio_balloon_to_target() thinks it is the 'info balloon' command,
gets info from the guest, and then tries to call the monitor callback
to print the info it got... and segfaults.

> > --- a/hw/virtio-balloon.c
> > +++ b/hw/virtio-balloon.c
> > @@ -227,8 +227,7 @@ static void virtio_balloon_stat(void *opaque, MonitorCompletion cb,
> >      complete_stats_request(dev);
> >  }
> >  
> > -static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
> > -                                     MonitorCompletion cb, void *cb_data)
> > +static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
> >  {
> >      VirtIOBalloon *dev = opaque;
> >  
> > @@ -238,8 +237,6 @@ static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
> >      if (target) {
> >          dev->num_pages = (ram_size - target) >> VIRTIO_BALLOON_PFN_SHIFT;
> >          virtio_notify_config(&dev->vdev);
> > -    } else {
> > -        virtio_balloon_stat(opaque, cb, cb_data);
> >      }
> >  }
> >  
> 
> Special case: do nothing when target == 0.  Is that necessary/

Why make a round trip to the guest when we're doing nothing?

		Amit

Re: [Qemu-devel] [PATCH 5/7] balloon: Separate out stat and balloon handling

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> On (Fri) 22 Jul 2011 [16:45:55], Markus Armbruster wrote:
>> Amit Shah <amit.shah@redhat.com> writes:
>> 
>> > Passing on '0' as ballooning target to indicate retrieval of stats is
>> > bad API.  It also makes 'balloon 0' in the monitor cause a segfault.
>> > Have two different functions handle the different functionality instead.
>> >
>> > Reported-by: Mike Cao <bcao@redhat.com>
>> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
>> 
>> Can you explain the fault?  It's not obvious to me...
>
> There's a bt at:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=694378
>
> The callback is populated when called via 'info balloon', where some
> detail is printed on the monitor after the guest responds with the
> current balloon info.

Which callback?  There are a few...

> On the other hand, 'balloon X' just updates the balloon size; with no
> information to be printed.  When 'balloon 0' is issued,
> virtio_balloon_to_target() thinks it is the 'info balloon' command,
> gets info from the guest, and then tries to call the monitor callback
> to print the info it got... and segfaults.

I still don't get it.

Okay, back from the debugger; here's what happens.

1. do_info_balloon() is an info_async() method.  It receives a callback
   with argument, to be called exactly once (callback frees the
   argument).  It passes the callback via qemu_balloon_status() and
   indirectly through qemu_balloon_event to virtio_balloon_to_target().

   virtio_balloon_to_target() executes its balloon stats half.  It
   stores the callback in the device state.

   If it can't send a stats request, it resets stats and calls the
   callback right away.

   Else, it sends a stats request.  The device model runs the callback
   when it receives the answer.

   Works.

2. do_balloon() is a cmd_async() method.  It receives a callback with
   argument, to be called when the command completes.  do_balloon()
   calls it right before it succeeds.  Odd, but should work.

   Nevertheless, it passes the callback on via qemu_ballon() and
   indirectly through qemu_balloon_event to virtio_balloon_to_target().

   a. If the argument is non-zero, virtio_balloon_to_target() executes
      its balloon half, which doesn't use the callback in any way.

      Odd, but works.

   b. If the argument is zero, virtio_balloon_to_target() executes its
      balloon stats half, just like in 1.  It either calls the callback
      right away, or arranges for it to be called later.

      Thus, the callback runs twice: use after free and double free.

Test case: start with -S -device virtio-balloon, execute "balloon 0" in
human monitor.  Runs the callback first from virtio_balloon_to_target(),
then again from do_balloon().

>> > --- a/hw/virtio-balloon.c
>> > +++ b/hw/virtio-balloon.c
>> > @@ -227,8 +227,7 @@ static void virtio_balloon_stat(void *opaque, MonitorCompletion cb,
>> >      complete_stats_request(dev);
>> >  }
>> >  
>> > -static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
>> > -                                     MonitorCompletion cb, void *cb_data)
>> > +static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
>> >  {
>> >      VirtIOBalloon *dev = opaque;
>> >  
>> > @@ -238,8 +237,6 @@ static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
>> >      if (target) {
>> >          dev->num_pages = (ram_size - target) >> VIRTIO_BALLOON_PFN_SHIFT;
>> >          virtio_notify_config(&dev->vdev);
>> > -    } else {
>> > -        virtio_balloon_stat(opaque, cb, cb_data);
>> >      }
>> >  }
>> >  
>> 
>> Special case: do nothing when target == 0.  Is that necessary/
>
> Why make a round trip to the guest when we're doing nothing?

Smells like unwarranted optimization of an uncommon case to me.

[Qemu-devel] [PULL 0/7] virtio-balloon: cleanups, fix segfault from use-after-free

[Amit Shah]

Hello,

This same as the last week's patchset, with Markus's analysis included
in 5/7's commit log.

I think this should go to 0.15 as well.

This series cleans up the virtio-balloon driver and fixes a
use-after-free segfault when 'balloon 0' is issued in the monitor.

The following changes since commit c886edfb851c0c590d4e77f058f2ec8ed95ad1b5:

  Let users select their pythons (2011-07-25 16:50:12 +0000)

are available in the git repository at:
  git://git.kernel.org/pub/scm/virt/qemu/amit/misc.git for-anthony


Amit Shah (7):
  balloon: Make functions, local vars static
  balloon: Add braces around if statements
  balloon: Simplify code flow
  virtio-balloon: Separate status handling into separate function
  balloon: Separate out stat and balloon handling
  balloon: Fix header comment; add Copyright
  virtio-balloon: Fix header comment; add Copyright

 balloon.c           |   47 +++++++++++++++++++++------------------
 balloon.h           |   12 ++++------
 hw/virtio-balloon.c |   60 +++++++++++++++++++++++++++++---------------------
 3 files changed, 65 insertions(+), 54 deletions(-)

-- 
1.7.6

[Qemu-devel] [PATCH 1/7] balloon: Make functions, local vars static

[Amit Shah]

balloon.h had function declarations for a couple of functions that are
local to balloon.c.  Make them static.

Drop the 'qemu_' prefix for balloon.c-local variables, and make them
static.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 balloon.c |   22 +++++++++++-----------
 balloon.h |    4 ----
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/balloon.c b/balloon.c
index 248c1b5..f9bcf07 100644
--- a/balloon.c
+++ b/balloon.c
@@ -31,30 +31,30 @@
 #include "trace.h"
 
 
-static QEMUBalloonEvent *qemu_balloon_event;
-void *qemu_balloon_event_opaque;
+static QEMUBalloonEvent *balloon_event_fn;
+static void *balloon_opaque;
 
 void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque)
 {
-    qemu_balloon_event = func;
-    qemu_balloon_event_opaque = opaque;
+    balloon_event_fn = func;
+    balloon_opaque = opaque;
 }
 
-int qemu_balloon(ram_addr_t target, MonitorCompletion cb, void *opaque)
+static int qemu_balloon(ram_addr_t target, MonitorCompletion cb, void *opaque)
 {
-    if (qemu_balloon_event) {
-        trace_balloon_event(qemu_balloon_event_opaque, target);
-        qemu_balloon_event(qemu_balloon_event_opaque, target, cb, opaque);
+    if (balloon_event_fn) {
+        trace_balloon_event(balloon_opaque, target);
+        balloon_event_fn(balloon_opaque, target, cb, opaque);
         return 1;
     } else {
         return 0;
     }
 }
 
-int qemu_balloon_status(MonitorCompletion cb, void *opaque)
+static int qemu_balloon_status(MonitorCompletion cb, void *opaque)
 {
-    if (qemu_balloon_event) {
-        qemu_balloon_event(qemu_balloon_event_opaque, 0, cb, opaque);
+    if (balloon_event_fn) {
+        balloon_event_fn(balloon_opaque, 0, cb, opaque);
         return 1;
     } else {
         return 0;
diff --git a/balloon.h b/balloon.h
index d478e28..06a8a46 100644
--- a/balloon.h
+++ b/balloon.h
@@ -21,10 +21,6 @@ typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target,
 
 void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque);
 
-int qemu_balloon(ram_addr_t target, MonitorCompletion cb, void *opaque);
-
-int qemu_balloon_status(MonitorCompletion cb, void *opaque);
-
 void monitor_print_balloon(Monitor *mon, const QObject *data);
 int do_info_balloon(Monitor *mon, MonitorCompletion cb, void *opaque);
 int do_balloon(Monitor *mon, const QDict *params,
-- 
1.7.6

[Qemu-devel] [PATCH 2/7] balloon: Add braces around if statements

[Amit Shah]

Signed-off-by: Amit Shah <amit.shah@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 balloon.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/balloon.c b/balloon.c
index f9bcf07..86f629e 100644
--- a/balloon.c
+++ b/balloon.c
@@ -65,9 +65,10 @@ static void print_balloon_stat(const char *key, QObject *obj, void *opaque)
 {
     Monitor *mon = opaque;
 
-    if (strcmp(key, "actual"))
+    if (strcmp(key, "actual")) {
         monitor_printf(mon, ",%s=%" PRId64, key,
                        qint_get_int(qobject_to_qint(obj)));
+    }
 }
 
 void monitor_print_balloon(Monitor *mon, const QObject *data)
@@ -75,9 +76,9 @@ void monitor_print_balloon(Monitor *mon, const QObject *data)
     QDict *qdict;
 
     qdict = qobject_to_qdict(data);
-    if (!qdict_haskey(qdict, "actual"))
+    if (!qdict_haskey(qdict, "actual")) {
         return;
-
+    }
     monitor_printf(mon, "balloon: actual=%" PRId64,
                    qdict_get_int(qdict, "actual") >> 20);
     qdict_iter(qdict, print_balloon_stat, mon);
-- 
1.7.6

[Qemu-devel] [PATCH 3/7] balloon: Simplify code flow

[Amit Shah]

Replace:
  if (foo) {
    ...
  } else {
    return 0;
  }

by

  if (!foo) {
    return 0;
  }
  ...

Signed-off-by: Amit Shah <amit.shah@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 balloon.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/balloon.c b/balloon.c
index 86f629e..d40be39 100644
--- a/balloon.c
+++ b/balloon.c
@@ -42,23 +42,21 @@ void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque)
 
 static int qemu_balloon(ram_addr_t target, MonitorCompletion cb, void *opaque)
 {
-    if (balloon_event_fn) {
-        trace_balloon_event(balloon_opaque, target);
-        balloon_event_fn(balloon_opaque, target, cb, opaque);
-        return 1;
-    } else {
+    if (!balloon_event_fn) {
         return 0;
     }
+    trace_balloon_event(balloon_opaque, target);
+    balloon_event_fn(balloon_opaque, target, cb, opaque);
+    return 1;
 }
 
 static int qemu_balloon_status(MonitorCompletion cb, void *opaque)
 {
-    if (balloon_event_fn) {
-        balloon_event_fn(balloon_opaque, 0, cb, opaque);
-        return 1;
-    } else {
+    if (!balloon_event_fn) {
         return 0;
     }
+    balloon_event_fn(balloon_opaque, 0, cb, opaque);
+    return 1;
 }
 
 static void print_balloon_stat(const char *key, QObject *obj, void *opaque)
-- 
1.7.6

[Qemu-devel] [PATCH 4/7] virtio-balloon: Separate status handling into separate function

[Amit Shah]

Separate out the code to retrieve balloon info from the code that sets
balloon values.

This will be used to separate the two callbacks from balloon.c and help
cope with 'balloon 0' on the monitor.  Currently, 'balloon 0' causes a
segfault in monitor_resume().

Signed-off-by: Amit Shah <amit.shah@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hw/virtio-balloon.c |   51 +++++++++++++++++++++++++++++++--------------------
 1 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 70a8710..2f371f2 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -199,36 +199,47 @@ static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
     return f;
 }
 
+static void virtio_balloon_stat(void *opaque, MonitorCompletion cb,
+                                void *cb_data)
+{
+    VirtIOBalloon *dev = opaque;
+
+    /* For now, only allow one request at a time.  This restriction can be
+     * removed later by queueing callback and data pairs.
+     */
+    if (dev->stats_callback != NULL) {
+        return;
+    }
+    dev->stats_callback = cb;
+    dev->stats_opaque_callback_data = cb_data;
+
+    if (ENABLE_GUEST_STATS
+        && (dev->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ))) {
+        virtqueue_push(dev->svq, &dev->stats_vq_elem, dev->stats_vq_offset);
+        virtio_notify(&dev->vdev, dev->svq);
+        return;
+    }
+
+    /* Stats are not supported.  Clear out any stale values that might
+     * have been set by a more featureful guest kernel.
+     */
+    reset_stats(dev);
+    complete_stats_request(dev);
+}
+
 static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
                                      MonitorCompletion cb, void *cb_data)
 {
     VirtIOBalloon *dev = opaque;
 
-    if (target > ram_size)
+    if (target > ram_size) {
         target = ram_size;
-
+    }
     if (target) {
         dev->num_pages = (ram_size - target) >> VIRTIO_BALLOON_PFN_SHIFT;
         virtio_notify_config(&dev->vdev);
     } else {
-        /* For now, only allow one request at a time.  This restriction can be
-         * removed later by queueing callback and data pairs.
-         */
-        if (dev->stats_callback != NULL) {
-            return;
-        }
-        dev->stats_callback = cb;
-        dev->stats_opaque_callback_data = cb_data; 
-        if (ENABLE_GUEST_STATS && (dev->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ))) {
-            virtqueue_push(dev->svq, &dev->stats_vq_elem, dev->stats_vq_offset);
-            virtio_notify(&dev->vdev, dev->svq);
-        } else {
-            /* Stats are not supported.  Clear out any stale values that might
-             * have been set by a more featureful guest kernel.
-             */
-            reset_stats(dev);
-            complete_stats_request(dev);
-        }
+        virtio_balloon_stat(opaque, cb, cb_data);
     }
 }
 
-- 
1.7.6

[Qemu-devel] [PATCH 5/7] balloon: Separate out stat and balloon handling

[Amit Shah]

Passing on '0' as ballooning target to indicate retrieval of stats is
bad API.  It also makes 'balloon 0' in the monitor cause a segfault.
Have two different functions handle the different functionality instead.

Detailed explanation from Markus's review:

1. do_info_balloon() is an info_async() method.  It receives a callback
   with argument, to be called exactly once (callback frees the
   argument).  It passes the callback via qemu_balloon_status() and
   indirectly through qemu_balloon_event to virtio_balloon_to_target().

   virtio_balloon_to_target() executes its balloon stats half.  It
   stores the callback in the device state.

   If it can't send a stats request, it resets stats and calls the
   callback right away.

   Else, it sends a stats request.  The device model runs the callback
   when it receives the answer.

   Works.

2. do_balloon() is a cmd_async() method.  It receives a callback with
   argument, to be called when the command completes.  do_balloon()
   calls it right before it succeeds.  Odd, but should work.

   Nevertheless, it passes the callback on via qemu_ballon() and
   indirectly through qemu_balloon_event to virtio_balloon_to_target().

   a. If the argument is non-zero, virtio_balloon_to_target() executes
      its balloon half, which doesn't use the callback in any way.

      Odd, but works.

   b. If the argument is zero, virtio_balloon_to_target() executes its
      balloon stats half, just like in 1.  It either calls the callback
      right away, or arranges for it to be called later.

      Thus, the callback runs twice: use after free and double free.

Test case: start with -S -device virtio-balloon, execute "balloon 0" in
human monitor.  Runs the callback first from virtio_balloon_to_target(),
then again from do_balloon().

Reported-by: Mike Cao <bcao@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 balloon.c           |   17 ++++++++++-------
 balloon.h           |    8 +++++---
 hw/virtio-balloon.c |    7 ++-----
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/balloon.c b/balloon.c
index d40be39..8be3812 100644
--- a/balloon.c
+++ b/balloon.c
@@ -32,30 +32,33 @@
 
 
 static QEMUBalloonEvent *balloon_event_fn;
+static QEMUBalloonStatus *balloon_stat_fn;
 static void *balloon_opaque;
 
-void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque)
+void qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
+                              QEMUBalloonStatus *stat_func, void *opaque)
 {
-    balloon_event_fn = func;
+    balloon_event_fn = event_func;
+    balloon_stat_fn = stat_func;
     balloon_opaque = opaque;
 }
 
-static int qemu_balloon(ram_addr_t target, MonitorCompletion cb, void *opaque)
+static int qemu_balloon(ram_addr_t target)
 {
     if (!balloon_event_fn) {
         return 0;
     }
     trace_balloon_event(balloon_opaque, target);
-    balloon_event_fn(balloon_opaque, target, cb, opaque);
+    balloon_event_fn(balloon_opaque, target);
     return 1;
 }
 
 static int qemu_balloon_status(MonitorCompletion cb, void *opaque)
 {
-    if (!balloon_event_fn) {
+    if (!balloon_stat_fn) {
         return 0;
     }
-    balloon_event_fn(balloon_opaque, 0, cb, opaque);
+    balloon_stat_fn(balloon_opaque, cb, opaque);
     return 1;
 }
 
@@ -135,7 +138,7 @@ int do_balloon(Monitor *mon, const QDict *params,
         return -1;
     }
 
-    ret = qemu_balloon(qdict_get_int(params, "value"), cb, opaque);
+    ret = qemu_balloon(qdict_get_int(params, "value"));
     if (ret == 0) {
         qerror_report(QERR_DEVICE_NOT_ACTIVE, "balloon");
         return -1;
diff --git a/balloon.h b/balloon.h
index 06a8a46..a6c31d5 100644
--- a/balloon.h
+++ b/balloon.h
@@ -16,10 +16,12 @@
 
 #include "monitor.h"
 
-typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target,
-                                MonitorCompletion cb, void *cb_data);
+typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
+typedef void (QEMUBalloonStatus)(void *opaque, MonitorCompletion cb,
+                                 void *cb_data);
 
-void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque);
+void qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
+                              QEMUBalloonStatus *stat_func, void *opaque);
 
 void monitor_print_balloon(Monitor *mon, const QObject *data);
 int do_info_balloon(Monitor *mon, MonitorCompletion cb, void *opaque);
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 2f371f2..40b43b0 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -227,8 +227,7 @@ static void virtio_balloon_stat(void *opaque, MonitorCompletion cb,
     complete_stats_request(dev);
 }
 
-static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
-                                     MonitorCompletion cb, void *cb_data)
+static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
 {
     VirtIOBalloon *dev = opaque;
 
@@ -238,8 +237,6 @@ static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
     if (target) {
         dev->num_pages = (ram_size - target) >> VIRTIO_BALLOON_PFN_SHIFT;
         virtio_notify_config(&dev->vdev);
-    } else {
-        virtio_balloon_stat(opaque, cb, cb_data);
     }
 }
 
@@ -284,7 +281,7 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
     s->svq = virtio_add_queue(&s->vdev, 128, virtio_balloon_receive_stats);
 
     reset_stats(s);
-    qemu_add_balloon_handler(virtio_balloon_to_target, s);
+    qemu_add_balloon_handler(virtio_balloon_to_target, virtio_balloon_stat, s);
 
     register_savevm(dev, "virtio-balloon", -1, 1,
                     virtio_balloon_save, virtio_balloon_load, s);
-- 
1.7.6

[Qemu-devel] [PATCH 6/7] balloon: Fix header comment; add Copyright

[Amit Shah]

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 balloon.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/balloon.c b/balloon.c
index 8be3812..a938475 100644
--- a/balloon.c
+++ b/balloon.c
@@ -1,7 +1,9 @@
 /*
- * QEMU System Emulator
+ * Generic Balloon handlers and management
  *
  * Copyright (c) 2003-2008 Fabrice Bellard
+ * Copyright (C) 2011 Red Hat, Inc.
+ * Copyright (C) 2011 Amit Shah <amit.shah@redhat.com>
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -30,7 +32,6 @@
 #include "balloon.h"
 #include "trace.h"
 
-
 static QEMUBalloonEvent *balloon_event_fn;
 static QEMUBalloonStatus *balloon_stat_fn;
 static void *balloon_opaque;
-- 
1.7.6

[Qemu-devel] [PATCH 7/7] virtio-balloon: Fix header comment; add Copyright

[Amit Shah]

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-balloon.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 40b43b0..2ba7e95 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -1,7 +1,9 @@
 /*
- * Virtio Block Device
+ * Virtio Balloon Device
  *
  * Copyright IBM, Corp. 2008
+ * Copyright (C) 2011 Red Hat, Inc.
+ * Copyright (C) 2011 Amit Shah <amit.shah@redhat.com>
  *
  * Authors:
  *  Anthony Liguori   <aliguori@us.ibm.com>
-- 
1.7.6