Re: [PATCH v4 2/4] vhost-user: introduce virtio API

On Thu, 12 Sep 2024 16:03:46 +0200
Laurent Vivier <lvivier(a)redhat.com> wrote:

...
  On 12/09/2024 15:36, Stefano Brivio wrote:
  On Thu, 12 Sep 2024 13:23:58 +0200
 Laurent Vivier <lvivier(a)redhat.com> wrote:
   
  On 10/09/2024 17:47, Stefano Brivio wrote:  
 > +
> +/**
> + * virtqueue_read_indirect_desc() - Copy virtio ring descriptors from guest
> + *                                  memory
> + * @dev:	Vhost-user device
> + * @desc:	Destination address to copy the descriptors to
> + * @addr:	Guest memory address to copy from
> + * @len:	Length of memory to copy
> + *
> + * Return: -1 if there is an error, 0 otherwise
> + */
> +static int virtqueue_read_indirect_desc(struct vu_dev *dev, struct vring_desc
*desc,
> +					uint64_t addr, size_t len)
> +{
> +	uint64_t read_len;
> +
> +	if (len > (VIRTQUEUE_MAX_SIZE * sizeof(struct vring_desc)))
> +		return -1;
> +
> +	if (len == 0)
> +		return -1;
> +
> +	while (len) {
> +		const struct vring_desc *orig_desc;
> +
> +		read_len = len;
> +		orig_desc = vu_gpa_to_va(dev, &read_len, addr);  
 In case you missed this in my review of v3 (I'm not sure if it's a
 valid concern):

 --
 Should we also return if read_len < sizeof(struct vring_desc) after
 this call? Can that ever happen, if we pick a particular value of addr
 so that it's almost at the end of a region?
 --   
 In fact, read_len can be < sizeof(struct vring_desc) after this call but if orig_desc
!=
 NULL it means we can continue in another region to continue to fill the structure.  
 
 Right, I see that.
   
  If there is not enough memory to fill
"len" bytes it exits with -1.   
 
 ...and this as well. But let's say that read_len is 1 (and struct
 vring_desc is 16 bytes). Then:
 
 		memcpy(desc, orig_desc, read_len);
 
 copies one byte
 
 		[...]
 
 		desc += read_len / sizeof(struct vring_desc);
 
 doesn't increase desc.
 
 At the next iteration with len > 0 and read_len > 0, the memcpy() will
 overwrite that one byte, as we didn't increase desc. Or it's not
 possible for some other reason?
    
 
 We can add a check for that case, but my guess is memory region size is a multiple of
page 
 size (4k or 64k or ...) so I think we will always be able to read 16 bytes (if we loop on

 16 bytes reads). 
Ah, okay, it makes sense. I would still suggest to have an explicit
check, so that we don't crash if there's an issue in the hypervisor,
but it's not a strong preference.

-- 
Stefano