On Tue, Sep 27, 2016 at 5:03 PM, Stefan Hajnoczi <stefa...@redhat.com> wrote:
> On Tue, Sep 27, 2016 at 02:12:09PM +0200, Ladi Prosek wrote:
>> On Tue, Sep 27, 2016 at 12:08 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote:
>> > On Tue, Sep 27, 2016 at 09:32:40AM +0200, Ladi Prosek wrote:
>> >> On Mon, Sep 19, 2016 at 3:28 PM, Stefan Hajnoczi <stefa...@redhat.com>
>> >> wrote:
>> >> > During device res> > +/* virtqueue_discard:
>> >> > + * @vq: The #VirtQueue
>> >> > + * @elem: The #VirtQueueElement
>> >> > + * @len: number of bytes written
>> >> > + *
>> >> > + * Pretend the most recent element wasn't popped from the virtqueue.
>> >> > The next
>> >> > + * call to virtqueue_pop() will refetch the element.
>> >> > + */
>> >> > void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
>> >> > unsigned int len)
>> >> > {
>> >> > vq->last_avail_idx--;
>> >> > - vq->inuse--;
>> >> > - virtqueue_unmap_sg(vq, elem, len);
>> >> > + virtqueue_detach_element(vq, elem, len);
>> >>
>> >> Random comment, not directly related to this change. Would it be worth
>> >> adding an assert to this function that elem->index and
>> >> vq->last_avail_idx match? In other words, enforce the "most recent"
>> >> qualifier mentioned in the comment. As more virtqueue_* functions are
>> >> added and the complexity goes up, it is easy to get confused. Also, I
>> >> think that naming this function virtqueue_unpop instead of
>> >> virtqueue_discard would help.
>> >
>> > elem->index is a descriptor ring index. vq->last_avail_idx is an index
>> > into the available ring. They are different but your suggestion makes
>> > sense in general.
>>
>> Oh, right, I didn't mean they would be identical but something like this:
>>
>> g_assert(elem->index == virtqueue_get_head(vq, vq->last_avail_idx));
>>
>> > We shouldn't read from vring memory again for an assertion so
>> > deferencing the available ring isn't possible (because we cannot rely on
>> > vring memory contents after processing the request).
>>
>> Not sure I follow, shouldn't available ring memory at that index still
>> be the same? Basically I'd like to assert that the next virtqueue_pop
>> would return the same element.
>
> Assertions cannot be guest-triggerable. The guest can make the
> assertion fail by writing a new value to the available ring.
>
> That might not sound like an issue but consider a scenario where the
> virtio PCI device is passed through to a nested guest. Now the nested
> guest can kill the parent hypervisor and all sibling VMs.
Got it, all clear now, thanks!
> Stefan
