| Message ID | 20180908130448.24516-1-liujunjie23@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | clean up callback when del virtqueue | expand |
ping > -----Original Message----- > From: liujunjie (A) > Sent: Saturday, September 08, 2018 9:05 PM > To: mst@redhat.com; jasowang@redhat.com > Cc: wangxin (U) <wangxinxin.wang@huawei.com>; Zhoujian (jay) > <jianjay.zhou@huawei.com>; Gonglei (Arei) <arei.gonglei@huawei.com>; > Huangweidong (C) <weidong.huang@huawei.com>; qemu-devel@nongnu.org; > liujunjie (A) <liujunjie23@huawei.com> > Subject: [PATCH] clean up callback when del virtqueue > > 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. > > Although the way described above is uncommon, we had better reinforce it. > > Signed-off-by: liujunjie <liujunjie23@huawei.com> > --- > 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); > + } > } > > 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; > + vdev->vq[n].handle_output = NULL; > + vdev->vq[n].handle_aio_output = NULL; > } > > static void virtio_set_isr(VirtIODevice *vdev, int value) > -- > 1.8.3.1 >
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 <liujunjie23@huawei.com> > --- > 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? > } > > 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? Thanks > + vdev->vq[n].handle_output = NULL; > + vdev->vq[n].handle_aio_output = NULL; > } > > static void virtio_set_isr(VirtIODevice *vdev, int value)
> -----Original Message----- > From: Jason Wang [mailto:jasowang@redhat.com] > Sent: Friday, September 14, 2018 8:45 PM > To: liujunjie (A) <liujunjie23@huawei.com>; mst@redhat.com > Cc: Huangweidong (C) <weidong.huang@huawei.com>; wangxin (U) > <wangxinxin.wang@huawei.com>; qemu-devel@nongnu.org; Gonglei (Arei) > <arei.gonglei@huawei.com>; Zhoujian (jay) <jianjay.zhou@huawei.com> > 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 <liujunjie23@huawei.com> > > --- > > 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)
On 2018年09月14日 21:14, liujunjie (A) wrote: > >> -----Original Message----- >> From: Jason Wang [mailto:jasowang@redhat.com] >> Sent: Friday, September 14, 2018 8:45 PM >> To: liujunjie (A) <liujunjie23@huawei.com>; mst@redhat.com >> Cc: Huangweidong (C) <weidong.huang@huawei.com>; wangxin (U) >> <wangxinxin.wang@huawei.com>; qemu-devel@nongnu.org; Gonglei (Arei) >> <arei.gonglei@huawei.com>; Zhoujian (jay) <jianjay.zhou@huawei.com> >> 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 <liujunjie23@huawei.com> >>> --- >>> 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. Ok, please send V2 and cc qemu stable. Thanks > >> Thanks >> >>> + vdev->vq[n].handle_output = NULL; >>> + vdev->vq[n].handle_aio_output = NULL; >>> } >>> >>> static void virtio_set_isr(VirtIODevice *vdev, int value)
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); + } } 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; + vdev->vq[n].handle_output = NULL; + vdev->vq[n].handle_aio_output = NULL; } static void virtio_set_isr(VirtIODevice *vdev, int value)
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. Although the way described above is uncommon, we had better reinforce it. Signed-off-by: liujunjie <liujunjie23@huawei.com> --- hw/net/virtio-net.c | 4 +++- hw/virtio/virtio.c | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-)