Re: [Qemu-devel] [PATCH] virtio-net: Don't pass NULL peer to tap routines

[Alex Williamson]

On Thu, 2010-09-23 at 12:43 -0500, Anthony Liguori wrote:
> On 09/22/2010 02:52 PM, Alex Williamson wrote:
> > During a hotplug, the netdev might be removed before the
> > connected virtio device.  When this happens, the guest might
> > be running cleanup operations that can trigger a segfault in
> > qemu.  Avoid one set of these by checking whether the peer
> > device is present before trying to do tap operations.
> >
> > Signed-off-by: Alex Williamson<address@hidden>
> >    
> 
> Can you explain this scenario a little better?
> 
> If nc.peer is NULL when set_features is called, it would seem to me like 
> we're in a pretty critical state.  I agree that we shouldn't set fault, 
> but I wonder if the real bug is that this can happen at all.

Unfortunately that critical state happens all the time since device_del
does an asynchronous ACPI call into the guest and libvirt isn't blocked
waiting for that to complete and doesn't poll to see if the device goes
away.  So it's actually pretty common today that the netdev disappears
before the device.  We talked about this in the community call on
Tuesday, and I think Michael is trying to think of a way to solve this,
perhaps by separating the guest releasing the device from the device
removal.

In the mean time, virtio-net has this hole that seems like it can be
avoided by simply checking some pointers on a slow path.  Since the
netdev has already disappeared, attempting to set features on it seems
pointless.  The change in the load function is really just a paranoia
check since it followed the same model of calling tap_*() funcs w/o
checking the value of nc.peer.  Thanks,

Alex

> > ---
> >
> >   hw/virtio-net.c |   10 +++++-----
> >   1 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index 0a9cae2..2c758ad 100644
> > --- a/hw/virtio-net.c
> > +++ b/hw/virtio-net.c
> > @@ -216,6 +216,10 @@ static void virtio_net_set_features(VirtIODevice 
> > *vdev, uint32_t features)
> >
> >       n->mergeable_rx_bufs = !!(features&  (1<<  VIRTIO_NET_F_MRG_RXBUF));
> >
> > +    if (!n->nic->nc.peer ||
> > +        n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> > +        return;
> > +    }
> >       if (n->has_vnet_hdr) {
> >           tap_set_offload(n->nic->nc.peer,
> >                           (features>>  VIRTIO_NET_F_GUEST_CSUM)&  1,
> > @@ -224,10 +228,6 @@ static void virtio_net_set_features(VirtIODevice 
> > *vdev, uint32_t features)
> >                           (features>>  VIRTIO_NET_F_GUEST_ECN)&  1,
> >                           (features>>  VIRTIO_NET_F_GUEST_UFO)&  1);
> >       }
> > -    if (!n->nic->nc.peer ||
> > -        n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> > -        return;
> > -    }
> >       if (!tap_get_vhost_net(n->nic->nc.peer)) {
> >           return;
> >       }
> > @@ -859,7 +859,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, 
> > int version_id)
> >               return -1;
> >           }
> >
> > -        if (n->has_vnet_hdr) {
> > +        if (n->nic->nc.peer&&  n->has_vnet_hdr) {
> >               tap_using_vnet_hdr(n->nic->nc.peer, 1);
> >               tap_set_offload(n->nic->nc.peer,
> >                       (n->vdev.guest_features>>  VIRTIO_NET_F_GUEST_CSUM)&  
> > 1,
> >
> >
> >    
>