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

[Michael S. Tsirkin]

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 <address@hidden> 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<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,
> > 
> > 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.

-- 
MST