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/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? 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?