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

[Anthony Liguori <anthony@codemonkey.ws>]

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,
>
>
>