On 03/09/2019 02:30 AM, Dr. David Alan Gilbert wrote:
* Peter Maydell (peter.mayd...@linaro.org) wrote:
On Wed, 6 Mar 2019 at 11:55, Dr. David Alan Gilbert (git)
<dgilb...@redhat.com> wrote:
From: Wei Wang <wei.w.w...@intel.com>
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 <wei.w.w...@intel.com>
CC: Michael S. Tsirkin <m...@redhat.com>
CC: Dr. David Alan Gilbert <dgilb...@redhat.com>
CC: Juan Quintela <quint...@redhat.com>
CC: Peter Xu <pet...@redhat.com>
Message-Id: <1544516693-5395-8-git-send-email-wei.w.w...@intel.com>
Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
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