Re: [Qemu-devel] [2.2 PATCH V2 for-4.5] virtio-net: fix unmap leak

[Stefano Stabellini]

On Thu, 27 Nov 2014, Konrad Rzeszutek Wilk wrote:
> On Nov 27, 2014 9:58 AM, Stefano Stabellini <address@hidden> wrote:
> >
> > On Thu, 27 Nov 2014, Konrad Rzeszutek Wilk wrote: 
> > > On Nov 27, 2014 7:46 AM, Stefano Stabellini <address@hidden> wrote: 
> > > > 
> > > > Konrad, I think we should have this fix in 4.5: without it 
> > > > vif=[ 'model=virtio-net' ] crashes QEMU. 
> > > > 
> > > 
> > > Is it an regression? 
> >
> > Good question: I was trying to investigate that. 
> >
> > virtio-net is currently *not* documented in the xl interface: 
> >
> >
> > ### model 
> >
> > This keyword is valid for HVM guest devices with `type=ioemu` only. 
> >
> > Specifies the type device to emulated for this guest. Valid values 
> > are: 
> >
> >   * `rtl8139` (default) -- Realtek RTL8139 
> >   * `e1000` -- Intel E1000 
> >   * in principle any device supported by your device model 
> >
> >
> > The last working version of virtio-net on Xen is QEMU v1.4.0. That means 
> > that the bug affects Xen 4.4 too (but it should work in Xen 4.3). 
> 
> Not a regression compared to 4.4 but it has been for two releases.

That is true. On the plus side, virtio-net has never been properly
documented as working in the first place.


> So if nobody noticed it for two releases will they notice it if it not fixed 
> in this release either? And can it be fixed in the next one?

We can fix the crash even in this release by backporting this rather
simple patch. However the patch would just avoid the crash: virtio-net
would still be not working once the guest is booted. I haven't figured
out the cause of that problem yet.


> > > > On Thu, 27 Nov 2014, Peter Maydell wrote: 
> > > > > On 27 November 2014 at 12:33, Michael S. Tsirkin <address@hidden> 
> > > > > wrote: 
> > > > > > On Thu, Nov 27, 2014 at 06:04:03PM +0800, Jason Wang wrote: 
> > > > > >> virtio_net_handle_ctrl() and other functions that process control 
> > > > > >> vq 
> > > > > >> request call iov_discard_front() which will shorten the iov. This 
> > > > > >> will 
> > > > > >> lead unmapping in virtqueue_push() leaks mapping. 
> > > > > >> 
> > > > > >> Fixes this by keeping the original iov untouched and using a temp 
> > > > > >> variable 
> > > > > >> in those functions. 
> > > > > >> 
> > > > > >> Cc: Wen Congyang <address@hidden> 
> > > > > >> Cc: Stefano Stabellini <address@hidden> 
> > > > > >> Cc: address@hidden 
> > > > > >> Signed-off-by: Jason Wang <address@hidden> 
> > > > > > 
> > > > > > Reviewed-by: Michael S. Tsirkin <address@hidden> 
> > > > > > 
> > > > > > Peter, can you pick this up or do you want a pull request? 
> > > > > 
> > > > > I can pick it up. I was waiting a bit to check that everybody 
> > > > > was happy that this is the correct way to fix the bug and the 
> > > > > patch is ok... 
> > > 
>