Messages in this thread | | | From | Yongji Xie <> | Date | Thu, 14 Jul 2022 14:06:28 +0800 | Subject | Re: [PATCH v2 1/5] vduse: Remove unnecessary spin lock protection |
| |
On Thu, Jul 14, 2022 at 10:27 AM Jason Wang <jasowang@redhat.com> wrote: > > On Wed, Jul 13, 2022 at 7:09 PM Yongji Xie <xieyongji@bytedance.com> wrote: > > > > On Wed, Jul 13, 2022 at 1:44 PM Jason Wang <jasowang@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@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? >
Yes, it might happen. But it would not have any problem. The userspace might get a page or a SIGBUS error.
> > 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. >
Yes, only map->bounce_page might be accessed concurrently in those two places. I will add some comments in them.
Thanks, Yongji
| |