[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL v2 36/58] Add helper to populate vhost-user message regions
From: |
Peter Maydell |
Subject: |
Re: [PULL v2 36/58] Add helper to populate vhost-user message regions |
Date: |
Fri, 19 Jun 2020 13:59:11 +0100 |
On Fri, 12 Jun 2020 at 15:52, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Raphael Norwitz <raphael.norwitz@nutanix.com>
>
> When setting vhost-user memory tables, memory region descriptors must be
> copied from the vhost_dev struct to the vhost-user message. To avoid
> duplicating code in setting the memory tables, we should use a helper to
> populate this field. This change adds this helper.
>
> Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> Message-Id: <1588533678-23450-2-git-send-email-raphael.norwitz@nutanix.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Hi; Coverity reports a problem with this patch (CID 1429804):
> ---
> hw/virtio/vhost-user.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index ec21e8fbe8..2e0552dd74 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -407,6 +407,15 @@ static int vhost_user_set_log_base(struct vhost_dev
> *dev, uint64_t base,
> return 0;
> }
>
> +static void vhost_user_fill_msg_region(VhostUserMemoryRegion *dst,
> + struct vhost_memory_region *src)
> +{
> + assert(src != NULL && dst != NULL);
> + dst->userspace_addr = src->userspace_addr;
> + dst->memory_size = src->memory_size;
> + dst->guest_phys_addr = src->guest_phys_addr;
This function only initializes 3 of the 4 fields of
the VhostUserMemoryRegion struct...
> +}
> +
> static int vhost_user_fill_set_mem_table_msg(struct vhost_user *u,
> struct vhost_dev *dev,
> VhostUserMsg *msg,
> @@ -417,6 +426,7 @@ static int vhost_user_fill_set_mem_table_msg(struct
> vhost_user *u,
> ram_addr_t offset;
> MemoryRegion *mr;
> struct vhost_memory_region *reg;
> + VhostUserMemoryRegion region_buffer;
...this variable starts uninitialized...
>
> msg->hdr.request = VHOST_USER_SET_MEM_TABLE;
>
> @@ -441,12 +451,8 @@ static int vhost_user_fill_set_mem_table_msg(struct
> vhost_user *u,
> error_report("Failed preparing vhost-user memory table msg");
> return -1;
> }
> - msg->payload.memory.regions[*fd_num].userspace_addr =
> - reg->userspace_addr;
> - msg->payload.memory.regions[*fd_num].memory_size =
> - reg->memory_size;
> - msg->payload.memory.regions[*fd_num].guest_phys_addr =
> - reg->guest_phys_addr;
> + vhost_user_fill_msg_region(®ion_buffer, reg);
> + msg->payload.memory.regions[*fd_num] = region_buffer;
...so this struct copy is copying uninitialized data...
> msg->payload.memory.regions[*fd_num].mmap_offset = offset;
...which coverity complains about even though it happens that the
following line fills in that field in the target of the struct copy.
> fds[(*fd_num)++] = fd;
> } else if (track_ramblocks) {
Coverity also complains about both of the other places that call
this function for similar reasons. My suggested fix: make
vhost_user_fill_msg_region() take an extra argument "uint64_t mmap_offset",
which it uses to initialize the dst->mmap_offset. Then you can
pass in "offset" at this callsite and delete the manual initialization
of .mmap_offset; and similarly for the other two callsites.
thanks
-- PMM
- [PULL v2 23/58] virtio-balloon: fix free page hinting check on unrealize, (continued)
- [PULL v2 23/58] virtio-balloon: fix free page hinting check on unrealize, Michael S. Tsirkin, 2020/06/12
- [PULL v2 27/58] MAINTAINERS: Fix the classification of bios-tables-test-allowed-diff.h, Michael S. Tsirkin, 2020/06/12
- [PULL v2 28/58] hw/pci/pcie: Move hot plug capability check to pre_plug callback, Michael S. Tsirkin, 2020/06/12
- [PULL v2 29/58] pci: assert configuration access is within bounds, Michael S. Tsirkin, 2020/06/12
- [PULL v2 26/58] virtio-balloon: Provide an interface for free page reporting, Michael S. Tsirkin, 2020/06/12
- [PULL v2 33/58] hw/pci-host: Use the IEC binary prefix definitions, Michael S. Tsirkin, 2020/06/12
- [PULL v2 34/58] char-socket: return -1 in case of disconnect during tcp_chr_write, Michael S. Tsirkin, 2020/06/12
- [PULL v2 35/58] vhost-user-blk: delay vhost_user_blk_disconnect, Michael S. Tsirkin, 2020/06/12
- [PULL v2 37/58] Add vhost-user helper to get MemoryRegion data, Michael S. Tsirkin, 2020/06/12
- [PULL v2 36/58] Add helper to populate vhost-user message regions, Michael S. Tsirkin, 2020/06/12
- Re: [PULL v2 36/58] Add helper to populate vhost-user message regions,
Peter Maydell <=
- [PULL v2 38/58] Add VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS, Michael S. Tsirkin, 2020/06/12
- [PULL v2 39/58] Transmit vhost-user memory regions individually, Michael S. Tsirkin, 2020/06/12
- [PULL v2 40/58] Lift max memory slots limit imposed by vhost-user, Michael S. Tsirkin, 2020/06/12
- [PULL v2 41/58] Refactor out libvhost-user fault generation logic, Michael S. Tsirkin, 2020/06/12
- [PULL v2 42/58] Support ram slot configuration in libvhost-user, Michael S. Tsirkin, 2020/06/12
- [PULL v2 43/58] Support adding individual regions in libvhost-user, Michael S. Tsirkin, 2020/06/12
- [PULL v2 44/58] Support individual region unmap in libvhost-user, Michael S. Tsirkin, 2020/06/12
- [PULL v2 45/58] Lift max ram slots limit in libvhost-user, Michael S. Tsirkin, 2020/06/12