Re: [Qemu-devel] [Xen-devel] virtio leaks cpu mappings, was: qemu crash with virtio on Xen domUs (backtrace included)

[Jason Wang <jasowang@redhat.com>]

On 11/26/2014 06:53 PM, Stefano Stabellini wrote:
> On Wed, 26 Nov 2014, Jason Wang wrote:
>> >On 11/25/2014 09:53 PM, Stefano Stabellini wrote:
>>> > >On Tue, 25 Nov 2014, Jason Wang wrote:
>>>> > >>On 11/25/2014 02:44 AM, Stefano Stabellini wrote:
>>>>> > >>>On Mon, 24 Nov 2014, Stefano Stabellini wrote:
>>>>>> > >>>>On Mon, 24 Nov 2014, Stefano Stabellini wrote:
>>>>>>> > >>>>>CC'ing Paolo.
>>>>>>> > >>>>>
>>>>>>> > >>>>>
>>>>>>> > >>>>>Wen,
>>>>>>> > >>>>>thanks for the logs.
>>>>>>> > >>>>>
>>>>>>> > >>>>>I investigated a little bit and it seems to me that the bug occurs when
>>>>>>> > >>>>>QEMU tries to unmap only a portion of a memory region previously mapped.
>>>>>>> > >>>>>That doesn't work with xen-mapcache.
>>>>>>> > >>>>>
>>>>>>> > >>>>>See these logs for example:
>>>>>>> > >>>>>
>>>>>>> > >>>>>DEBUG address_space_map phys_addr=78ed8b44 vaddr=7fab50afbb68 len=0xa
>>>>>>> > >>>>>DEBUG address_space_unmap vaddr=7fab50afbb68 len=0x6
>>>>>> > >>>>Sorry the logs don't quite match, it was supposed to be:
>>>>>> > >>>>
>>>>>> > >>>>DEBUG address_space_map phys_addr=78ed8b44 vaddr=7fab50afbb64 len=0xa
>>>>>> > >>>>DEBUG address_space_unmap vaddr=7fab50afbb68 len=0x6
>>>>> > >>>It looks like the problem is caused by iov_discard_front, called by
>>>>> > >>>virtio_net_handle_ctrl. By changing iov_base after the sg has already
>>>>> > >>>been mapped (cpu_physical_memory_map), it causes a leak in the mapping
>>>>> > >>>because the corresponding cpu_physical_memory_unmap will only unmap a
>>>>> > >>>portion of the original sg.  On Xen the problem is worse because
>>>>> > >>>xen-mapcache aborts.
>>>>> > >>>
>>>>> > >>>diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>> > >>>index 2ac6ce5..b2b5c2d 100644
>>>>> > >>>--- a/hw/net/virtio-net.c
>>>>> > >>>+++ b/hw/net/virtio-net.c
>>>>> > >>>@@ -775,7 +775,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>>>>> > >>>      struct iovec *iov;
>>>>> > >>>      unsigned int iov_cnt;
>>>>> > >>>
>>>>> > >>>-    while (virtqueue_pop(vq, &elem)) {
>>>>> > >>>+    while (virtqueue_pop_nomap(vq, &elem)) {
>>>>> > >>>          if (iov_size(elem.in_sg, elem.in_num) < sizeof(status) ||
>>>>> > >>>              iov_size(elem.out_sg, elem.out_num) < sizeof(ctrl)) {
>>>>> > >>>              error_report("virtio-net ctrl missing headers");
>>>>> > >>>@@ -784,8 +784,12 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>>>>> > >>>
>>>>> > >>>          iov = elem.out_sg;
>>>>> > >>>          iov_cnt = elem.out_num;
>>>>> > >>>-        s = iov_to_buf(iov, iov_cnt, 0, &ctrl, sizeof(ctrl));
>>>>> > >>>          iov_discard_front(&iov, &iov_cnt, sizeof(ctrl));
>>>>> > >>>+
>>>>> > >>>+        virtqueue_map_sg(elem.in_sg, elem.in_addr, elem.in_num, 1);
>>>>> > >>>+        virtqueue_map_sg(elem.out_sg, elem.out_addr, elem.out_num, 0);
>>>>> > >>>+
>>>>> > >>>+        s = iov_to_buf(iov, iov_cnt, 0, &ctrl, sizeof(ctrl));
>>>> > >>Does this really work?
>>> > >It seems to work here, as in it doesn't crash QEMU and I am able to boot
>>> > >a guest with network. I didn't try any MAC related commands.
>>> > >
>> >
>> >It was because the guest (not a recent kernel?) never issue commands
>> >through control vq.
>> >
>> >We'd better hide the implementation details such as virtqueue_map_sg()
>> >in virtio core instead of letting device call it directly.
>>>> > >>The code in fact skips the location that contains
>>>> > >>virtio_net_ctrl_hdr. And virtio_net_handle_mac() still calls
>>>> > >>iov_discard_front().
>>>> > >>
>>>> > >>How about copy iov to a temp variable and use it in this function?
>>> > >That would only work if I moved the cpu_physical_memory_unmap call
>>> > >outside of virtqueue_fill, so that we can pass different iov to them.
>>> > >We need to unmap the same iov that was previously mapped by
>>> > >virtqueue_pop.
>>> > >
>> >
>> >I mean something like following or just passing the offset of iov to
>> >virtio_net_handle_*().
> Sorry, you are right, your patch works too. I tried something like this
> yesterday but I was confused because even if a crash doesn't happen
> anymore, virtio-net still doesn't work on Xen (it boots but the network
> doesn't work properly within the guest).
> But that seems to be a separate issue and it affects my series too.
>
> A possible problem with this approach is that virtqueue_push is now
> called passing the original iov, not the shortened one.
>
> Are you sure that is OK?

It's ok, except for unmapping, virtqueue_push does not care iov at all.
> If so we can drop my series and use this instead.
>

I will submit a formal patch for this.

Thanks