| Message ID | b88a906994b10ac7418586242cd349c247de2813.1311149456.git.amit.shah@redhat.com |
|---|---|
| State | New |
| Headers | show |
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);
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
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.
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);
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(-)