| Message ID | 20100922195244.30890.93113.stgit@s20.home |
|---|---|
| State | New |
| Headers | show |
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, > > >
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, > > > > > > >
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).
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
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.
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,
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(-)