Amit Shah <amit.s...@redhat.com> writes:
> On (Fri) 22 Jul 2011 [16:45:55], Markus Armbruster wrote:
>> Amit Shah <amit.s...@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 <b...@redhat.com>
>> > Signed-off-by: Amit Shah <amit.s...@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.
