Jason Wang
2022-Jul-14 02:27 UTC
[PATCH v2 1/5] vduse: Remove unnecessary spin lock protection
On Wed, Jul 13, 2022 at 7:09 PM Yongji Xie <xieyongji at bytedance.com> wrote:> > On Wed, Jul 13, 2022 at 1:44 PM Jason Wang <jasowang at redhat.com> wrote: > > > > > > ? 2022/7/6 13:04, Xie Yongji ??: > > > Taking iotlb lock to access bounce page in page fault > > > handler is meaningless since vduse_domain_free_bounce_pages() > > > would only be called during file release. > > > > > > Signed-off-by: Xie Yongji <xieyongji at bytedance.com> > > > --- > > > drivers/vdpa/vdpa_user/iova_domain.c | 7 ++----- > > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/vdpa/vdpa_user/iova_domain.c b/drivers/vdpa/vdpa_user/iova_domain.c > > > index 6daa3978d290..bca1f0b8850c 100644 > > > --- a/drivers/vdpa/vdpa_user/iova_domain.c > > > +++ b/drivers/vdpa/vdpa_user/iova_domain.c > > > @@ -211,17 +211,14 @@ static struct page * > > > vduse_domain_get_bounce_page(struct vduse_iova_domain *domain, u64 iova) > > > { > > > struct vduse_bounce_map *map; > > > - struct page *page = NULL; > > > + struct page *page; > > > > > > - spin_lock(&domain->iotlb_lock); > > > map = &domain->bounce_maps[iova >> PAGE_SHIFT]; > > > if (!map->bounce_page) > > > - goto out; > > > + return NULL; > > > > > > Interesting, I wonder why we don't serialize with > > vduse_domain_map_bounce_page() with iotlb_lock? > > > > Userspace should only access the bounce page after we set up the dma > mapping, so we don't need serialization from the iotlb_lock in this > case.What about the buggy/malicious user space that tries to access those pages before or just in the middle of it has been mapped?> And vduse_domain_map_bounce_page() only sets the > map->bounce_page rather than clears the map->bounce_page, we would not > have any problem without the lock protection.Probably, I see an assignment of orig_phys after the alloc_page() but it seems only used in bouncing which will only be called by dma ops. At least we'd better have a comment to explain the synchronization here. Thanks> > Thanks, > Yongji >