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

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

[Stefano Stabellini]

Konrad, I think we should have this fix in 4.5: without it
vif=[ 'model=virtio-net' ] crashes QEMU.


On Thu, 27 Nov 2014, Peter Maydell wrote:
> On 27 November 2014 at 12:33, Michael S. Tsirkin <mst@redhat.com> 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 <wency@cn.fujitsu.com>
> >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >> Cc: qemu-stable@nongnu.org
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > 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...

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

[Konrad Rzeszutek Wilk]

On Nov 27, 2014 7:46 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
>
> Konrad, I think we should have this fix in 4.5: without it 
> vif=[ 'model=virtio-net' ] crashes QEMU. 
>

Is it an regression?
>
> On Thu, 27 Nov 2014, Peter Maydell wrote: 
> > On 27 November 2014 at 12:33, Michael S. Tsirkin <mst@redhat.com> 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 <wency@cn.fujitsu.com> 
> > >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> 
> > >> Cc: qemu-stable@nongnu.org 
> > >> Signed-off-by: Jason Wang <jasowang@redhat.com> 
> > > 
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> 
> > > 
> > > 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... 

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 7:46 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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).


> > On Thu, 27 Nov 2014, Peter Maydell wrote: 
> > > On 27 November 2014 at 12:33, Michael S. Tsirkin <mst@redhat.com> 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 <wency@cn.fujitsu.com> 
> > > >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> 
> > > >> Cc: qemu-stable@nongnu.org 
> > > >> Signed-off-by: Jason Wang <jasowang@redhat.com> 
> > > > 
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> 
> > > > 
> > > > 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... 
> 

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

[Konrad Rzeszutek Wilk]

On Nov 27, 2014 9:58 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
>
> On Thu, 27 Nov 2014, Konrad Rzeszutek Wilk wrote: 
> > On Nov 27, 2014 7:46 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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.

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?


>
>
> > > On Thu, 27 Nov 2014, Peter Maydell wrote: 
> > > > On 27 November 2014 at 12:33, Michael S. Tsirkin <mst@redhat.com> 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 <wency@cn.fujitsu.com> 
> > > > >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> 
> > > > >> Cc: qemu-stable@nongnu.org 
> > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com> 
> > > > > 
> > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> 
> > > > > 
> > > > > 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... 
> > 

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2898 bytes --]

On Thu, 27 Nov 2014, Konrad Rzeszutek Wilk wrote:
> On Nov 27, 2014 9:58 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> >
> > On Thu, 27 Nov 2014, Konrad Rzeszutek Wilk wrote: 
> > > On Nov 27, 2014 7:46 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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 <mst@redhat.com> 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 <wency@cn.fujitsu.com> 
> > > > > >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> 
> > > > > >> Cc: qemu-stable@nongnu.org 
> > > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com> 
> > > > > > 
> > > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> 
> > > > > > 
> > > > > > 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... 
> > > 
> 

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

[Konrad Rzeszutek Wilk]

On Nov 27, 2014 10:26 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
>
> On Thu, 27 Nov 2014, Konrad Rzeszutek Wilk wrote: 
> > On Nov 27, 2014 9:58 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: 
> > > 
> > > On Thu, 27 Nov 2014, Konrad Rzeszutek Wilk wrote: 
> > > > On Nov 27, 2014 7:46 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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. 
>

Perhaps then the hack^H^Hfix is to return an error if user is using virtio-net then?

And then in later releases make it work right.

>
> > > > > On Thu, 27 Nov 2014, Peter Maydell wrote: 
> > > > > > On 27 November 2014 at 12:33, Michael S. Tsirkin <mst@redhat.com> 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 <wency@cn.fujitsu.com> 
> > > > > > >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> 
> > > > > > >> Cc: qemu-stable@nongnu.org 
> > > > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com> 
> > > > > > > 
> > > > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> 
> > > > > > > 
> > > > > > > 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... 
> > > > 
> >

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3711 bytes --]

On Thu, 27 Nov 2014, Konrad Rzeszutek Wilk wrote:
> On Nov 27, 2014 10:26 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> >
> > On Thu, 27 Nov 2014, Konrad Rzeszutek Wilk wrote: 
> > > On Nov 27, 2014 9:58 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: 
> > > > 
> > > > On Thu, 27 Nov 2014, Konrad Rzeszutek Wilk wrote: 
> > > > > On Nov 27, 2014 7:46 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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. 
> >
> 
> Perhaps then the hack^H^Hfix is to return an error if user is using virtio-net then?
> 
> And then in later releases make it work right.

Sorry, I take it back: the fix is enough to get virtio-net working, I
had a configuration error that was confusing my test results.

Given that the fix is very simple, I think we should backport it to Xen 4.5
and Xen 4.4.


> > > > > > On Thu, 27 Nov 2014, Peter Maydell wrote: 
> > > > > > > On 27 November 2014 at 12:33, Michael S. Tsirkin <mst@redhat.com> 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 <wency@cn.fujitsu.com> 
> > > > > > > >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> 
> > > > > > > >> Cc: qemu-stable@nongnu.org 
> > > > > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com> 
> > > > > > > > 
> > > > > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> 
> > > > > > > > 
> > > > > > > > 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... 
> > > > > 
> > >

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

[Konrad Rzeszutek Wilk]

On Thu, Nov 27, 2014 at 05:46:28PM +0000, Stefano Stabellini wrote:
> On Thu, 27 Nov 2014, Konrad Rzeszutek Wilk wrote:
> > On Nov 27, 2014 10:26 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > >
> > > On Thu, 27 Nov 2014, Konrad Rzeszutek Wilk wrote: 
> > > > On Nov 27, 2014 9:58 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: 
> > > > > 
> > > > > On Thu, 27 Nov 2014, Konrad Rzeszutek Wilk wrote: 
> > > > > > On Nov 27, 2014 7:46 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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. 
> > >
> > 
> > Perhaps then the hack^H^Hfix is to return an error if user is using virtio-net then?
> > 
> > And then in later releases make it work right.
> 
> Sorry, I take it back: the fix is enough to get virtio-net working, I
> had a configuration error that was confusing my test results.
> 
> Given that the fix is very simple, I think we should backport it to Xen 4.5
> and Xen 4.4.

OK.
>From 4.5 perspective: Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> 
> > > > > > > On Thu, 27 Nov 2014, Peter Maydell wrote: 
> > > > > > > > On 27 November 2014 at 12:33, Michael S. Tsirkin <mst@redhat.com> 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 <wency@cn.fujitsu.com> 
> > > > > > > > >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> 
> > > > > > > > >> Cc: qemu-stable@nongnu.org 
> > > > > > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com> 
> > > > > > > > > 
> > > > > > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> 
> > > > > > > > > 
> > > > > > > > > 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... 
> > > > > > 
> > > >