From 102a2786c9df756cffdbcfd11096124e4dc6c311 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:29 +1030 Subject: virtio_net: drop config_enable Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio net. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/net/virtio_net.c | 27 ++++----------------------- 1 file changed, 4 insertions(+), 23 deletions(-) (limited to 'drivers/net/virtio_net.c') diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 59caa06f34a6..743fb04a4df9 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -123,9 +123,6 @@ struct virtnet_info { /* Host can handle any s/g split between our header and packet data */ bool any_header_sg; - /* enable config space updates */ - bool config_enable; - /* Active statistics */ struct virtnet_stats __percpu *stats; @@ -1408,9 +1405,6 @@ static void virtnet_config_changed_work(struct work_struct *work) u16 v; mutex_lock(&vi->config_lock); - if (!vi->config_enable) - goto done; - if (virtio_cread_feature(vi->vdev, VIRTIO_NET_F_STATUS, struct virtio_net_config, status, &v) < 0) goto done; @@ -1758,7 +1752,6 @@ static int virtnet_probe(struct virtio_device *vdev) } mutex_init(&vi->config_lock); - vi->config_enable = true; INIT_WORK(&vi->config_work, virtnet_config_changed_work); /* If we can receive ANY GSO packets, we must allocate large ones. */ @@ -1875,17 +1868,13 @@ static void virtnet_remove(struct virtio_device *vdev) unregister_hotcpu_notifier(&vi->nb); - /* Prevent config work handler from accessing the device. */ - mutex_lock(&vi->config_lock); - vi->config_enable = false; - mutex_unlock(&vi->config_lock); + /* Make sure no work handler is accessing the device. */ + flush_work(&vi->config_work); unregister_netdev(vi->dev); remove_vq_common(vi); - flush_work(&vi->config_work); - free_percpu(vi->stats); free_netdev(vi->dev); } @@ -1898,10 +1887,8 @@ static int virtnet_freeze(struct virtio_device *vdev) unregister_hotcpu_notifier(&vi->nb); - /* Prevent config work handler from accessing the device */ - mutex_lock(&vi->config_lock); - vi->config_enable = false; - mutex_unlock(&vi->config_lock); + /* Make sure no work handler is accessing the device */ + flush_work(&vi->config_work); netif_device_detach(vi->dev); cancel_delayed_work_sync(&vi->refill); @@ -1916,8 +1903,6 @@ static int virtnet_freeze(struct virtio_device *vdev) remove_vq_common(vi); - flush_work(&vi->config_work); - return 0; } @@ -1941,10 +1926,6 @@ static int virtnet_restore(struct virtio_device *vdev) netif_device_attach(vi->dev); - mutex_lock(&vi->config_lock); - vi->config_enable = true; - mutex_unlock(&vi->config_lock); - rtnl_lock(); virtnet_set_queues(vi, vi->curr_queue_pairs); rtnl_unlock(); -- cgit v1.2.3-59-g8ed1b From 080c637373904258ecc20cedc552b2472ab03d10 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:29 +1030 Subject: virtio-net: drop config_mutex config_mutex served two purposes: prevent multiple concurrent config change handlers, and synchronize access to config_enable flag. Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1 workqueue: make all workqueues non-reentrant all workqueues are non-reentrant, and config_enable is now gone. Get rid of the unnecessary lock. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/net/virtio_net.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) (limited to 'drivers/net/virtio_net.c') diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 743fb04a4df9..23e4a69d92de 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -132,9 +132,6 @@ struct virtnet_info { /* Work struct for config space updates */ struct work_struct config_work; - /* Lock for config space updates */ - struct mutex config_lock; - /* Does the affinity hint is set for virtqueues? */ bool affinity_hint_set; @@ -1404,7 +1401,6 @@ static void virtnet_config_changed_work(struct work_struct *work) container_of(work, struct virtnet_info, config_work); u16 v; - mutex_lock(&vi->config_lock); if (virtio_cread_feature(vi->vdev, VIRTIO_NET_F_STATUS, struct virtio_net_config, status, &v) < 0) goto done; @@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work) netif_tx_stop_all_queues(vi->dev); } done: - mutex_unlock(&vi->config_lock); + return; } static void virtnet_config_changed(struct virtio_device *vdev) @@ -1751,7 +1747,6 @@ static int virtnet_probe(struct virtio_device *vdev) u64_stats_init(&virtnet_stats->rx_syncp); } - mutex_init(&vi->config_lock); INIT_WORK(&vi->config_work, virtnet_config_changed_work); /* If we can receive ANY GSO packets, we must allocate large ones. */ -- cgit v1.2.3-59-g8ed1b From 507613bf31f4bc0a344a1dfc1bc9074fed6eab8f Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:30 +1030 Subject: virtio_net: minor cleanup goto done; done: return; is ugly, it was put there to make diff review easier. replace by open-coded return. Signed-off-by: Michael S. Tsirkin Acked-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/net/virtio_net.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'drivers/net/virtio_net.c') diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 23e4a69d92de..ef04d2394282 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1403,7 +1403,7 @@ static void virtnet_config_changed_work(struct work_struct *work) if (virtio_cread_feature(vi->vdev, VIRTIO_NET_F_STATUS, struct virtio_net_config, status, &v) < 0) - goto done; + return; if (v & VIRTIO_NET_S_ANNOUNCE) { netdev_notify_peers(vi->dev); @@ -1414,7 +1414,7 @@ static void virtnet_config_changed_work(struct work_struct *work) v &= VIRTIO_NET_S_LINK_UP; if (vi->status == v) - goto done; + return; vi->status = v; @@ -1425,8 +1425,6 @@ static void virtnet_config_changed_work(struct work_struct *work) netif_carrier_off(vi->dev); netif_tx_stop_all_queues(vi->dev); } -done: - return; } static void virtnet_config_changed(struct virtio_device *vdev) -- cgit v1.2.3-59-g8ed1b From 4baf1e33d0842c9673fef4af207d4b74da8d0126 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:30 +1030 Subject: virtio_net: enable VQs early virtio spec requires drivers to set DRIVER_OK before using VQs. This is set automatically after probe returns, virtio net violated this rule by using receive VQs within probe. To fix, call virtio_device_ready before using VQs. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/net/virtio_net.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'drivers/net/virtio_net.c') diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index ef04d2394282..aba7b93286b3 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1792,6 +1792,8 @@ static int virtnet_probe(struct virtio_device *vdev) goto free_vqs; } + virtio_device_ready(vdev); + /* Last of all, set up some receive buffers. */ for (i = 0; i < vi->curr_queue_pairs; i++) { try_fill_recv(&vi->rq[i], GFP_KERNEL); -- cgit v1.2.3-59-g8ed1b From 024655555021e971203c519770609509e0af4468 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:31 +1030 Subject: virtio_net: fix use after free on allocation failure In the extremely unlikely event that driver initialization fails after RX buffers are added, virtio net frees RX buffers while VQs are still active, potentially causing device to use a freed buffer. To fix, reset device first - same as we do on device removal. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/net/virtio_net.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'drivers/net/virtio_net.c') diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index aba7b93286b3..53031e58a5fc 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1830,6 +1830,8 @@ static int virtnet_probe(struct virtio_device *vdev) return 0; free_recv_bufs: + vi->vdev->config->reset(vdev); + free_receive_bufs(vi); unregister_netdev(dev); free_vqs: -- cgit v1.2.3-59-g8ed1b From e53fbd11e983e896adaabef2d2f1695d6e0af829 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:32 +1030 Subject: virtio_net: enable VQs early on restore virtio spec requires drivers to set DRIVER_OK before using VQs. This is set automatically after restore returns, virtio net violated this rule by using receive VQs within restore. To fix, call virtio_device_ready before using VQs. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/net/virtio_net.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'drivers/net/virtio_net.c') diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 53031e58a5fc..4e0cbbcd60a7 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1912,6 +1912,8 @@ static int virtnet_restore(struct virtio_device *vdev) if (err) return err; + virtio_device_ready(vdev); + if (netif_running(vi->dev)) { for (i = 0; i < vi->curr_queue_pairs; i++) if (!try_fill_recv(&vi->rq[i], GFP_KERNEL)) -- cgit v1.2.3-59-g8ed1b