From e1d64c084b2cc7e907b4e64026d8c8dba59116f8 Mon Sep 17 00:00:00 2001 From: zhanghailiang Date: Tue, 26 Aug 2014 16:06:17 +0800 Subject: [PATCH 1/6] net: Forbid dealing with packets when VM is not running For all NICs(except virtio-net) emulated by qemu, Such as e1000, rtl8139, pcnet and ne2k_pci, Qemu can still receive packets when VM is not running. If this happened in *migration's* last PAUSE VM stage, but before the end of the migration, the new receiving packets will possibly dirty parts of RAM which has been cached in *iovec*(will be sent asynchronously) and dirty parts of new RAM which will be missed. This will lead serious network fault in VM. To avoid this, we forbid receiving packets in generic net code when VM is not running. Bug reproduction steps: (1) Start a VM which configured at least one NIC (2) In VM, open several Terminal and do *Ping IP -i 0.1* (3) Migrate the VM repeatedly between two Hosts And the *PING* command in VM will very likely fail with message: 'Destination HOST Unreachable', the NIC in VM will stay unavailable unless you run 'service network restart' Signed-off-by: zhanghailiang Reviewed-by: Jason Wang Reviewed-by: Juan Quintela Reviewed-by: Michael S. Tsirkin Signed-off-by: Stefan Hajnoczi --- net/net.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/net/net.c b/net/net.c index 6d930ea63b..962c05f6db 100644 --- a/net/net.c +++ b/net/net.c @@ -41,6 +41,7 @@ #include "qapi-visit.h" #include "qapi/opts-visitor.h" #include "qapi/dealloc-visitor.h" +#include "sysemu/sysemu.h" /* Net bridge is currently not supported for W32. */ #if !defined(_WIN32) @@ -452,6 +453,12 @@ void qemu_set_vnet_hdr_len(NetClientState *nc, int len) int qemu_can_send_packet(NetClientState *sender) { + int vm_running = runstate_is_running(); + + if (!vm_running) { + return 0; + } + if (!sender->peer) { return 1; } From e8bcf842001739765b8dcc1996d86a0ffd2054d5 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Tue, 2 Sep 2014 17:26:12 +0300 Subject: [PATCH 2/6] virtio-net: don't run bh on vm stopped commit 783e7706937fe15523b609b545587a028a2bdd03 virtio-net: stop/start bh when appropriate is incomplete: BH might execute within the same main loop iteration but after vmstop, so in theory, we might trigger an assertion. I was unable to reproduce this in practice, but it seems clear enough that the potential is there, so worth fixing. Cc: qemu-stable@nongnu.org Reported-by: Stefan Hajnoczi Signed-off-by: Michael S. Tsirkin Signed-off-by: Stefan Hajnoczi --- hw/net/virtio-net.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 268eff9df8..365e266b74 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -1224,7 +1224,12 @@ static void virtio_net_tx_timer(void *opaque) VirtIONetQueue *q = opaque; VirtIONet *n = q->n; VirtIODevice *vdev = VIRTIO_DEVICE(n); - assert(vdev->vm_running); + /* This happens when device was stopped but BH wasn't. */ + if (!vdev->vm_running) { + /* Make sure tx waiting is set, so we'll run when restarted. */ + assert(q->tx_waiting); + return; + } q->tx_waiting = 0; @@ -1244,7 +1249,12 @@ static void virtio_net_tx_bh(void *opaque) VirtIODevice *vdev = VIRTIO_DEVICE(n); int32_t ret; - assert(vdev->vm_running); + /* This happens when device was stopped but BH wasn't. */ + if (!vdev->vm_running) { + /* Make sure tx waiting is set, so we'll run when restarted. */ + assert(q->tx_waiting); + return; + } q->tx_waiting = 0; From 269bd822e7f5ab80048b05fb7076236ed66ffbce Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 4 Sep 2014 13:32:54 +0300 Subject: [PATCH 3/6] virtio: don't call device on !vm_running On vm stop, virtio changes vm_running state too soon, so callbacks can get envoked with vm_running = false; Cc: qemu-stable@nongnu.org Cc: Jason Wang Signed-off-by: Michael S. Tsirkin Signed-off-by: Stefan Hajnoczi --- hw/virtio/virtio.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 5c981801f3..ac222385d6 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1108,7 +1108,10 @@ static void virtio_vmstate_change(void *opaque, int running, RunState state) BusState *qbus = qdev_get_parent_bus(DEVICE(vdev)); VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); bool backend_run = running && (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK); - vdev->vm_running = running; + + if (running) { + vdev->vm_running = running; + } if (backend_run) { virtio_set_status(vdev, vdev->status); @@ -1121,6 +1124,10 @@ static void virtio_vmstate_change(void *opaque, int running, RunState state) if (!backend_run) { virtio_set_status(vdev, vdev->status); } + + if (!running) { + vdev->vm_running = running; + } } void virtio_init(VirtIODevice *vdev, const char *name, From 07d8084624b3f5cbde7777849147a6a3a862e90a Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 4 Sep 2014 11:39:10 +0300 Subject: [PATCH 4/6] net: invoke callback when purging queue devices rely on packet callbacks eventually running, but we violate this rule whenever we purge the queue. To fix, invoke callbacks on all packets on purge. Set length to 0, this way callers can detect that this happened and re-queue if necessary. Cc: qemu-stable@nongnu.org Cc: Jason Wang Signed-off-by: Michael S. Tsirkin Acked-by: Jason Wang Signed-off-by: Stefan Hajnoczi --- net/queue.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/net/queue.c b/net/queue.c index 859d02a136..f948318718 100644 --- a/net/queue.c +++ b/net/queue.c @@ -233,6 +233,9 @@ void qemu_net_queue_purge(NetQueue *queue, NetClientState *from) if (packet->sender == from) { QTAILQ_REMOVE(&queue->packets, packet, entry); queue->nq_count--; + if (packet->sent_cb) { + packet->sent_cb(packet->sender, 0); + } g_free(packet); } } From ca77d85e1dbf929ae677a0bac96e9b3edd1704da Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 4 Sep 2014 11:39:13 +0300 Subject: [PATCH 5/6] net: complete all queued packets on VM stop This completes all packets, ensuring that callbacks will not run when VM is stopped. Cc: qemu-stable@nongnu.org Cc: Jason Wang Signed-off-by: Michael S. Tsirkin Signed-off-by: Stefan Hajnoczi --- net/net.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/net/net.c b/net/net.c index 962c05f6db..7acc162b44 100644 --- a/net/net.c +++ b/net/net.c @@ -48,6 +48,7 @@ # define CONFIG_NET_BRIDGE #endif +static VMChangeStateEntry *net_change_state_entry; static QTAILQ_HEAD(, NetClientState) net_clients; const char *host_net_devices[] = { @@ -511,7 +512,8 @@ void qemu_purge_queued_packets(NetClientState *nc) qemu_net_queue_purge(nc->peer->incoming_queue, nc); } -void qemu_flush_queued_packets(NetClientState *nc) +static +void qemu_flush_or_purge_queued_packets(NetClientState *nc, bool purge) { nc->receive_disabled = 0; @@ -525,9 +527,17 @@ void qemu_flush_queued_packets(NetClientState *nc) * the file descriptor (for tap, for example). */ qemu_notify_event(); + } else if (purge) { + /* Unable to empty the queue, purge remaining packets */ + qemu_net_queue_purge(nc->incoming_queue, nc); } } +void qemu_flush_queued_packets(NetClientState *nc) +{ + qemu_flush_or_purge_queued_packets(nc, false); +} + static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender, unsigned flags, const uint8_t *buf, int size, @@ -1175,6 +1185,22 @@ void qmp_set_link(const char *name, bool up, Error **errp) } } +static void net_vm_change_state_handler(void *opaque, int running, + RunState state) +{ + /* Complete all queued packets, to guarantee we don't modify + * state later when VM is not running. + */ + if (!running) { + NetClientState *nc; + NetClientState *tmp; + + QTAILQ_FOREACH_SAFE(nc, &net_clients, next, tmp) { + qemu_flush_or_purge_queued_packets(nc, true); + } + } +} + void net_cleanup(void) { NetClientState *nc; @@ -1190,6 +1216,8 @@ void net_cleanup(void) qemu_del_net_client(nc); } } + + qemu_del_vm_change_state_handler(net_change_state_entry); } void net_check_clients(void) @@ -1275,6 +1303,9 @@ int net_init_clients(void) #endif } + net_change_state_entry = + qemu_add_vm_change_state_handler(net_vm_change_state_handler, NULL); + QTAILQ_INIT(&net_clients); if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL, 1) == -1) From 086abc1ccd0fa5103345adda819e6c6436949579 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 4 Sep 2014 11:39:17 +0300 Subject: [PATCH 6/6] virtio-net: purge outstanding packets when starting vhost whenever we start vhost, virtio could have outstanding packets queued, when they complete later we'll modify the ring while vhost is processing it. To prevent this, purge outstanding packets on vhost start. Cc: qemu-stable@nongnu.org Cc: Jason Wang Signed-off-by: Michael S. Tsirkin Signed-off-by: Stefan Hajnoczi --- hw/net/virtio-net.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 365e266b74..826a2a5fca 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -125,10 +125,23 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) return; } if (!n->vhost_started) { - int r; + int r, i; + if (!vhost_net_query(get_vhost_net(nc->peer), vdev)) { return; } + + /* Any packets outstanding? Purge them to avoid touching rings + * when vhost is running. + */ + for (i = 0; i < queues; i++) { + NetClientState *qnc = qemu_get_subqueue(n->nic, i); + + /* Purge both directions: TX and RX. */ + qemu_net_queue_purge(qnc->peer->incoming_queue, qnc); + qemu_net_queue_purge(qnc->incoming_queue, qnc->peer); + } + n->vhost_started = 1; r = vhost_net_start(vdev, n->nic->ncs, queues); if (r < 0) {