[Qemu-devel] [PATCH] vhost-net: Do not set features for backend when peer was deleted

[Qemu-devel] [PATCH] vhost-net: Do not set features for backend when peer was deleted

[Jason Wang]

When hot-unplug a virtio nic with vhost-net backend, guest may
continue to program the nic even if its peer have been deleted. We can
not set features at this time as vhost_net_ack_features() may still
try to use the tap related vhost_net structure which have been freed
in tap_cleanup(). And setting offload features for a deleted backend
is also meaningless in this situation

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio-net.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 7e1688c..68c8e48 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -245,6 +245,9 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
 {
     VirtIONet *n = to_virtio_net(vdev);
 
+    if (n->nic->peer_deleted)
+        return;
+
     n->mergeable_rx_bufs = !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF));
 
     if (n->has_vnet_hdr) {

[Qemu-devel] Re: [PATCH] vhost-net: Do not set features for backend when peer was deleted

[Michael S. Tsirkin]

On Wed, Oct 27, 2010 at 12:43:51AM +0800, Jason Wang wrote:
> When hot-unplug a virtio nic with vhost-net backend, guest may
> continue to program the nic even if its peer have been deleted. We can
> not set features at this time as vhost_net_ack_features() may still
> try to use the tap related vhost_net structure which have been freed
> in tap_cleanup(). And setting offload features for a deleted backend
> is also meaningless in this situation
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Hmm. Actually, this is not enough.
We really must stop and cleanup vhost net.

Two issues are 
1. vhost_net_stop needs virtio device pointer
2. virtio net has a vhost_started flag

Two ways to fix it that I see:
1. add a callback in nic and invoke when peer_deleted is set
2. add vhost_started and virtio device pointers in vhost

Path 1 seems easier ...

MST

> ---
>  hw/virtio-net.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 7e1688c..68c8e48 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -245,6 +245,9 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
>  {
>      VirtIONet *n = to_virtio_net(vdev);
>  
> +    if (n->nic->peer_deleted)
> +        return;
> +
>      n->mergeable_rx_bufs = !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF));
>  
>      if (n->has_vnet_hdr) {

[Qemu-devel] Re: [PATCH] vhost-net: Do not set features for backend when peer was deleted

[Jason Wang]

Michael S. Tsirkin writes:
 > On Wed, Oct 27, 2010 at 12:43:51AM +0800, Jason Wang wrote:
 > > When hot-unplug a virtio nic with vhost-net backend, guest may
 > > continue to program the nic even if its peer have been deleted. We can
 > > not set features at this time as vhost_net_ack_features() may still
 > > try to use the tap related vhost_net structure which have been freed
 > > in tap_cleanup(). And setting offload features for a deleted backend
 > > is also meaningless in this situation
 > > 
 > > Signed-off-by: Jason Wang <jasowang@redhat.com>
 > 
 > Hmm. Actually, this is not enough.
 > We really must stop and cleanup vhost net.
 > 

But this patch just prevent the guest from programming a nic with its
vhost-net backend deleted. The vhost net was still stopped and cleaned
when the peer have been removed.

 > Two issues are 
 > 1. vhost_net_stop needs virtio device pointer
 > 2. virtio net has a vhost_started flag
 > 
 > Two ways to fix it that I see:
 > 1. add a callback in nic and invoke when peer_deleted is set
 > 2. add vhost_started and virtio device pointers in vhost
 > 
 > Path 1 seems easier ...
 > 
 > MST
 > 
 > > ---
 > >  hw/virtio-net.c |    3 +++
 > >  1 files changed, 3 insertions(+), 0 deletions(-)
 > > 
 > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
 > > index 7e1688c..68c8e48 100644
 > > --- a/hw/virtio-net.c
 > > +++ b/hw/virtio-net.c
 > > @@ -245,6 +245,9 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
 > >  {
 > >      VirtIONet *n = to_virtio_net(vdev);
 > >  
 > > +    if (n->nic->peer_deleted)
 > > +        return;
 > > +
 > >      n->mergeable_rx_bufs = !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF));
 > >  
 > >      if (n->has_vnet_hdr) {
 > 

Re: [Qemu-devel] Re: [PATCH] vhost-net: Do not set features for backend when peer was deleted

[Michael S. Tsirkin]

On Wed, Oct 27, 2010 at 02:59:16PM +0800, Jason Wang wrote:
> Michael S. Tsirkin writes:
>  > On Wed, Oct 27, 2010 at 12:43:51AM +0800, Jason Wang wrote:
>  > > When hot-unplug a virtio nic with vhost-net backend, guest may
>  > > continue to program the nic even if its peer have been deleted. We can
>  > > not set features at this time as vhost_net_ack_features() may still
>  > > try to use the tap related vhost_net structure which have been freed
>  > > in tap_cleanup(). And setting offload features for a deleted backend
>  > > is also meaningless in this situation
>  > > 
>  > > Signed-off-by: Jason Wang <jasowang@redhat.com>
>  > 
>  > Hmm. Actually, this is not enough.
>  > We really must stop and cleanup vhost net.
>  > 
> 
> But this patch just prevent the guest from programming a nic with its
> vhost-net backend deleted. The vhost net was still stopped and cleaned
> when the peer have been removed.

No, I think it was cleanup but not stopped first. E.g. try to migrate at
this point and see it crash. I suspect we also leak fds for notifiers
and what not.

>  > Two issues are 
>  > 1. vhost_net_stop needs virtio device pointer
>  > 2. virtio net has a vhost_started flag
>  > 
>  > Two ways to fix it that I see:
>  > 1. add a callback in nic and invoke when peer_deleted is set
>  > 2. add vhost_started and virtio device pointers in vhost
>  > 
>  > Path 1 seems easier ...
>  > 
>  > MST
>  > 
>  > > ---
>  > >  hw/virtio-net.c |    3 +++
>  > >  1 files changed, 3 insertions(+), 0 deletions(-)
>  > > 
>  > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>  > > index 7e1688c..68c8e48 100644
>  > > --- a/hw/virtio-net.c
>  > > +++ b/hw/virtio-net.c
>  > > @@ -245,6 +245,9 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
>  > >  {
>  > >      VirtIONet *n = to_virtio_net(vdev);
>  > >  
>  > > +    if (n->nic->peer_deleted)
>  > > +        return;
>  > > +
>  > >      n->mergeable_rx_bufs = !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF));
>  > >  
>  > >      if (n->has_vnet_hdr) {
>  > 

Re: [Qemu-devel] Re: [PATCH] vhost-net: Do not set features for backend when peer was deleted

[Michael S. Tsirkin]

On Wed, Oct 27, 2010 at 03:26:04PM +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 27, 2010 at 02:59:16PM +0800, Jason Wang wrote:
> > Michael S. Tsirkin writes:
> >  > On Wed, Oct 27, 2010 at 12:43:51AM +0800, Jason Wang wrote:
> >  > > When hot-unplug a virtio nic with vhost-net backend, guest may
> >  > > continue to program the nic even if its peer have been deleted. We can
> >  > > not set features at this time as vhost_net_ack_features() may still
> >  > > try to use the tap related vhost_net structure which have been freed
> >  > > in tap_cleanup(). And setting offload features for a deleted backend
> >  > > is also meaningless in this situation
> >  > > 
> >  > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> >  > 
> >  > Hmm. Actually, this is not enough.
> >  > We really must stop and cleanup vhost net.
> >  > 
> > 
> > But this patch just prevent the guest from programming a nic with its
> > vhost-net backend deleted. The vhost net was still stopped and cleaned
> > when the peer have been removed.
> 
> No, I think it was cleanup but not stopped first. E.g. try to migrate at
> this point and see it crash. I suspect we also leak fds for notifiers
> and what not.

Hmm. I missed the fact that we bring the link down, which
will stop vhost_net. So no, it's not broken.

But I'd rather we didn't look at peer_deleted and leave
this as an implementation detail in net.c: I think the
bug is in returning an invalid pointer from get_vhost_net.
Posted a patch to fix that.

> >  > Two issues are 
> >  > 1. vhost_net_stop needs virtio device pointer
> >  > 2. virtio net has a vhost_started flag
> >  > 
> >  > Two ways to fix it that I see:
> >  > 1. add a callback in nic and invoke when peer_deleted is set
> >  > 2. add vhost_started and virtio device pointers in vhost
> >  > 
> >  > Path 1 seems easier ...
> >  > 
> >  > MST
> >  > 
> >  > > ---
> >  > >  hw/virtio-net.c |    3 +++
> >  > >  1 files changed, 3 insertions(+), 0 deletions(-)
> >  > > 
> >  > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> >  > > index 7e1688c..68c8e48 100644
> >  > > --- a/hw/virtio-net.c
> >  > > +++ b/hw/virtio-net.c
> >  > > @@ -245,6 +245,9 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
> >  > >  {
> >  > >      VirtIONet *n = to_virtio_net(vdev);
> >  > >  
> >  > > +    if (n->nic->peer_deleted)
> >  > > +        return;
> >  > > +
> >  > >      n->mergeable_rx_bufs = !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF));
> >  > >  
> >  > >      if (n->has_vnet_hdr) {
> >  >