* [PATCH net v2] vhost: fix a theoretical race in device cleanup
@ 2014-02-13 9:45 ` Michael S. Tsirkin
0 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2014-02-13 9:45 UTC (permalink / raw)
To: linux-kernel, David Miller
Cc: kvm, virtio-dev, virtualization, netdev, Jason Wang
vhost_zerocopy_callback accesses VQ right after it drops a ubuf
reference. In theory, this could race with device removal which waits
on the ubuf kref, and crash on use after free.
Do all accesses within rcu read side critical section, and synchronize
on release.
Since callbacks are always invoked from bh, synchronize_rcu_bh seems
enough and will help release complete a bit faster.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
This is was previously posted as part of patch
series, but it's an independent fix really.
Theoretical race so not needed for stable I think.
changes from v1:
fixed typo in commit log
drivers/vhost/net.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index b12176f..f1be80d 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -308,6 +308,8 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
struct vhost_virtqueue *vq = ubufs->vq;
int cnt;
+ rcu_read_lock_bh();
+
/* set len to mark this desc buffers done DMA */
vq->heads[ubuf->desc].len = success ?
VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN;
@@ -322,6 +324,8 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
*/
if (cnt <= 1 || !(cnt % 16))
vhost_poll_queue(&vq->poll);
+
+ rcu_read_unlock_bh();
}
/* Expects to be always run from workqueue - which acts as
@@ -804,6 +808,8 @@ static int vhost_net_release(struct inode *inode, struct file *f)
fput(tx_sock->file);
if (rx_sock)
fput(rx_sock->file);
+ /* Make sure no callbacks are outstanding */
+ synchronize_rcu_bh();
/* We do an extra flush before freeing memory,
* since jobs can re-queue themselves. */
vhost_net_flush(n);
--
MST
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH net v2] vhost: fix a theoretical race in device cleanup
@ 2014-02-13 9:45 ` Michael S. Tsirkin
0 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2014-02-13 9:45 UTC (permalink / raw)
To: linux-kernel, David Miller; +Cc: virtio-dev, netdev, kvm, virtualization
vhost_zerocopy_callback accesses VQ right after it drops a ubuf
reference. In theory, this could race with device removal which waits
on the ubuf kref, and crash on use after free.
Do all accesses within rcu read side critical section, and synchronize
on release.
Since callbacks are always invoked from bh, synchronize_rcu_bh seems
enough and will help release complete a bit faster.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
This is was previously posted as part of patch
series, but it's an independent fix really.
Theoretical race so not needed for stable I think.
changes from v1:
fixed typo in commit log
drivers/vhost/net.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index b12176f..f1be80d 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -308,6 +308,8 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
struct vhost_virtqueue *vq = ubufs->vq;
int cnt;
+ rcu_read_lock_bh();
+
/* set len to mark this desc buffers done DMA */
vq->heads[ubuf->desc].len = success ?
VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN;
@@ -322,6 +324,8 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
*/
if (cnt <= 1 || !(cnt % 16))
vhost_poll_queue(&vq->poll);
+
+ rcu_read_unlock_bh();
}
/* Expects to be always run from workqueue - which acts as
@@ -804,6 +808,8 @@ static int vhost_net_release(struct inode *inode, struct file *f)
fput(tx_sock->file);
if (rx_sock)
fput(rx_sock->file);
+ /* Make sure no callbacks are outstanding */
+ synchronize_rcu_bh();
/* We do an extra flush before freeing memory,
* since jobs can re-queue themselves. */
vhost_net_flush(n);
--
MST
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [virtio-dev] [PATCH net v2] vhost: fix a theoretical race in device cleanup
2014-02-13 9:45 ` Michael S. Tsirkin
@ 2014-02-13 10:01 ` Jason Wang
-1 siblings, 0 replies; 7+ messages in thread
From: Jason Wang @ 2014-02-13 10:01 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel, David Miller
Cc: kvm, virtio-dev, virtualization, netdev
On 02/13/2014 05:45 PM, Michael S. Tsirkin wrote:
> vhost_zerocopy_callback accesses VQ right after it drops a ubuf
> reference. In theory, this could race with device removal which waits
> on the ubuf kref, and crash on use after free.
>
> Do all accesses within rcu read side critical section, and synchronize
> on release.
>
> Since callbacks are always invoked from bh, synchronize_rcu_bh seems
> enough and will help release complete a bit faster.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> This is was previously posted as part of patch
> series, but it's an independent fix really.
> Theoretical race so not needed for stable I think.
>
> changes from v1:
> fixed typo in commit log
>
> drivers/vhost/net.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index b12176f..f1be80d 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -308,6 +308,8 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
> struct vhost_virtqueue *vq = ubufs->vq;
> int cnt;
>
> + rcu_read_lock_bh();
> +
> /* set len to mark this desc buffers done DMA */
> vq->heads[ubuf->desc].len = success ?
> VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN;
> @@ -322,6 +324,8 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
> */
> if (cnt <= 1 || !(cnt % 16))
> vhost_poll_queue(&vq->poll);
> +
> + rcu_read_unlock_bh();
> }
>
> /* Expects to be always run from workqueue - which acts as
> @@ -804,6 +808,8 @@ static int vhost_net_release(struct inode *inode, struct file *f)
> fput(tx_sock->file);
> if (rx_sock)
> fput(rx_sock->file);
> + /* Make sure no callbacks are outstanding */
> + synchronize_rcu_bh();
> /* We do an extra flush before freeing memory,
> * since jobs can re-queue themselves. */
> vhost_net_flush(n);
Acked-by: Jason Wang <jasowang@redhat.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v2] vhost: fix a theoretical race in device cleanup
@ 2014-02-13 10:01 ` Jason Wang
0 siblings, 0 replies; 7+ messages in thread
From: Jason Wang @ 2014-02-13 10:01 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel, David Miller
Cc: kvm, virtio-dev, virtualization, netdev
On 02/13/2014 05:45 PM, Michael S. Tsirkin wrote:
> vhost_zerocopy_callback accesses VQ right after it drops a ubuf
> reference. In theory, this could race with device removal which waits
> on the ubuf kref, and crash on use after free.
>
> Do all accesses within rcu read side critical section, and synchronize
> on release.
>
> Since callbacks are always invoked from bh, synchronize_rcu_bh seems
> enough and will help release complete a bit faster.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> This is was previously posted as part of patch
> series, but it's an independent fix really.
> Theoretical race so not needed for stable I think.
>
> changes from v1:
> fixed typo in commit log
>
> drivers/vhost/net.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index b12176f..f1be80d 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -308,6 +308,8 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
> struct vhost_virtqueue *vq = ubufs->vq;
> int cnt;
>
> + rcu_read_lock_bh();
> +
> /* set len to mark this desc buffers done DMA */
> vq->heads[ubuf->desc].len = success ?
> VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN;
> @@ -322,6 +324,8 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
> */
> if (cnt <= 1 || !(cnt % 16))
> vhost_poll_queue(&vq->poll);
> +
> + rcu_read_unlock_bh();
> }
>
> /* Expects to be always run from workqueue - which acts as
> @@ -804,6 +808,8 @@ static int vhost_net_release(struct inode *inode, struct file *f)
> fput(tx_sock->file);
> if (rx_sock)
> fput(rx_sock->file);
> + /* Make sure no callbacks are outstanding */
> + synchronize_rcu_bh();
> /* We do an extra flush before freeing memory,
> * since jobs can re-queue themselves. */
> vhost_net_flush(n);
Acked-by: Jason Wang <jasowang@redhat.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [virtio-dev] [PATCH net v2] vhost: fix a theoretical race in device cleanup
2014-02-13 9:45 ` Michael S. Tsirkin
(?)
@ 2014-02-13 10:01 ` Jason Wang
-1 siblings, 0 replies; 7+ messages in thread
From: Jason Wang @ 2014-02-13 10:01 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel, David Miller
Cc: virtio-dev, netdev, kvm, virtualization
On 02/13/2014 05:45 PM, Michael S. Tsirkin wrote:
> vhost_zerocopy_callback accesses VQ right after it drops a ubuf
> reference. In theory, this could race with device removal which waits
> on the ubuf kref, and crash on use after free.
>
> Do all accesses within rcu read side critical section, and synchronize
> on release.
>
> Since callbacks are always invoked from bh, synchronize_rcu_bh seems
> enough and will help release complete a bit faster.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> This is was previously posted as part of patch
> series, but it's an independent fix really.
> Theoretical race so not needed for stable I think.
>
> changes from v1:
> fixed typo in commit log
>
> drivers/vhost/net.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index b12176f..f1be80d 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -308,6 +308,8 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
> struct vhost_virtqueue *vq = ubufs->vq;
> int cnt;
>
> + rcu_read_lock_bh();
> +
> /* set len to mark this desc buffers done DMA */
> vq->heads[ubuf->desc].len = success ?
> VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN;
> @@ -322,6 +324,8 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
> */
> if (cnt <= 1 || !(cnt % 16))
> vhost_poll_queue(&vq->poll);
> +
> + rcu_read_unlock_bh();
> }
>
> /* Expects to be always run from workqueue - which acts as
> @@ -804,6 +808,8 @@ static int vhost_net_release(struct inode *inode, struct file *f)
> fput(tx_sock->file);
> if (rx_sock)
> fput(rx_sock->file);
> + /* Make sure no callbacks are outstanding */
> + synchronize_rcu_bh();
> /* We do an extra flush before freeing memory,
> * since jobs can re-queue themselves. */
> vhost_net_flush(n);
Acked-by: Jason Wang <jasowang@redhat.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v2] vhost: fix a theoretical race in device cleanup
2014-02-13 9:45 ` Michael S. Tsirkin
@ 2014-02-13 23:48 ` David Miller
-1 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2014-02-13 23:48 UTC (permalink / raw)
To: mst; +Cc: linux-kernel, kvm, virtio-dev, virtualization, netdev, jasowang
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Thu, 13 Feb 2014 11:45:11 +0200
> vhost_zerocopy_callback accesses VQ right after it drops a ubuf
> reference. In theory, this could race with device removal which waits
> on the ubuf kref, and crash on use after free.
>
> Do all accesses within rcu read side critical section, and synchronize
> on release.
>
> Since callbacks are always invoked from bh, synchronize_rcu_bh seems
> enough and will help release complete a bit faster.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> This is was previously posted as part of patch
> series, but it's an independent fix really.
> Theoretical race so not needed for stable I think.
Ok, no -stable, applied.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v2] vhost: fix a theoretical race in device cleanup
@ 2014-02-13 23:48 ` David Miller
0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2014-02-13 23:48 UTC (permalink / raw)
To: mst; +Cc: virtio-dev, kvm, netdev, linux-kernel, virtualization
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Thu, 13 Feb 2014 11:45:11 +0200
> vhost_zerocopy_callback accesses VQ right after it drops a ubuf
> reference. In theory, this could race with device removal which waits
> on the ubuf kref, and crash on use after free.
>
> Do all accesses within rcu read side critical section, and synchronize
> on release.
>
> Since callbacks are always invoked from bh, synchronize_rcu_bh seems
> enough and will help release complete a bit faster.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> This is was previously posted as part of patch
> series, but it's an independent fix really.
> Theoretical race so not needed for stable I think.
Ok, no -stable, applied.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-02-13 23:48 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-13 9:45 [PATCH net v2] vhost: fix a theoretical race in device cleanup Michael S. Tsirkin
2014-02-13 9:45 ` Michael S. Tsirkin
2014-02-13 10:01 ` [virtio-dev] " Jason Wang
2014-02-13 10:01 ` Jason Wang
2014-02-13 10:01 ` Jason Wang
2014-02-13 23:48 ` David Miller
2014-02-13 23:48 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.