Re: [Qemu-devel] [PATCH] clean up callback when del virtqueue

[liujunjie (A)]

> -----Original Message-----
> From: Jason Wang [mailto:address@hidden
> Sent: Friday, September 14, 2018 8:45 PM
> To: liujunjie (A) <address@hidden>; address@hidden
> Cc: Huangweidong (C) <address@hidden>; wangxin (U)
> <address@hidden>; address@hidden; Gonglei (Arei)
> <address@hidden>; Zhoujian (jay) <address@hidden>
> Subject: Re: [Qemu-devel] [PATCH] clean up callback when del virtqueue
> 
> 
> 
> On 2018年09月08日 21:04, liujunjie wrote:
> > Before, we did not clear callback like handle_output when delete the
> > virtqueue which may result be segmentfault.
> > The scene is as follows:
> > 1. Start a vm with multiqueue vhost-net, 2. then we write
> > VIRTIO_PCI_GUEST_FEATURES in PCI configuration to triger multiqueue
> > disable in this vm which will delete the virtqueue.
> > In this step, the tx_bh is deleted but the callback
> > virtio_net_handle_tx_bh still exist.
> > 3. Finally, we write VIRTIO_PCI_QUEUE_NOTIFY in PCI configuration to
> > notify the deleted virtqueue. In this way, virtio_net_handle_tx_bh
> > will be called and qemu will be crashed.
> 
> Good catch.
> 
> >
> > Although the way described above is uncommon, we had better reinforce it.
> >
> > Signed-off-by: liujunjie <address@hidden>
> > ---
> >   hw/net/virtio-net.c | 4 +++-
> >   hw/virtio/virtio.c  | 3 +++
> >   2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index
> > f154756..9bb20e3 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -1467,7 +1467,9 @@ static void virtio_net_handle_tx_bh(VirtIODevice
> *vdev, VirtQueue *vq)
> >           return;
> >       }
> >       virtio_queue_set_notification(vq, 0);
> > -    qemu_bh_schedule(q->tx_bh);
> > +    if (q->tx_bh) {
> > +        qemu_bh_schedule(q->tx_bh);
> > +    }
> 
> I believe this is not necessary since handle_output is NULL now?

Yes, it is not necessary.

> 
> >   }
> >
> >   static void virtio_net_tx_timer(void *opaque) diff --git
> > a/hw/virtio/virtio.c b/hw/virtio/virtio.c index d4e4d98..7577518
> > 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1604,6 +1604,9 @@ void virtio_del_queue(VirtIODevice *vdev, int n)
> >
> >       vdev->vq[n].vring.num = 0;
> >       vdev->vq[n].vring.num_default = 0;
> > +    vdev->vq[n].vring.align = 0;
> 
> Is this related or fix for another bug?

It's not related for another bug. So it is also unnecessary to add this, too.
The reason why add it is only trying to correspond to the function 
<virtio_add_queue> in the same file.

> 
> Thanks
> 
> > +    vdev->vq[n].handle_output = NULL;
> > +    vdev->vq[n].handle_aio_output = NULL;
> >   }
> >
> >   static void virtio_set_isr(VirtIODevice *vdev, int value)