Messages in this thread | | | Subject | Re: [PATCH 6/6] vhost: don't do synchronize_rcu() in vhost_uninit_vq_maps() | From | Jason Wang <> | Date | Tue, 23 Jul 2019 21:16:20 +0800 |
| |
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@redhat.com> >> Fixes: 7f466032dc9e ("vhost: access vq metadata through kernel virtual address") >> Signed-off-by: Jason Wang <jasowang@redhat.com> > 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. >
| |