From patchwork Tue Jan 16 17:51:52 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Michael S. Tsirkin" X-Patchwork-Id: 861827 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=nongnu.org (client-ip=2001:4830:134:3::11; helo=lists.gnu.org; envelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org; receiver=) Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3zLdNZ01c8z9t32 for ; Wed, 17 Jan 2018 05:02:37 +1100 (AEDT) Received: from localhost ([::1]:52378 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ebVZE-0001l2-2t for incoming@patchwork.ozlabs.org; Tue, 16 Jan 2018 13:02:36 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45811) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ebVP2-0000ew-Oc for qemu-devel@nongnu.org; Tue, 16 Jan 2018 12:52:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ebVP1-0003Fv-JP for qemu-devel@nongnu.org; Tue, 16 Jan 2018 12:52:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59954) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ebVP1-0003Dy-9w for qemu-devel@nongnu.org; Tue, 16 Jan 2018 12:52:03 -0500 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 594BE52121; Tue, 16 Jan 2018 17:51:57 +0000 (UTC) Received: from redhat.com (ovpn-125-221.rdu2.redhat.com [10.10.125.221]) by smtp.corp.redhat.com (Postfix) with SMTP id 4BA8A6F991; Tue, 16 Jan 2018 17:51:53 +0000 (UTC) Date: Tue, 16 Jan 2018 19:51:52 +0200 From: "Michael S. Tsirkin" To: qemu-devel@nongnu.org Message-ID: <1516121887-32738-22-git-send-email-mst@redhat.com> References: <1516121887-32738-1-git-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1516121887-32738-1-git-send-email-mst@redhat.com> X-Mutt-Fcc: =sent X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Tue, 16 Jan 2018 17:52:02 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PULL v2 21/34] vhost: Build temporary section list and deref after commit X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Peter Maydell , "Dr. David Alan Gilbert" , Igor Mammedov Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" From: "Dr. David Alan Gilbert" Igor spotted that there's a race, where a region that's unref'd in a _del callback might be free'd before the set_mem_table call in the _commit callback, and thus the vhost might end up using free memory. Fix this by building a complete temporary sections list, ref'ing every section (during add and nop) and then unref'ing the whole list right at the end of commit. Signed-off-by: Dr. David Alan Gilbert Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Igor Mammedov --- include/hw/virtio/vhost.h | 2 ++ hw/virtio/vhost.c | 73 ++++++++++++++++++++++++++++++----------------- 2 files changed, 49 insertions(+), 26 deletions(-) diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h index 1dc2d73..09854b6 100644 --- a/include/hw/virtio/vhost.h +++ b/include/hw/virtio/vhost.h @@ -60,6 +60,8 @@ struct vhost_dev { struct vhost_memory *mem; int n_mem_sections; MemoryRegionSection *mem_sections; + int n_tmp_sections; + MemoryRegionSection *tmp_sections; struct vhost_virtqueue *vqs; int nvqs; /* the first virtqueue which would be used by this vhost dev */ diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 386aef8..8a85dde 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -621,6 +621,8 @@ static void vhost_begin(MemoryListener *listener) memory_listener); dev->mem_changed_end_addr = 0; dev->mem_changed_start_addr = -1; + dev->tmp_sections = NULL; + dev->n_tmp_sections = 0; } static void vhost_commit(MemoryListener *listener) @@ -629,17 +631,25 @@ static void vhost_commit(MemoryListener *listener) memory_listener); hwaddr start_addr = 0; ram_addr_t size = 0; + MemoryRegionSection *old_sections; + int n_old_sections; + uint64_t log_size; int r; + old_sections = dev->mem_sections; + n_old_sections = dev->n_mem_sections; + dev->mem_sections = dev->tmp_sections; + dev->n_mem_sections = dev->n_tmp_sections; + if (!dev->memory_changed) { - return; + goto out; } if (!dev->started) { - return; + goto out; } if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) { - return; + goto out; } if (dev->started) { @@ -656,7 +666,7 @@ static void vhost_commit(MemoryListener *listener) VHOST_OPS_DEBUG("vhost_set_mem_table failed"); } dev->memory_changed = false; - return; + goto out; } log_size = vhost_get_log_size(dev); /* We allocate an extra 4K bytes to log, @@ -675,6 +685,27 @@ static void vhost_commit(MemoryListener *listener) vhost_dev_log_resize(dev, log_size); } dev->memory_changed = false; + +out: + /* Deref the old list of sections, this must happen _after_ the + * vhost_set_mem_table to ensure the client isn't still using the + * section we're about to unref. + */ + while (n_old_sections--) { + memory_region_unref(old_sections[n_old_sections].mr); + } + g_free(old_sections); + return; +} + +static void vhost_add_section(struct vhost_dev *dev, + MemoryRegionSection *section) +{ + ++dev->n_tmp_sections; + dev->tmp_sections = g_renew(MemoryRegionSection, dev->tmp_sections, + dev->n_tmp_sections); + dev->tmp_sections[dev->n_tmp_sections - 1] = *section; + memory_region_ref(section->mr); } static void vhost_region_add(MemoryListener *listener, @@ -687,36 +718,31 @@ static void vhost_region_add(MemoryListener *listener, return; } - ++dev->n_mem_sections; - dev->mem_sections = g_renew(MemoryRegionSection, dev->mem_sections, - dev->n_mem_sections); - dev->mem_sections[dev->n_mem_sections - 1] = *section; - memory_region_ref(section->mr); + vhost_add_section(dev, section); vhost_set_memory(listener, section, true); } -static void vhost_region_del(MemoryListener *listener, +static void vhost_region_nop(MemoryListener *listener, MemoryRegionSection *section) { struct vhost_dev *dev = container_of(listener, struct vhost_dev, memory_listener); - int i; if (!vhost_section(section)) { return; } - vhost_set_memory(listener, section, false); - memory_region_unref(section->mr); - for (i = 0; i < dev->n_mem_sections; ++i) { - if (dev->mem_sections[i].offset_within_address_space - == section->offset_within_address_space) { - --dev->n_mem_sections; - memmove(&dev->mem_sections[i], &dev->mem_sections[i+1], - (dev->n_mem_sections - i) * sizeof(*dev->mem_sections)); - break; - } + vhost_add_section(dev, section); +} + +static void vhost_region_del(MemoryListener *listener, + MemoryRegionSection *section) +{ + if (!vhost_section(section)) { + return; } + + vhost_set_memory(listener, section, false); } static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) @@ -783,11 +809,6 @@ static void vhost_iommu_region_del(MemoryListener *listener, } } -static void vhost_region_nop(MemoryListener *listener, - MemoryRegionSection *section) -{ -} - static int vhost_virtqueue_set_addr(struct vhost_dev *dev, struct vhost_virtqueue *vq, unsigned idx, bool enable_log)