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

[Laurent Vivier <lvivier@redhat.com>]

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).

Thanks,
Laurent