Patchwork Add event notification for guest balloon changes

login
register
mail settings
Submitter Daniel P. Berrange
Date May 16, 2012, 10:10 a.m.
Message ID <1337163047-6159-1-git-send-email-berrange@redhat.com>
Download mbox | patch
Permalink /patch/159569/
State New
Headers show

Comments

Daniel P. Berrange - May 16, 2012, 10:10 a.m.
From: "Daniel P. Berrange" <berrange@redhat.com>

After setting a balloon target value, applications have to
continually poll 'query-balloon' to determine whether the
guest has reacted to this request. The virtio-balloon backend
knows exactly when the guest has reacted though, and thus it
is possible to emit a JSON event to tell the mgmt application
whenever the guest balloon changes.

This introduces a new 'qemu_balloon_change()' API which is
to be called by balloon driver backends, whenever they have
a change in balloon value. This takes the 'actual' balloon
value, as would be found in the BalloonInfo struct.

The qemu_balloon_change API emits a JSON monitor event which
looks like:

  {"timestamp": {"seconds": 1337162462, "microseconds": 814521},
   "event": "BALLOON_CHANGE", "data": {"actual": 944766976}}

* balloon.c, balloon.h: Introduce qemu_balloon_change() for
  emitting balloon change events on the monitor
* hw/virtio-balloon.c: Invoke qemu_balloon_change() whenever
  the guest changes the balloon actual value
* monitor.c, monitor.h: Define QEVENT_BALLOON_CHANGE

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 balloon.c           |   14 ++++++++++++++
 balloon.h           |    2 ++
 hw/virtio-balloon.c |    5 +++++
 monitor.c           |    3 +++
 monitor.h           |    1 +
 5 files changed, 25 insertions(+), 0 deletions(-)
Luiz Capitulino - May 16, 2012, 6:42 p.m.
On Wed, 16 May 2012 11:10:47 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> After setting a balloon target value, applications have to
> continually poll 'query-balloon' to determine whether the
> guest has reacted to this request. The virtio-balloon backend
> knows exactly when the guest has reacted though, and thus it
> is possible to emit a JSON event to tell the mgmt application
> whenever the guest balloon changes.
> 
> This introduces a new 'qemu_balloon_change()' API which is
> to be called by balloon driver backends, whenever they have
> a change in balloon value. This takes the 'actual' balloon
> value, as would be found in the BalloonInfo struct.
> 
> The qemu_balloon_change API emits a JSON monitor event which
> looks like:
> 
>   {"timestamp": {"seconds": 1337162462, "microseconds": 814521},
>    "event": "BALLOON_CHANGE", "data": {"actual": 944766976}}

It's missing an entry in QMP/qmp-events.txt and I have a comment below,
but in general looks good.

Amit, would be good to get your ack.

> 
> * balloon.c, balloon.h: Introduce qemu_balloon_change() for
>   emitting balloon change events on the monitor
> * hw/virtio-balloon.c: Invoke qemu_balloon_change() whenever
>   the guest changes the balloon actual value
> * monitor.c, monitor.h: Define QEVENT_BALLOON_CHANGE
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  balloon.c           |   14 ++++++++++++++
>  balloon.h           |    2 ++
>  hw/virtio-balloon.c |    5 +++++
>  monitor.c           |    3 +++
>  monitor.h           |    1 +
>  5 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/balloon.c b/balloon.c
> index aa354f7..913862b 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -30,6 +30,7 @@
>  #include "balloon.h"
>  #include "trace.h"
>  #include "qmp-commands.h"
> +#include "qjson.h"
>  
>  static QEMUBalloonEvent *balloon_event_fn;
>  static QEMUBalloonStatus *balloon_stat_fn;
> @@ -80,6 +81,19 @@ static int qemu_balloon_status(BalloonInfo *info)
>      return 1;
>  }
>  
> +void qemu_balloon_change(int64_t actual)
> +{
> +    QObject *data;
> +
> +    data = qobject_from_jsonf("{ 'actual': %" PRId64 " }",
> +                              actual);
> +
> +    monitor_protocol_event(QEVENT_BALLOON_CHANGE, data);
> +
> +    qobject_decref(data);
> +}
> +
> +
>  BalloonInfo *qmp_query_balloon(Error **errp)
>  {
>      BalloonInfo *info;
> diff --git a/balloon.h b/balloon.h
> index b60fd5d..2ebac0d 100644
> --- a/balloon.h
> +++ b/balloon.h
> @@ -24,4 +24,6 @@ int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
>  			     QEMUBalloonStatus *stat_func, void *opaque);
>  void qemu_remove_balloon_handler(void *opaque);
>  
> +void qemu_balloon_change(int64_t actual);
> +
>  #endif
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index ce9d2c9..9137573 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -146,8 +146,13 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
>  {
>      VirtIOBalloon *dev = to_virtio_balloon(vdev);
>      struct virtio_balloon_config config;
> +    uint32_t oldactual = dev->actual;
>      memcpy(&config, config_data, 8);
>      dev->actual = le32_to_cpu(config.actual);
> +    if (dev->actual != oldactual) {
> +        qemu_balloon_change(ram_size -
> +                            (dev->actual << VIRTIO_BALLOON_PFN_SHIFT));
> +    }

This can cause several events to be emitted until the memory is adjusted
to the value asked by the user. I'm undecided if this is a feature, but
if I were a client issuing the balloon command I'd expect to get the event
only when the memory is fully adjusted to the value I asked.

Not sure if this possible to implement though, or if we really want it.


>  }
>  
>  static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
> diff --git a/monitor.c b/monitor.c
> index 12a6fe2..ef59cd9 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -493,6 +493,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
>          case QEVENT_WAKEUP:
>              event_name = "WAKEUP";
>              break;
> +        case QEVENT_BALLOON_CHANGE:
> +            event_name = "BALLOON_CHANGE";
> +            break;
>          default:
>              abort();
>              break;
> diff --git a/monitor.h b/monitor.h
> index 0d49800..8de0160 100644
> --- a/monitor.h
> +++ b/monitor.h
> @@ -41,6 +41,7 @@ typedef enum MonitorEvent {
>      QEVENT_DEVICE_TRAY_MOVED,
>      QEVENT_SUSPEND,
>      QEVENT_WAKEUP,
> +    QEVENT_BALLOON_CHANGE,
>      QEVENT_MAX,
>  } MonitorEvent;
>
Anthony Liguori - May 16, 2012, 6:58 p.m.
On 05/16/2012 01:42 PM, Luiz Capitulino wrote:
> On Wed, 16 May 2012 11:10:47 +0100
> "Daniel P. Berrange"<berrange@redhat.com>  wrote:
>
>> From: "Daniel P. Berrange"<berrange@redhat.com>
>>
>> After setting a balloon target value, applications have to
>> continually poll 'query-balloon' to determine whether the
>> guest has reacted to this request. The virtio-balloon backend
>> knows exactly when the guest has reacted though, and thus it
>> is possible to emit a JSON event to tell the mgmt application
>> whenever the guest balloon changes.
>>
>> This introduces a new 'qemu_balloon_change()' API which is
>> to be called by balloon driver backends, whenever they have
>> a change in balloon value. This takes the 'actual' balloon
>> value, as would be found in the BalloonInfo struct.
>>
>> The qemu_balloon_change API emits a JSON monitor event which
>> looks like:
>>
>>    {"timestamp": {"seconds": 1337162462, "microseconds": 814521},
>>     "event": "BALLOON_CHANGE", "data": {"actual": 944766976}}
>
> It's missing an entry in QMP/qmp-events.txt and I have a comment below,
> but in general looks good.
>
> Amit, would be good to get your ack.

I think it would be safer to limit this event to (1) only firing once target has 
been reached (2) firing if target is deviated from without a corresponding 
change in target.

Otherwise, a guest could just flood libvirt with events.  This would queue 
memory in QEMU indefinitely as the events got queued up to potentially serving 
as a DoS against other guests.

Regards,

Anthony LIguori

>
>>
>> * balloon.c, balloon.h: Introduce qemu_balloon_change() for
>>    emitting balloon change events on the monitor
>> * hw/virtio-balloon.c: Invoke qemu_balloon_change() whenever
>>    the guest changes the balloon actual value
>> * monitor.c, monitor.h: Define QEVENT_BALLOON_CHANGE
>>
>> Signed-off-by: Daniel P. Berrange<berrange@redhat.com>
>> ---
>>   balloon.c           |   14 ++++++++++++++
>>   balloon.h           |    2 ++
>>   hw/virtio-balloon.c |    5 +++++
>>   monitor.c           |    3 +++
>>   monitor.h           |    1 +
>>   5 files changed, 25 insertions(+), 0 deletions(-)
>>
>> diff --git a/balloon.c b/balloon.c
>> index aa354f7..913862b 100644
>> --- a/balloon.c
>> +++ b/balloon.c
>> @@ -30,6 +30,7 @@
>>   #include "balloon.h"
>>   #include "trace.h"
>>   #include "qmp-commands.h"
>> +#include "qjson.h"
>>
>>   static QEMUBalloonEvent *balloon_event_fn;
>>   static QEMUBalloonStatus *balloon_stat_fn;
>> @@ -80,6 +81,19 @@ static int qemu_balloon_status(BalloonInfo *info)
>>       return 1;
>>   }
>>
>> +void qemu_balloon_change(int64_t actual)
>> +{
>> +    QObject *data;
>> +
>> +    data = qobject_from_jsonf("{ 'actual': %" PRId64 " }",
>> +                              actual);
>> +
>> +    monitor_protocol_event(QEVENT_BALLOON_CHANGE, data);
>> +
>> +    qobject_decref(data);
>> +}
>> +
>> +
>>   BalloonInfo *qmp_query_balloon(Error **errp)
>>   {
>>       BalloonInfo *info;
>> diff --git a/balloon.h b/balloon.h
>> index b60fd5d..2ebac0d 100644
>> --- a/balloon.h
>> +++ b/balloon.h
>> @@ -24,4 +24,6 @@ int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
>>   			     QEMUBalloonStatus *stat_func, void *opaque);
>>   void qemu_remove_balloon_handler(void *opaque);
>>
>> +void qemu_balloon_change(int64_t actual);
>> +
>>   #endif
>> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
>> index ce9d2c9..9137573 100644
>> --- a/hw/virtio-balloon.c
>> +++ b/hw/virtio-balloon.c
>> @@ -146,8 +146,13 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
>>   {
>>       VirtIOBalloon *dev = to_virtio_balloon(vdev);
>>       struct virtio_balloon_config config;
>> +    uint32_t oldactual = dev->actual;
>>       memcpy(&config, config_data, 8);
>>       dev->actual = le32_to_cpu(config.actual);
>> +    if (dev->actual != oldactual) {
>> +        qemu_balloon_change(ram_size -
>> +                            (dev->actual<<  VIRTIO_BALLOON_PFN_SHIFT));
>> +    }
>
> This can cause several events to be emitted until the memory is adjusted
> to the value asked by the user. I'm undecided if this is a feature, but
> if I were a client issuing the balloon command I'd expect to get the event
> only when the memory is fully adjusted to the value I asked.
>
> Not sure if this possible to implement though, or if we really want it.
>
>
>>   }
>>
>>   static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
>> diff --git a/monitor.c b/monitor.c
>> index 12a6fe2..ef59cd9 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -493,6 +493,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
>>           case QEVENT_WAKEUP:
>>               event_name = "WAKEUP";
>>               break;
>> +        case QEVENT_BALLOON_CHANGE:
>> +            event_name = "BALLOON_CHANGE";
>> +            break;
>>           default:
>>               abort();
>>               break;
>> diff --git a/monitor.h b/monitor.h
>> index 0d49800..8de0160 100644
>> --- a/monitor.h
>> +++ b/monitor.h
>> @@ -41,6 +41,7 @@ typedef enum MonitorEvent {
>>       QEVENT_DEVICE_TRAY_MOVED,
>>       QEVENT_SUSPEND,
>>       QEVENT_WAKEUP,
>> +    QEVENT_BALLOON_CHANGE,
>>       QEVENT_MAX,
>>   } MonitorEvent;
>>
>
Luiz Capitulino - May 16, 2012, 7:03 p.m.
On Wed, 16 May 2012 13:58:34 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 05/16/2012 01:42 PM, Luiz Capitulino wrote:
> > On Wed, 16 May 2012 11:10:47 +0100
> > "Daniel P. Berrange"<berrange@redhat.com>  wrote:
> >
> >> From: "Daniel P. Berrange"<berrange@redhat.com>
> >>
> >> After setting a balloon target value, applications have to
> >> continually poll 'query-balloon' to determine whether the
> >> guest has reacted to this request. The virtio-balloon backend
> >> knows exactly when the guest has reacted though, and thus it
> >> is possible to emit a JSON event to tell the mgmt application
> >> whenever the guest balloon changes.
> >>
> >> This introduces a new 'qemu_balloon_change()' API which is
> >> to be called by balloon driver backends, whenever they have
> >> a change in balloon value. This takes the 'actual' balloon
> >> value, as would be found in the BalloonInfo struct.
> >>
> >> The qemu_balloon_change API emits a JSON monitor event which
> >> looks like:
> >>
> >>    {"timestamp": {"seconds": 1337162462, "microseconds": 814521},
> >>     "event": "BALLOON_CHANGE", "data": {"actual": 944766976}}
> >
> > It's missing an entry in QMP/qmp-events.txt and I have a comment below,
> > but in general looks good.
> >
> > Amit, would be good to get your ack.
> 
> I think it would be safer to limit this event to (1) only firing once target has 
> been reached (2) firing if target is deviated from without a corresponding 
> change in target.
> 
> Otherwise, a guest could just flood libvirt with events.  This would queue 
> memory in QEMU indefinitely as the events got queued up to potentially serving 
> as a DoS against other guests.

Yeah, I've said something along these lines below:

> >> @@ -146,8 +146,13 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
> >>   {
> >>       VirtIOBalloon *dev = to_virtio_balloon(vdev);
> >>       struct virtio_balloon_config config;
> >> +    uint32_t oldactual = dev->actual;
> >>       memcpy(&config, config_data, 8);
> >>       dev->actual = le32_to_cpu(config.actual);
> >> +    if (dev->actual != oldactual) {
> >> +        qemu_balloon_change(ram_size -
> >> +                            (dev->actual<<  VIRTIO_BALLOON_PFN_SHIFT));
> >> +    }
> >
> > This can cause several events to be emitted until the memory is adjusted
> > to the value asked by the user. I'm undecided if this is a feature, but
> > if I were a client issuing the balloon command I'd expect to get the event
> > only when the memory is fully adjusted to the value I asked.
> >
> > Not sure if this possible to implement though, or if we really want it.
Daniel P. Berrange - May 17, 2012, 7:49 a.m.
On Wed, May 16, 2012 at 01:58:34PM -0500, Anthony Liguori wrote:
> On 05/16/2012 01:42 PM, Luiz Capitulino wrote:
> >On Wed, 16 May 2012 11:10:47 +0100
> >"Daniel P. Berrange"<berrange@redhat.com>  wrote:
> >
> >>From: "Daniel P. Berrange"<berrange@redhat.com>
> >>
> >>After setting a balloon target value, applications have to
> >>continually poll 'query-balloon' to determine whether the
> >>guest has reacted to this request. The virtio-balloon backend
> >>knows exactly when the guest has reacted though, and thus it
> >>is possible to emit a JSON event to tell the mgmt application
> >>whenever the guest balloon changes.
> >>
> >>This introduces a new 'qemu_balloon_change()' API which is
> >>to be called by balloon driver backends, whenever they have
> >>a change in balloon value. This takes the 'actual' balloon
> >>value, as would be found in the BalloonInfo struct.
> >>
> >>The qemu_balloon_change API emits a JSON monitor event which
> >>looks like:
> >>
> >>   {"timestamp": {"seconds": 1337162462, "microseconds": 814521},
> >>    "event": "BALLOON_CHANGE", "data": {"actual": 944766976}}
> >
> >It's missing an entry in QMP/qmp-events.txt and I have a comment below,
> >but in general looks good.
> >
> >Amit, would be good to get your ack.
> 
> I think it would be safer to limit this event to (1) only firing
> once target has been reached (2) firing if target is deviated from
> without a corresponding change in target.
> 
> Otherwise, a guest could just flood libvirt with events.  This would
> queue memory in QEMU indefinitely as the events got queued up to
> potentially serving as a DoS against other guests.

Hmm, that's a good point, but my concern was that if we only emit
the event when the target is reached, what happens if the guest
gets very close to the target but never actually reaches it for
some reason.

Should we perhaps just rate limit it to once per second ?

BTW, if we're considering guest initiated events to be a potential
DOS in this way, then I should point out the RTC_CHANGE event
will already suffer this way, if a malicious guest continually
adjusts its hardware close. So we might want to apply rate limiting
to that event too ?


Daniel
Luiz Capitulino - May 17, 2012, 12:56 p.m.
On Thu, 17 May 2012 08:49:44 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Wed, May 16, 2012 at 01:58:34PM -0500, Anthony Liguori wrote:
> > On 05/16/2012 01:42 PM, Luiz Capitulino wrote:
> > >On Wed, 16 May 2012 11:10:47 +0100
> > >"Daniel P. Berrange"<berrange@redhat.com>  wrote:
> > >
> > >>From: "Daniel P. Berrange"<berrange@redhat.com>
> > >>
> > >>After setting a balloon target value, applications have to
> > >>continually poll 'query-balloon' to determine whether the
> > >>guest has reacted to this request. The virtio-balloon backend
> > >>knows exactly when the guest has reacted though, and thus it
> > >>is possible to emit a JSON event to tell the mgmt application
> > >>whenever the guest balloon changes.
> > >>
> > >>This introduces a new 'qemu_balloon_change()' API which is
> > >>to be called by balloon driver backends, whenever they have
> > >>a change in balloon value. This takes the 'actual' balloon
> > >>value, as would be found in the BalloonInfo struct.
> > >>
> > >>The qemu_balloon_change API emits a JSON monitor event which
> > >>looks like:
> > >>
> > >>   {"timestamp": {"seconds": 1337162462, "microseconds": 814521},
> > >>    "event": "BALLOON_CHANGE", "data": {"actual": 944766976}}
> > >
> > >It's missing an entry in QMP/qmp-events.txt and I have a comment below,
> > >but in general looks good.
> > >
> > >Amit, would be good to get your ack.
> > 
> > I think it would be safer to limit this event to (1) only firing
> > once target has been reached (2) firing if target is deviated from
> > without a corresponding change in target.
> > 
> > Otherwise, a guest could just flood libvirt with events.  This would
> > queue memory in QEMU indefinitely as the events got queued up to
> > potentially serving as a DoS against other guests.
> 
> Hmm, that's a good point, but my concern was that if we only emit
> the event when the target is reached, what happens if the guest
> gets very close to the target but never actually reaches it for
> some reason.

Having a way to detect the last balloon change would be perfect.

> Should we perhaps just rate limit it to once per second ?
> 
> BTW, if we're considering guest initiated events to be a potential
> DOS in this way, then I should point out the RTC_CHANGE event
> will already suffer this way, if a malicious guest continually
> adjusts its hardware close. So we might want to apply rate limiting
> to that event too ?

I think several events can suffer from that. For example, a VNC
client could repeatedly connect & disconnect from QEMU. If we're going
to fix this, then we'd need a general solution for it.

But I think the balloon case is different, because we're not fighting
malicious guests/clients, it's really the balloon operation that can
cause the flood.
Anthony Liguori - May 17, 2012, 9:20 p.m.
On 05/17/2012 07:56 AM, Luiz Capitulino wrote:
> On Thu, 17 May 2012 08:49:44 +0100
> "Daniel P. Berrange"<berrange@redhat.com>  wrote:
>
>> On Wed, May 16, 2012 at 01:58:34PM -0500, Anthony Liguori wrote:
>>> On 05/16/2012 01:42 PM, Luiz Capitulino wrote:
>>>> On Wed, 16 May 2012 11:10:47 +0100
>>>> "Daniel P. Berrange"<berrange@redhat.com>   wrote:
>>>>
>>>>> From: "Daniel P. Berrange"<berrange@redhat.com>
>>>>>
>>>>> After setting a balloon target value, applications have to
>>>>> continually poll 'query-balloon' to determine whether the
>>>>> guest has reacted to this request. The virtio-balloon backend
>>>>> knows exactly when the guest has reacted though, and thus it
>>>>> is possible to emit a JSON event to tell the mgmt application
>>>>> whenever the guest balloon changes.
>>>>>
>>>>> This introduces a new 'qemu_balloon_change()' API which is
>>>>> to be called by balloon driver backends, whenever they have
>>>>> a change in balloon value. This takes the 'actual' balloon
>>>>> value, as would be found in the BalloonInfo struct.
>>>>>
>>>>> The qemu_balloon_change API emits a JSON monitor event which
>>>>> looks like:
>>>>>
>>>>>    {"timestamp": {"seconds": 1337162462, "microseconds": 814521},
>>>>>     "event": "BALLOON_CHANGE", "data": {"actual": 944766976}}
>>>>
>>>> It's missing an entry in QMP/qmp-events.txt and I have a comment below,
>>>> but in general looks good.
>>>>
>>>> Amit, would be good to get your ack.
>>>
>>> I think it would be safer to limit this event to (1) only firing
>>> once target has been reached (2) firing if target is deviated from
>>> without a corresponding change in target.
>>>
>>> Otherwise, a guest could just flood libvirt with events.  This would
>>> queue memory in QEMU indefinitely as the events got queued up to
>>> potentially serving as a DoS against other guests.
>>
>> Hmm, that's a good point, but my concern was that if we only emit
>> the event when the target is reached, what happens if the guest
>> gets very close to the target but never actually reaches it for
>> some reason.
>
> Having a way to detect the last balloon change would be perfect.

libvirt certainly would have to maintain a timeout and make a decision on what 
to do if the guest doesn't balloon to target.  Not sure how having events help 
at all here.

>> Should we perhaps just rate limit it to once per second ?
>>
>> BTW, if we're considering guest initiated events to be a potential
>> DOS in this way, then I should point out the RTC_CHANGE event
>> will already suffer this way, if a malicious guest continually
>> adjusts its hardware close. So we might want to apply rate limiting
>> to that event too ?
>
> I think several events can suffer from that. For example, a VNC
> client could repeatedly connect&  disconnect from QEMU. If we're going
> to fix this, then we'd need a general solution for it.

No, VNC clients are a whole different ballgame.  VNC connections will only 
happen from the management network, we don't worry about memory allocation from 
malicious VNC clients.

Regards,

Anthony Liguori

> But I think the balloon case is different, because we're not fighting
> malicious guests/clients, it's really the balloon operation that can
> cause the flood.
>
>
Luiz Capitulino - May 18, 2012, 1:09 p.m.
On Thu, 17 May 2012 16:20:42 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> >> Hmm, that's a good point, but my concern was that if we only emit
> >> the event when the target is reached, what happens if the guest
> >> gets very close to the target but never actually reaches it for
> >> some reason.
> >
> > Having a way to detect the last balloon change would be perfect.
> 
> libvirt certainly would have to maintain a timeout and make a decision on what 
> to do if the guest doesn't balloon to target.  Not sure how having events help 
> at all here.

I meant that if there's a way to detect the last balloon change, then
that's the time we could emit BALLOON_CHANGE (vs. emitting only when the
target is reached). I don't think the timeout is a bad idea, though.

> >> Should we perhaps just rate limit it to once per second ?
> >>
> >> BTW, if we're considering guest initiated events to be a potential
> >> DOS in this way, then I should point out the RTC_CHANGE event
> >> will already suffer this way, if a malicious guest continually
> >> adjusts its hardware close. So we might want to apply rate limiting
> >> to that event too ?
> >
> > I think several events can suffer from that. For example, a VNC
> > client could repeatedly connect&  disconnect from QEMU. If we're going
> > to fix this, then we'd need a general solution for it.
> 
> No, VNC clients are a whole different ballgame.  VNC connections will only 
> happen from the management network, we don't worry about memory allocation from 
> malicious VNC clients.

That's true, but as far as an event floods are concerned, I'm not completely
sure we should trust the management network.

But that was only an example, I think that the SUSPENDED event could also
be used by a malicious guests the way Daniel describes above.
Amit Shah - May 21, 2012, 11:14 a.m.
On (Thu) 17 May 2012 [08:49:44], Daniel P. Berrange wrote:
> On Wed, May 16, 2012 at 01:58:34PM -0500, Anthony Liguori wrote:
> > On 05/16/2012 01:42 PM, Luiz Capitulino wrote:
> > >On Wed, 16 May 2012 11:10:47 +0100
> > >"Daniel P. Berrange"<berrange@redhat.com>  wrote:
> > >
> > >>From: "Daniel P. Berrange"<berrange@redhat.com>
> > >>
> > >>After setting a balloon target value, applications have to
> > >>continually poll 'query-balloon' to determine whether the
> > >>guest has reacted to this request. The virtio-balloon backend
> > >>knows exactly when the guest has reacted though, and thus it
> > >>is possible to emit a JSON event to tell the mgmt application
> > >>whenever the guest balloon changes.
> > >>
> > >>This introduces a new 'qemu_balloon_change()' API which is
> > >>to be called by balloon driver backends, whenever they have
> > >>a change in balloon value. This takes the 'actual' balloon
> > >>value, as would be found in the BalloonInfo struct.
> > >>
> > >>The qemu_balloon_change API emits a JSON monitor event which
> > >>looks like:
> > >>
> > >>   {"timestamp": {"seconds": 1337162462, "microseconds": 814521},
> > >>    "event": "BALLOON_CHANGE", "data": {"actual": 944766976}}
> > >
> > >It's missing an entry in QMP/qmp-events.txt and I have a comment below,
> > >but in general looks good.
> > >
> > >Amit, would be good to get your ack.
> > 
> > I think it would be safer to limit this event to (1) only firing
> > once target has been reached (2) firing if target is deviated from
> > without a corresponding change in target.
> > 
> > Otherwise, a guest could just flood libvirt with events.  This would
> > queue memory in QEMU indefinitely as the events got queued up to
> > potentially serving as a DoS against other guests.
> 
> Hmm, that's a good point, but my concern was that if we only emit
> the event when the target is reached, what happens if the guest
> gets very close to the target but never actually reaches it for
> some reason.
> 
> Should we perhaps just rate limit it to once per second ?

This also has a slight disadvantage of missing to send out a
notification if it was received within the 1-second window, but no
further events come from the guest.

Overall, the qmp emitter itself could gain a new API to rate-limit
events, if so configured.

		Amit
Daniel P. Berrange - May 21, 2012, 11:29 a.m.
On Mon, May 21, 2012 at 04:44:38PM +0530, Amit Shah wrote:
> On (Thu) 17 May 2012 [08:49:44], Daniel P. Berrange wrote:
> > On Wed, May 16, 2012 at 01:58:34PM -0500, Anthony Liguori wrote:
> > > On 05/16/2012 01:42 PM, Luiz Capitulino wrote:
> > > >On Wed, 16 May 2012 11:10:47 +0100
> > > >"Daniel P. Berrange"<berrange@redhat.com>  wrote:
> > > >
> > > >>From: "Daniel P. Berrange"<berrange@redhat.com>
> > > >>
> > > >>After setting a balloon target value, applications have to
> > > >>continually poll 'query-balloon' to determine whether the
> > > >>guest has reacted to this request. The virtio-balloon backend
> > > >>knows exactly when the guest has reacted though, and thus it
> > > >>is possible to emit a JSON event to tell the mgmt application
> > > >>whenever the guest balloon changes.
> > > >>
> > > >>This introduces a new 'qemu_balloon_change()' API which is
> > > >>to be called by balloon driver backends, whenever they have
> > > >>a change in balloon value. This takes the 'actual' balloon
> > > >>value, as would be found in the BalloonInfo struct.
> > > >>
> > > >>The qemu_balloon_change API emits a JSON monitor event which
> > > >>looks like:
> > > >>
> > > >>   {"timestamp": {"seconds": 1337162462, "microseconds": 814521},
> > > >>    "event": "BALLOON_CHANGE", "data": {"actual": 944766976}}
> > > >
> > > >It's missing an entry in QMP/qmp-events.txt and I have a comment below,
> > > >but in general looks good.
> > > >
> > > >Amit, would be good to get your ack.
> > > 
> > > I think it would be safer to limit this event to (1) only firing
> > > once target has been reached (2) firing if target is deviated from
> > > without a corresponding change in target.
> > > 
> > > Otherwise, a guest could just flood libvirt with events.  This would
> > > queue memory in QEMU indefinitely as the events got queued up to
> > > potentially serving as a DoS against other guests.
> > 
> > Hmm, that's a good point, but my concern was that if we only emit
> > the event when the target is reached, what happens if the guest
> > gets very close to the target but never actually reaches it for
> > some reason.
> > 
> > Should we perhaps just rate limit it to once per second ?
> 
> This also has a slight disadvantage of missing to send out a
> notification if it was received within the 1-second window, but no
> further events come from the guest.

No, it would not (and indeed must not) miss any events. In the worst
case it would delay delivery of an event by upto 1 second, if an
identical event had already been sent within the last second.

eg it would work as follows

 1. First event arrives at time 250ms. Send it immediately

 2. Second event arrives at time 300ms. Already had an event 50ms
    ago. Do not send this event. Set 1 second timer.

 3. Third event arrives at time 400ms. A timer is pending. Do not
    send this event. Discard 2nd event.

 4. Fourth event arrives at time 500ms. A timer is pending. Do not
    send this event. Discard 3rd event.

 5. Timer fires at time 1300ms. Send fourth event

If the 5th event arrives before time 2300ms, we repeat the timer
delay process, otherwise we can send it immediately.

So after every 1 second window we can *guarantee* that the app
has received the most recent event from that time window.

> Overall, the qmp emitter itself could gain a new API to rate-limit
> events, if so configured.

Not all event types can be automatically rate-limited. Throttling
the RTC CHANGE or BALLOON CHANGE events is fine, because an appliction
only ever cares about the current value. Throttling SPICE CONNECT /
DISCONNECT events can cause trouble because there is stateful info
associated with each event that mustbe tracked by the application.


Daniel
Amit Shah - May 21, 2012, 12:16 p.m.
On (Mon) 21 May 2012 [12:29:52], Daniel P. Berrange wrote:
> On Mon, May 21, 2012 at 04:44:38PM +0530, Amit Shah wrote:
> > On (Thu) 17 May 2012 [08:49:44], Daniel P. Berrange wrote:
> > > On Wed, May 16, 2012 at 01:58:34PM -0500, Anthony Liguori wrote:
> > > > On 05/16/2012 01:42 PM, Luiz Capitulino wrote:
> > > > >On Wed, 16 May 2012 11:10:47 +0100
> > > > >"Daniel P. Berrange"<berrange@redhat.com>  wrote:
> > > > >
> > > > >>From: "Daniel P. Berrange"<berrange@redhat.com>
> > > > >>
> > > > >>After setting a balloon target value, applications have to
> > > > >>continually poll 'query-balloon' to determine whether the
> > > > >>guest has reacted to this request. The virtio-balloon backend
> > > > >>knows exactly when the guest has reacted though, and thus it
> > > > >>is possible to emit a JSON event to tell the mgmt application
> > > > >>whenever the guest balloon changes.
> > > > >>
> > > > >>This introduces a new 'qemu_balloon_change()' API which is
> > > > >>to be called by balloon driver backends, whenever they have
> > > > >>a change in balloon value. This takes the 'actual' balloon
> > > > >>value, as would be found in the BalloonInfo struct.
> > > > >>
> > > > >>The qemu_balloon_change API emits a JSON monitor event which
> > > > >>looks like:
> > > > >>
> > > > >>   {"timestamp": {"seconds": 1337162462, "microseconds": 814521},
> > > > >>    "event": "BALLOON_CHANGE", "data": {"actual": 944766976}}
> > > > >
> > > > >It's missing an entry in QMP/qmp-events.txt and I have a comment below,
> > > > >but in general looks good.
> > > > >
> > > > >Amit, would be good to get your ack.
> > > > 
> > > > I think it would be safer to limit this event to (1) only firing
> > > > once target has been reached (2) firing if target is deviated from
> > > > without a corresponding change in target.
> > > > 
> > > > Otherwise, a guest could just flood libvirt with events.  This would
> > > > queue memory in QEMU indefinitely as the events got queued up to
> > > > potentially serving as a DoS against other guests.
> > > 
> > > Hmm, that's a good point, but my concern was that if we only emit
> > > the event when the target is reached, what happens if the guest
> > > gets very close to the target but never actually reaches it for
> > > some reason.
> > > 
> > > Should we perhaps just rate limit it to once per second ?
> > 
> > This also has a slight disadvantage of missing to send out a
> > notification if it was received within the 1-second window, but no
> > further events come from the guest.
> 
> No, it would not (and indeed must not) miss any events. In the worst
> case it would delay delivery of an event by upto 1 second, if an
> identical event had already been sent within the last second.
> 
> eg it would work as follows
> 
>  1. First event arrives at time 250ms. Send it immediately
> 
>  2. Second event arrives at time 300ms. Already had an event 50ms
>     ago. Do not send this event. Set 1 second timer.
> 
>  3. Third event arrives at time 400ms. A timer is pending. Do not
>     send this event. Discard 2nd event.
> 
>  4. Fourth event arrives at time 500ms. A timer is pending. Do not
>     send this event. Discard 3rd event.
> 
>  5. Timer fires at time 1300ms. Send fourth event
> 
> If the 5th event arrives before time 2300ms, we repeat the timer
> delay process, otherwise we can send it immediately.
> 
> So after every 1 second window we can *guarantee* that the app
> has received the most recent event from that time window.

(Yes, I just meant to note that such care should be taken.)

> > Overall, the qmp emitter itself could gain a new API to rate-limit
> > events, if so configured.
> 
> Not all event types can be automatically rate-limited.

Of course, I mentioned we need a new API for that.

		Amit

Patch

diff --git a/balloon.c b/balloon.c
index aa354f7..913862b 100644
--- a/balloon.c
+++ b/balloon.c
@@ -30,6 +30,7 @@ 
 #include "balloon.h"
 #include "trace.h"
 #include "qmp-commands.h"
+#include "qjson.h"
 
 static QEMUBalloonEvent *balloon_event_fn;
 static QEMUBalloonStatus *balloon_stat_fn;
@@ -80,6 +81,19 @@  static int qemu_balloon_status(BalloonInfo *info)
     return 1;
 }
 
+void qemu_balloon_change(int64_t actual)
+{
+    QObject *data;
+
+    data = qobject_from_jsonf("{ 'actual': %" PRId64 " }",
+                              actual);
+
+    monitor_protocol_event(QEVENT_BALLOON_CHANGE, data);
+
+    qobject_decref(data);
+}
+
+
 BalloonInfo *qmp_query_balloon(Error **errp)
 {
     BalloonInfo *info;
diff --git a/balloon.h b/balloon.h
index b60fd5d..2ebac0d 100644
--- a/balloon.h
+++ b/balloon.h
@@ -24,4 +24,6 @@  int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
 			     QEMUBalloonStatus *stat_func, void *opaque);
 void qemu_remove_balloon_handler(void *opaque);
 
+void qemu_balloon_change(int64_t actual);
+
 #endif
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index ce9d2c9..9137573 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -146,8 +146,13 @@  static void virtio_balloon_set_config(VirtIODevice *vdev,
 {
     VirtIOBalloon *dev = to_virtio_balloon(vdev);
     struct virtio_balloon_config config;
+    uint32_t oldactual = dev->actual;
     memcpy(&config, config_data, 8);
     dev->actual = le32_to_cpu(config.actual);
+    if (dev->actual != oldactual) {
+        qemu_balloon_change(ram_size -
+                            (dev->actual << VIRTIO_BALLOON_PFN_SHIFT));
+    }
 }
 
 static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
diff --git a/monitor.c b/monitor.c
index 12a6fe2..ef59cd9 100644
--- a/monitor.c
+++ b/monitor.c
@@ -493,6 +493,9 @@  void monitor_protocol_event(MonitorEvent event, QObject *data)
         case QEVENT_WAKEUP:
             event_name = "WAKEUP";
             break;
+        case QEVENT_BALLOON_CHANGE:
+            event_name = "BALLOON_CHANGE";
+            break;
         default:
             abort();
             break;
diff --git a/monitor.h b/monitor.h
index 0d49800..8de0160 100644
--- a/monitor.h
+++ b/monitor.h
@@ -41,6 +41,7 @@  typedef enum MonitorEvent {
     QEVENT_DEVICE_TRAY_MOVED,
     QEVENT_SUSPEND,
     QEVENT_WAKEUP,
+    QEVENT_BALLOON_CHANGE,
     QEVENT_MAX,
 } MonitorEvent;