| Message ID | 20160930135959.9159-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 30, 2016 at 02:59:59PM +0100, Chris Wilson wrote: > In order to keep the dmabuf alive whilst the mmap is, we need to hold a > reference to the dmabuf and not the backing object. This is important as > the dmabuf not only keeps the object alive, but also the device so that > > dmabuf = vgem_create_dmabuf(); > ptr = mmap(... dmabuf ...); > close(dmabuf); > > persists across module-unload as well as device closure. I don't see where we grab the ref to the dma-buf here instead of the backing storage. And doesn't the exact same issue happen when you use dumb mmap? Or maybe I'm just a bit confused about what's going on here ... -Daniel > > Testcase: igt/vgem_basic/unload > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Petri Latvala <petri.latvala@intel.com> > --- > drivers/gpu/drm/vgem/vgem_drv.c | 17 +++-------------- > 1 file changed, 3 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c > index f36c14729b55..74a83e41efa9 100644 > --- a/drivers/gpu/drm/vgem/vgem_drv.c > +++ b/drivers/gpu/drm/vgem/vgem_drv.c > @@ -198,7 +198,6 @@ static struct drm_ioctl_desc vgem_ioctls[] = { > > static int vgem_mmap(struct file *filp, struct vm_area_struct *vma) > { > - unsigned long flags = vma->vm_flags; > int ret; > > ret = drm_gem_mmap(filp, vma); > @@ -208,7 +207,7 @@ static int vgem_mmap(struct file *filp, struct vm_area_struct *vma) > /* Keep the WC mmaping set by drm_gem_mmap() but our pages > * are ordinary and not special. > */ > - vma->vm_flags = flags | VM_DONTEXPAND | VM_DONTDUMP; > + vma->vm_flags &= ~(VM_IO | VM_PFNMAP); > return 0; > } > > @@ -281,21 +280,11 @@ static int vgem_prime_mmap(struct drm_gem_object *obj, > { > int ret; > > - if (obj->size < vma->vm_end - vma->vm_start) > - return -EINVAL; > - > - if (!obj->filp) > - return -ENODEV; > - > - ret = obj->filp->f_op->mmap(obj->filp, vma); > + ret = drm_gem_mmap_obj(obj, obj->size, vma); > if (ret) > return ret; > > - fput(vma->vm_file); > - vma->vm_file = get_file(obj->filp); > - vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP; > - vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags)); > - > + vma->vm_flags &= ~(VM_IO | VM_PFNMAP); > return 0; > } > > -- > 2.9.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Oct 05, 2016 at 03:11:00PM +0200, Daniel Vetter wrote: > On Fri, Sep 30, 2016 at 02:59:59PM +0100, Chris Wilson wrote: > > In order to keep the dmabuf alive whilst the mmap is, we need to hold a > > reference to the dmabuf and not the backing object. This is important as > > the dmabuf not only keeps the object alive, but also the device so that > > > > dmabuf = vgem_create_dmabuf(); > > ptr = mmap(... dmabuf ...); > > close(dmabuf); > > > > persists across module-unload as well as device closure. > > I don't see where we grab the ref to the dma-buf here instead of the > backing storage. And doesn't the exact same issue happen when you use dumb > mmap? Or maybe I'm just a bit confused about what's going on here ... The reference to the dmabuf is in vma->vm_file, which was used to keep the module alive. So this might be overzealous, in that there shouldn't be a way to get back from the shmemfs object to the module. However, that does make me aware of a failure path we have in patches that do add callbacks from shmemfs to the driver... At least using the ptr after closing the module is ok. So this patch is not required. -Chris
On Wed, Oct 05, 2016 at 02:40:09PM +0100, Chris Wilson wrote: > On Wed, Oct 05, 2016 at 03:11:00PM +0200, Daniel Vetter wrote: > > On Fri, Sep 30, 2016 at 02:59:59PM +0100, Chris Wilson wrote: > > > In order to keep the dmabuf alive whilst the mmap is, we need to hold a > > > reference to the dmabuf and not the backing object. This is important as > > > the dmabuf not only keeps the object alive, but also the device so that > > > > > > dmabuf = vgem_create_dmabuf(); > > > ptr = mmap(... dmabuf ...); > > > close(dmabuf); > > > > > > persists across module-unload as well as device closure. > > > > I don't see where we grab the ref to the dma-buf here instead of the > > backing storage. And doesn't the exact same issue happen when you use dumb > > mmap? Or maybe I'm just a bit confused about what's going on here ... > > The reference to the dmabuf is in vma->vm_file, which was used to keep > the module alive. So this might be overzealous, in that there shouldn't > be a way to get back from the shmemfs object to the module. However, > that does make me aware of a failure path we have in patches that do add > callbacks from shmemfs to the driver... > > At least using the ptr after closing the module is ok. So this patch is > not required. I think if we entirely redirect the mmap to shmem, and not forget to also update vm_file, the driver could get unloaded without ill effects. I'll leave this one out for now, picked up 1&2 from v4 to -misc meanwhile. -Daniel
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index f36c14729b55..74a83e41efa9 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -198,7 +198,6 @@ static struct drm_ioctl_desc vgem_ioctls[] = { static int vgem_mmap(struct file *filp, struct vm_area_struct *vma) { - unsigned long flags = vma->vm_flags; int ret; ret = drm_gem_mmap(filp, vma); @@ -208,7 +207,7 @@ static int vgem_mmap(struct file *filp, struct vm_area_struct *vma) /* Keep the WC mmaping set by drm_gem_mmap() but our pages * are ordinary and not special. */ - vma->vm_flags = flags | VM_DONTEXPAND | VM_DONTDUMP; + vma->vm_flags &= ~(VM_IO | VM_PFNMAP); return 0; } @@ -281,21 +280,11 @@ static int vgem_prime_mmap(struct drm_gem_object *obj, { int ret; - if (obj->size < vma->vm_end - vma->vm_start) - return -EINVAL; - - if (!obj->filp) - return -ENODEV; - - ret = obj->filp->f_op->mmap(obj->filp, vma); + ret = drm_gem_mmap_obj(obj, obj->size, vma); if (ret) return ret; - fput(vma->vm_file); - vma->vm_file = get_file(obj->filp); - vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP; - vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags)); - + vma->vm_flags &= ~(VM_IO | VM_PFNMAP); return 0; }
In order to keep the dmabuf alive whilst the mmap is, we need to hold a reference to the dmabuf and not the backing object. This is important as the dmabuf not only keeps the object alive, but also the device so that dmabuf = vgem_create_dmabuf(); ptr = mmap(... dmabuf ...); close(dmabuf); persists across module-unload as well as device closure. Testcase: igt/vgem_basic/unload Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Petri Latvala <petri.latvala@intel.com> --- drivers/gpu/drm/vgem/vgem_drv.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)