Commit c44317ef authored by Dr. David Alan Gilbert's avatar Dr. David Alan Gilbert Committed by Michael S. Tsirkin
Browse files

vhost: Build temporary section list and deref after commit 



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: default avatarDr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: default avatarIgor Mammedov <imammedo@redhat.com>
Reviewed-by: default avatarMichael S. Tsirkin <mst@redhat.com>
Signed-off-by: default avatarMichael S. Tsirkin <mst@redhat.com>
parent 710fccf8

hw/virtio/vhost.c

+47 −26
Original line number Diff line number Diff line
@@ -627,6 +627,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)
@@ -635,17 +637,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) {
@@ -662,7 +672,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,
@@ -681,6 +691,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,
@@ -693,36 +724,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)
@@ -789,11 +815,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)

include/hw/virtio/vhost.h

+2 −0
Original line number Diff line number Diff line
@@ -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 */