Patchwork virtio-net: Don't pass NULL peer to tap routines

login
register
mail settings
Submitter Alex Williamson
Date Sept. 22, 2010, 7:52 p.m.
Message ID <20100922195244.30890.93113.stgit@s20.home>
Download mbox | patch
Permalink /patch/65449/
State New
Headers show

Comments

Alex Williamson - Sept. 22, 2010, 7:52 p.m.
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 <alex.williamson@redhat.com>
---

 hw/virtio-net.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
Anthony Liguori - Sept. 23, 2010, 5:43 p.m.
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<alex.williamson@redhat.com>
>    

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.

Regards,

Anthony Liguori

> ---
>
>   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,
>
>
>
Alex Williamson - Sept. 23, 2010, 6:25 p.m.
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<alex.williamson@redhat.com>
> >    
> 
> 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,
> >
> >
> >    
>
Markus Armbruster - Sept. 24, 2010, 9:31 a.m.
Alex Williamson <alex.williamson@redhat.com> writes:

> 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

unplug?

>> > 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<alex.williamson@redhat.com>
>> >    
>> 
>> 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,

I figure we should either make netdev_del fail when the netdev is in
use, or make its users cope graciously with the netdev going away (make
it look like somebody yanked the cable).
Alex Williamson - Sept. 24, 2010, 2:17 p.m.
On Fri, 2010-09-24 at 11:31 +0200, Markus Armbruster wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
> 
> > 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
> 
> unplug?

yep

> >> > 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<alex.williamson@redhat.com>
> >> >    
> >> 
> >> 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,
> 
> I figure we should either make netdev_del fail when the netdev is in
> use, or make its users cope graciously with the netdev going away (make
> it look like somebody yanked the cable).

I'm not sure how useful it is, but I like the idea that we can swap the
netdev from under a running guest.  I believe this is possible with the
emulated drivers since they don't try to push features into the tap
device.  Perhaps something like you suggest where a netdev going away
sets a link down on the device.  If/when a netdev gets reattached, the
link returns and features are renegotiated.  Then we could move the
guest between NAT'd bridges and transparent bridges and it'd look like
we moved the network cable from one switch to another in the guest.

Alex
Michael S. Tsirkin - Sept. 26, 2010, 11:57 a.m.
On Fri, Sep 24, 2010 at 08:17:09AM -0600, Alex Williamson wrote:
> On Fri, 2010-09-24 at 11:31 +0200, Markus Armbruster wrote:
> > Alex Williamson <alex.williamson@redhat.com> writes:
> > 
> > > 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
> > 
> > unplug?
> 
> yep
> 
> > >> > 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<alex.williamson@redhat.com>
> > >> >    
> > >> 
> > >> 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,
> > 
> > I figure we should either make netdev_del fail when the netdev is in
> > use, or make its users cope graciously with the netdev going away (make
> > it look like somebody yanked the cable).
> 
> I'm not sure how useful it is, but I like the idea that we can swap the
> netdev from under a running guest.  I believe this is possible with the
> emulated drivers since they don't try to push features into the tap
> device.  Perhaps something like you suggest where a netdev going away
> sets a link down on the device.  If/when a netdev gets reattached, the
> link returns and features are renegotiated.

Existing guests don't renegotiate the features on link change though,
do they? To renegotiate features, it's probably cleaner
to have an event for this, not reuse link state change.

> Then we could move the guest between NAT'd bridges and transparent
> bridges and it'd look like we moved the network cable from one switch
> to another in the guest.
> 
> Alex

This last might in fact be possible without feature renegotiation
since both backends are tap.

Patch

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,