qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PULL 19/22] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE


From: Wei Wang
Subject: Re: [Qemu-devel] [PULL 19/22] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT
Date: Sat, 09 Mar 2019 19:22:07 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0

On 03/09/2019 02:30 AM, Dr. David Alan Gilbert wrote:
* Peter Maydell (address@hidden) wrote:
On Wed, 6 Mar 2019 at 11:55, Dr. David Alan Gilbert (git)
<address@hidden> wrote:
From: Wei Wang <address@hidden>

Wei: Can you look at this....

Sure, I'll send a followup patch to fix this.



The new feature enables the virtio-balloon device to receive hints of
guest free pages from the free page vq.

A notifier is registered to the migration precopy notifier chain. The
notifier calls free_page_start after the migration thread syncs the dirty
bitmap, so that the free page optimization starts to clear bits of free
pages from the bitmap. It calls the free_page_stop before the migration
thread syncs the bitmap, which is the end of the current round of ram
save. The free_page_stop is also called to stop the optimization in the
case when there is an error occurred in the process of ram saving.

Note: balloon will report pages which were free at the time of this call.
As the reporting happens asynchronously, dirty bit logging must be
enabled before this free_page_start call is made. Guest reporting must be
disabled before the migration dirty bitmap is synchronized.

Signed-off-by: Wei Wang <address@hidden>
CC: Michael S. Tsirkin <address@hidden>
CC: Dr. David Alan Gilbert <address@hidden>
CC: Juan Quintela <address@hidden>
CC: Peter Xu <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Michael S. Tsirkin <address@hidden>
Signed-off-by: Dr. David Alan Gilbert <address@hidden>
   dgilbert: Dropped kernel header update, fixed up CMD_ID_* name change
---
Hi -- Coverity points out a use-after-free here (CID 1399412):

+static bool get_free_page_hints(VirtIOBalloon *dev)
+{
+    VirtQueueElement *elem;
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VirtQueue *vq = dev->free_page_vq;
+
+    while (dev->block_iothread) {
+        qemu_cond_wait(&dev->free_page_cond, &dev->free_page_lock);
+    }
+
+    elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
+    if (!elem) {
+        return false;
+    }
+
+    if (elem->out_num) {
+        uint32_t id;
+        size_t size = iov_to_buf(elem->out_sg, elem->out_num, 0,
+                                 &id, sizeof(id));
+        virtqueue_push(vq, elem, size);
+        g_free(elem);
Here we free elem...

+
+        virtio_tswap32s(vdev, &id);
+        if (unlikely(size != sizeof(id))) {
+            virtio_error(vdev, "received an incorrect cmd id");
+            return false;
+        }
+        if (id == dev->free_page_report_cmd_id) {
+            dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
+        } else {
+            /*
+             * Stop the optimization only when it has started. This
+             * avoids a stale stop sign for the previous command.
+             */
+            if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
+                dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+            }
+        }
+    }
+
+    if (elem->in_num) {
...but we can fall through here and try to dereference elem...

+        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
+            qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
+                                      elem->in_sg[0].iov_len);
+        }
+        virtqueue_push(vq, elem, 1);
+        g_free(elem);
...and then free it again.
OK, so the question here is:
   Is it allowed to have both in and out in the same queue element; if
it's not then we need to error the device.
   If it is allowed then we need to fix up the out_num case.


I think it is allowed. From virtqueue_pop, an elem could consist of several inbufs and outbufs.

Probably we could just delay the free till the end of this function..will post the fix patch for review soon.

Best,
Wei



reply via email to

[Prev in Thread] Current Thread [Next in Thread]