From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=39615 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OypqJ-0006NW-QT for qemu-devel@nongnu.org; Thu, 23 Sep 2010 13:44:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OypqI-00023p-Ef for qemu-devel@nongnu.org; Thu, 23 Sep 2010 13:44:23 -0400 Received: from mail-qy0-f173.google.com ([209.85.216.173]:56499) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OypqI-00023V-CR for qemu-devel@nongnu.org; Thu, 23 Sep 2010 13:44:22 -0400 Received: by qyk9 with SMTP id 9so2279qyk.4 for ; Thu, 23 Sep 2010 10:44:21 -0700 (PDT) Message-ID: <4C9B91CC.90802@codemonkey.ws> Date: Thu, 23 Sep 2010 12:43:40 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] virtio-net: Don't pass NULL peer to tap routines References: <20100922195244.30890.93113.stgit@s20.home> In-Reply-To: <20100922195244.30890.93113.stgit@s20.home> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: qemu-devel@nongnu.org, mst@redhat.com 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 > 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, > > >