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

[Stefano Brivio <sbrivio@redhat.com>]

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

> On 12/09/2024 15:36, Stefano Brivio wrote:
> > On Thu, 12 Sep 2024 13:23:58 +0200
> > Laurent Vivier <lvivier@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