On 2019/7/23 下午5:16, Michael S. Tsirkin wrote:
On Tue, Jul 23, 2019 at 03:57:18AM -0400, Jason Wang wrote:
There's no need for RCU synchronization in vhost_uninit_vq_maps()
since we've already serialized with readers (memory accessors). This
also avoid the possible userspace DOS through ioctl() because of the
possible high latency caused by synchronize_rcu().
Reported-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
Fixes: 7f466032dc9e ("vhost: access vq metadata through kernel virtual address")
Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
I agree synchronize_rcu in both mmu notifiers and ioctl
is a problem we must fix.
---
drivers/vhost/vhost.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 5b8821d00fe4..a17df1f4069a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -334,7 +334,9 @@ static void vhost_uninit_vq_maps(struct vhost_virtqueue *vq)
}
spin_unlock(&vq->mmu_lock);
- synchronize_rcu();
+ /* No need for synchronize_rcu() or kfree_rcu() since we are
+ * serialized with memory accessors (e.g vq mutex held).
+ */
for (i = 0; i < VHOST_NUM_ADDRS; i++)
if (map[i])
--
2.18.1
.. however we can not RCU with no synchronization in sight.
Sometimes there are hacks like using a lock/unlock
pair instead of sync, but here no one bothers.
specifically notifiers call reset vq maps which calls
uninit vq maps which is not under any lock.
Notifier did this:
if (map) {
if (uaddr->write) {
for (i = 0; i < map->npages; i++)
set_page_dirty(map->pages[i]);
}
rcu_assign_pointer(vq->maps[index], NULL);
}
spin_unlock(&vq->mmu_lock);
if (map) {
synchronize_rcu();
vhost_map_unprefetch(map);
}
So it indeed have a synchronize_rcu() there.
Thanks
You will get use after free when map is then accessed.
If you always have a lock then just take that lock
and no need for RCU.
