* [PATCH v2 07/15] virtio_net: drop config_enable
[not found] <1412608214-31944-1-git-send-email-mst@redhat.com>
@ 2014-10-06 15:11 ` Michael S. Tsirkin
2014-10-06 15:28 ` Cornelia Huck
2014-10-06 15:11 ` [PATCH v2 08/15] virtio-net: drop config_mutex Michael S. Tsirkin
` (4 subsequent siblings)
5 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2014-10-06 15:11 UTC (permalink / raw)
To: linux-kernel; +Cc: kvm, Rusty Russell, virtualization, netdev
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 <mst@redhat.com>
---
drivers/net/virtio_net.c | 27 ++++-----------------------
1 file changed, 4 insertions(+), 23 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 59caa06..743fb04 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();
--
MST
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 07/15] virtio_net: drop config_enable
2014-10-06 15:11 ` [PATCH v2 07/15] virtio_net: drop config_enable Michael S. Tsirkin
@ 2014-10-06 15:28 ` Cornelia Huck
0 siblings, 0 replies; 8+ messages in thread
From: Cornelia Huck @ 2014-10-06 15:28 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
On Mon, 6 Oct 2014 18:11:05 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> 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 <mst@redhat.com>
> ---
> drivers/net/virtio_net.c | 27 ++++-----------------------
> 1 file changed, 4 insertions(+), 23 deletions(-)
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 08/15] virtio-net: drop config_mutex
[not found] <1412608214-31944-1-git-send-email-mst@redhat.com>
2014-10-06 15:11 ` [PATCH v2 07/15] virtio_net: drop config_enable Michael S. Tsirkin
@ 2014-10-06 15:11 ` Michael S. Tsirkin
2014-10-06 15:11 ` [PATCH v2 09/15] virtio_net: minor cleanup Michael S. Tsirkin
` (3 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2014-10-06 15:11 UTC (permalink / raw)
To: linux-kernel; +Cc: kvm, Rusty Russell, virtualization, Cornelia Huck, netdev
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 <mst@redhat.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
drivers/net/virtio_net.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 743fb04..23e4a69 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. */
--
MST
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 09/15] virtio_net: minor cleanup
[not found] <1412608214-31944-1-git-send-email-mst@redhat.com>
2014-10-06 15:11 ` [PATCH v2 07/15] virtio_net: drop config_enable Michael S. Tsirkin
2014-10-06 15:11 ` [PATCH v2 08/15] virtio-net: drop config_mutex Michael S. Tsirkin
@ 2014-10-06 15:11 ` Michael S. Tsirkin
2014-10-06 15:11 ` [PATCH v2 11/15] virtio_net: enable VQs early Michael S. Tsirkin
` (2 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2014-10-06 15:11 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, kvm, virtualization
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 <mst@redhat.com>
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
drivers/net/virtio_net.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 23e4a69..ef04d23 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)
--
MST
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 11/15] virtio_net: enable VQs early
[not found] <1412608214-31944-1-git-send-email-mst@redhat.com>
` (2 preceding siblings ...)
2014-10-06 15:11 ` [PATCH v2 09/15] virtio_net: minor cleanup Michael S. Tsirkin
@ 2014-10-06 15:11 ` Michael S. Tsirkin
2014-10-06 15:39 ` Cornelia Huck
2014-10-06 15:11 ` [PATCH v2 14/15] 9p/trans_virtio: " Michael S. Tsirkin
2014-10-06 15:11 ` [PATCH v2 15/15] virtio_net: fix use after free on allocation failure Michael S. Tsirkin
5 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2014-10-06 15:11 UTC (permalink / raw)
To: linux-kernel; +Cc: kvm, Rusty Russell, virtualization, netdev
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_enable_vqs_early before using VQs.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/net/virtio_net.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ef04d23..430f3ae 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_enable_vqs_early(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);
--
MST
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 14/15] 9p/trans_virtio: enable VQs early
[not found] <1412608214-31944-1-git-send-email-mst@redhat.com>
` (3 preceding siblings ...)
2014-10-06 15:11 ` [PATCH v2 11/15] virtio_net: enable VQs early Michael S. Tsirkin
@ 2014-10-06 15:11 ` Michael S. Tsirkin
2014-10-06 15:11 ` [PATCH v2 15/15] virtio_net: fix use after free on allocation failure Michael S. Tsirkin
5 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2014-10-06 15:11 UTC (permalink / raw)
To: linux-kernel
Cc: kvm, Eric Van Hensbergen, netdev, virtualization, v9fs-developer,
Ron Minnich, David S. Miller
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after probe returns, but virtio 9p device
adds self to channel list within probe, at which point VQ can be
used in violation of the spec.
To fix, call virtio_enable_vqs_early before using VQs.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/9p/trans_virtio.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 6940d8f..766ba48 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -575,6 +575,8 @@ static int p9_virtio_probe(struct virtio_device *vdev)
/* Ceiling limit to avoid denial of service attacks */
chan->p9_max_pages = nr_free_buffer_pages()/4;
+ virtio_enable_vqs_early(vdev);
+
mutex_lock(&virtio_9p_lock);
list_add_tail(&chan->chan_list, &virtio_chan_list);
mutex_unlock(&virtio_9p_lock);
--
MST
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 15/15] virtio_net: fix use after free on allocation failure
[not found] <1412608214-31944-1-git-send-email-mst@redhat.com>
` (4 preceding siblings ...)
2014-10-06 15:11 ` [PATCH v2 14/15] 9p/trans_virtio: " Michael S. Tsirkin
@ 2014-10-06 15:11 ` Michael S. Tsirkin
5 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2014-10-06 15:11 UTC (permalink / raw)
To: linux-kernel; +Cc: kvm, Rusty Russell, virtualization, Cornelia Huck, netdev
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 <mst@redhat.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
drivers/net/virtio_net.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 430f3ae..3551417 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:
--
MST
^ permalink raw reply related [flat|nested] 8+ messages in thread