[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-stable] [PATCH 4/6] balloon: fix segfault and harden the stats
From: |
Ladi Prosek |
Subject: |
Re: [Qemu-stable] [PATCH 4/6] balloon: fix segfault and harden the stats queue |
Date: |
Mon, 2 Jan 2017 13:02:07 +0100 |
Please ignore, git-publish gone wrong.
On Mon, Jan 2, 2017 at 12:58 PM, Ladi Prosek <address@hidden> wrote:
> The segfault here is triggered by the driver notifying the stats queue
> twice after adding a buffer to it. This effectively resets stats_vq_elem
> back to NULL and QEMU crashes on the next stats timer tick in
> balloon_stats_poll_cb.
>
> This is a regression introduced in 51b19ebe4320f3dc, although admittedly
> the device assumed too much about the stats queue protocol even before
> that commit. This commit adds a few more checks and ensures that the one
> stats buffer gets deallocated on device reset.
>
> Cc: address@hidden
> Signed-off-by: Ladi Prosek <address@hidden>
> Reviewed-by: Michael S. Tsirkin <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> (cherry picked from commit 4eae2a657d1ff5ada56eb9b4966eae0eff333b0b)
> Signed-off-by: Ladi Prosek <address@hidden>
>
> Conflicts:
> * 0.12.1.2 does not return pointers from virtqueue_pop so only the
> "harden the stats queue" part of the upstream patch description
> applies
> * a new field stats_vq_elem_pending is introduced to keep track
> of the state of stats_vq_elem in lieu of its nullness upstream
> * virtio_balloon_device_reset only resets stats_vq_elem_pending
> because there is nothing to free
> ---
> hw/virtio-balloon.c | 27 ++++++++++++++++++++++++---
> 1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 495a483..24e50af 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -44,6 +44,7 @@ typedef struct VirtIOBalloon
> uint32_t actual;
> uint64_t stats[VIRTIO_BALLOON_S_NR];
> VirtQueueElement stats_vq_elem;
> + bool stats_vq_elem_pending;
> size_t stats_vq_offset;
> MonitorCompletion *stats_callback;
> void *stats_opaque_callback_data;
> @@ -150,13 +151,21 @@ static void complete_stats_request(VirtIOBalloon *vb)
> static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
> {
> VirtIOBalloon *s = DO_UPCAST(VirtIOBalloon, vdev, vdev);
> - VirtQueueElement *elem = &s->stats_vq_elem;
> + VirtQueueElement elem;
> VirtIOBalloonStat stat;
> size_t offset = 0;
>
> - if (!virtqueue_pop(vq, elem)) {
> + if (!virtqueue_pop(vq, &elem)) {
> return;
> }
> + if (s->stats_vq_elem_pending) {
> + /* This should never happen if the driver follows the spec. */
> + virtqueue_push(vq, &s->stats_vq_elem, 0);
> + virtio_notify(vdev, vq);
> + }
> +
> + s->stats_vq_elem = elem;
> + s->stats_vq_elem_pending = true;
>
> /* Initialize the stats to get rid of any stale values. This is only
> * needed to handle the case where a guest supports fewer stats than it
> @@ -164,7 +173,7 @@ static void virtio_balloon_receive_stats(VirtIODevice
> *vdev, VirtQueue *vq)
> */
> reset_stats(s);
>
> - while (iov_to_buf(elem->out_sg, elem->out_num, &stat, offset,
> sizeof(stat))
> + while (iov_to_buf(elem.out_sg, elem.out_num, &stat, offset, sizeof(stat))
> == sizeof(stat)) {
> uint16_t tag = tswap16(stat.tag);
> uint64_t val = tswap64(stat.val);
> @@ -178,6 +187,15 @@ static void virtio_balloon_receive_stats(VirtIODevice
> *vdev, VirtQueue *vq)
> complete_stats_request(s);
> }
>
> +static void virtio_balloon_device_reset(VirtIODevice *vdev)
> +{
> + VirtIOBalloon *s = to_virtio_balloon(vdev);
> +
> + if (s->stats_vq_elem_pending) {
> + s->stats_vq_elem_pending = false;
> + }
> +}
> +
> static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t
> *config_data)
> {
> VirtIOBalloon *dev = to_virtio_balloon(vdev);
> @@ -224,8 +242,10 @@ static void virtio_balloon_stat(void *opaque,
> MonitorCompletion cb,
> dev->stats_opaque_callback_data = cb_data;
>
> if (ENABLE_GUEST_STATS
> + && dev->stats_vq_elem_pending
> && (dev->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ))) {
> virtqueue_push(dev->svq, &dev->stats_vq_elem, dev->stats_vq_offset);
> + dev->stats_vq_elem_pending = false;
> virtio_notify(&dev->vdev, dev->svq);
> return;
> }
> @@ -287,6 +307,7 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
> VIRTIO_ID_BALLOON,
> 8, sizeof(VirtIOBalloon));
>
> + s->vdev.reset = virtio_balloon_device_reset;
> s->vdev.get_config = virtio_balloon_get_config;
> s->vdev.set_config = virtio_balloon_set_config;
> s->vdev.get_features = virtio_balloon_get_features;
> --
> 2.7.4
>