Add the new vma_set_file() function to allow changing vma->vm_file with the necessary refcount dance.
v2: add more users of this.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/dma-buf/dma-buf.c | 16 +++++----------- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 4 +--- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 3 +-- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 4 ++-- drivers/gpu/drm/msm/msm_gem.c | 4 +--- drivers/gpu/drm/omapdrm/omap_gem.c | 3 +-- drivers/gpu/drm/vgem/vgem_drv.c | 3 +-- drivers/staging/android/ashmem.c | 5 ++--- include/linux/mm.h | 2 ++ mm/mmap.c | 16 ++++++++++++++++ 10 files changed, 32 insertions(+), 28 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index a6ba4d598f0e..e4316aa7e0f4 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1163,20 +1163,14 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, return -EINVAL;
/* readjust the vma */ - get_file(dmabuf->file); - oldfile = vma->vm_file; - vma->vm_file = dmabuf->file; + oldfile = vma_set_file(vma, dmabuf->file); vma->vm_pgoff = pgoff;
ret = dmabuf->ops->mmap(dmabuf, vma); - if (ret) { - /* restore old parameters on failure */ - vma->vm_file = oldfile; - fput(dmabuf->file); - } else { - if (oldfile) - fput(oldfile); - } + /* restore old parameters on failure */ + if (ret) + vma_set_file(vma, oldfile); + return ret;
} diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index 312e9d58d5a7..10ce267c0947 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -145,10 +145,8 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, * address_space (so unmap_mapping_range does what we want, * in particular in the case of mmap'd dmabufs) */ - fput(vma->vm_file); - get_file(etnaviv_obj->base.filp); vma->vm_pgoff = 0; - vma->vm_file = etnaviv_obj->base.filp; + vma_set_file(vma, etnaviv_obj->base.filp);
vma->vm_page_prot = vm_page_prot; } diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index fec0e1e3dc3e..8ce4c9e28b87 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -119,8 +119,7 @@ static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct * if (ret) return ret;
- fput(vma->vm_file); - vma->vm_file = get_file(obj->base.filp); + vma_set_file(vma, obj->base.filp);
return 0; } diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index 3d69e51f3e4d..c9d5f1a38af3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -893,8 +893,8 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma) * requires avoiding extraneous references to their filp, hence why * we prefer to use an anonymous file for their mmaps. */ - fput(vma->vm_file); - vma->vm_file = anon; + vma_set_file(vma, anon); + fput(anon);
switch (mmo->mmap_type) { case I915_MMAP_TYPE_WC: diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index de915ff6f4b4..a71f42870d5e 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -223,10 +223,8 @@ int msm_gem_mmap_obj(struct drm_gem_object *obj, * address_space (so unmap_mapping_range does what we want, * in particular in the case of mmap'd dmabufs) */ - fput(vma->vm_file); - get_file(obj->filp); vma->vm_pgoff = 0; - vma->vm_file = obj->filp; + vma_set_file(vma, obj->filp);
vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); } diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 979d53a93c2b..0d4542ff1d7d 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -564,9 +564,8 @@ int omap_gem_mmap_obj(struct drm_gem_object *obj, * address_space (so unmap_mapping_range does what we want, * in particular in the case of mmap'd dmabufs) */ - fput(vma->vm_file); vma->vm_pgoff = 0; - vma->vm_file = get_file(obj->filp); + vma_set_file(vma, obj->filp);
vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); } diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index fa54a6d1403d..ea0eecae5153 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -397,8 +397,7 @@ static int vgem_prime_mmap(struct drm_gem_object *obj, if (ret) return ret;
- fput(vma->vm_file); - vma->vm_file = get_file(obj->filp); + vma_set_file(vma, obj->filp); vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP; vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c index 10b4be1f3e78..a51dc089896e 100644 --- a/drivers/staging/android/ashmem.c +++ b/drivers/staging/android/ashmem.c @@ -450,9 +450,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma) vma_set_anonymous(vma); }
- if (vma->vm_file) - fput(vma->vm_file); - vma->vm_file = asma->file; + vma_set_file(vma, asma->file); + fput(asma->file);
out: mutex_unlock(&ashmem_mutex); diff --git a/include/linux/mm.h b/include/linux/mm.h index ca6e6a81576b..a558602afe1b 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2693,6 +2693,8 @@ static inline void vma_set_page_prot(struct vm_area_struct *vma) } #endif
+struct file *vma_set_file(struct vm_area_struct *vma, struct file *file); + #ifdef CONFIG_NUMA_BALANCING unsigned long change_prot_numa(struct vm_area_struct *vma, unsigned long start, unsigned long end); diff --git a/mm/mmap.c b/mm/mmap.c index 40248d84ad5f..d3c3c510f643 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -136,6 +136,22 @@ void vma_set_page_prot(struct vm_area_struct *vma) WRITE_ONCE(vma->vm_page_prot, vm_page_prot); }
+/* + * Change backing file, only valid to use during initial VMA setup. + */ +struct file *vma_set_file(struct vm_area_struct *vma, struct file *file) +{ + if (file) + get_file(file); + + swap(vma->vm_file, file); + + if (file) + fput(file); + + return file; +} + /* * Requires inode->i_mapping->i_mmap_rwsem */
We have reoccurring requests on this so better document that this approach doesn't work and dma_buf_mmap() needs to be used instead.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/drm_prime.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 4910c446db83..16fa2bfc271e 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -956,7 +956,7 @@ EXPORT_SYMBOL(drm_gem_prime_import); /** * drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array * @sgt: scatter-gather table to convert - * @pages: optional array of page pointers to store the page array in + * @pages: deprecated array of page pointers to store the page array in * @addrs: optional array to store the dma bus address of each page * @max_entries: size of both the passed-in arrays * @@ -965,6 +965,11 @@ EXPORT_SYMBOL(drm_gem_prime_import); * * Drivers can use this in their &drm_driver.gem_prime_import_sg_table * implementation. + * + * Specifying the pages array is deprecated and strongly discouraged for new + * drivers. The pages array is only useful for page faults and those can + * corrupt fields in the struct page if they are not handled by the exporting + * driver. */ int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages, dma_addr_t *addrs, int max_entries)
On Thu, Oct 08, 2020 at 01:23:40PM +0200, Christian König wrote:
We have reoccurring requests on this so better document that this approach doesn't work and dma_buf_mmap() needs to be used instead.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/drm_prime.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 4910c446db83..16fa2bfc271e 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -956,7 +956,7 @@ EXPORT_SYMBOL(drm_gem_prime_import); /**
- drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array
- @sgt: scatter-gather table to convert
- @pages: optional array of page pointers to store the page array in
- @pages: deprecated array of page pointers to store the page array in
- @addrs: optional array to store the dma bus address of each page
- @max_entries: size of both the passed-in arrays
@@ -965,6 +965,11 @@ EXPORT_SYMBOL(drm_gem_prime_import);
- Drivers can use this in their &drm_driver.gem_prime_import_sg_table
- implementation.
- Specifying the pages array is deprecated and strongly discouraged for new
- drivers. The pages array is only useful for page faults and those can
- corrupt fields in the struct page if they are not handled by the exporting
*/
- driver.
I'd make this a _lot_ stronger: Aside from amdgpu and radeon all drivers using this only need it for the pages array. Imo just open-code the sg table walking loop in amdgpu/radeon (it's really not much code), and then drop the dma_addr_t parameter from this function here (it's set to NULL by everyone else).
And then deprecate this entire function here with a big warning that a) dma_buf_map_attachment is allowed to leave the struct page pointers NULL and b) this breaks mmap, users must call dma_buf_mmap instead.
Also maybe make it an uppercase DEPRECATED or something like that :-) -Daniel
int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages, dma_addr_t *addrs, int max_entries) -- 2.17.1
On Thu, Oct 08, 2020 at 04:09:14PM +0200, Daniel Vetter wrote:
On Thu, Oct 08, 2020 at 01:23:40PM +0200, Christian König wrote:
We have reoccurring requests on this so better document that this approach doesn't work and dma_buf_mmap() needs to be used instead.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/drm_prime.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 4910c446db83..16fa2bfc271e 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -956,7 +956,7 @@ EXPORT_SYMBOL(drm_gem_prime_import); /**
- drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array
- @sgt: scatter-gather table to convert
- @pages: optional array of page pointers to store the page array in
- @pages: deprecated array of page pointers to store the page array in
- @addrs: optional array to store the dma bus address of each page
- @max_entries: size of both the passed-in arrays
@@ -965,6 +965,11 @@ EXPORT_SYMBOL(drm_gem_prime_import);
- Drivers can use this in their &drm_driver.gem_prime_import_sg_table
- implementation.
- Specifying the pages array is deprecated and strongly discouraged for new
- drivers. The pages array is only useful for page faults and those can
- corrupt fields in the struct page if they are not handled by the exporting
*/
- driver.
I'd make this a _lot_ stronger: Aside from amdgpu and radeon all drivers using this only need it for the pages array. Imo just open-code the sg table walking loop in amdgpu/radeon (it's really not much code), and then drop the dma_addr_t parameter from this function here (it's set to NULL by everyone else).
And then deprecate this entire function here with a big warning that a) dma_buf_map_attachment is allowed to leave the struct page pointers NULL and b) this breaks mmap, users must call dma_buf_mmap instead.
Also maybe make it an uppercase DEPRECATED or something like that :-)
OK I just realized I missed nouveau. That would be 3 places where we need to stuff the dma_addr_t list into something ttm can take. Still feels better than this half-deprecated function kludge ... -Daniel
-Daniel
int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages, dma_addr_t *addrs, int max_entries) -- 2.17.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Am 08.10.20 um 16:14 schrieb Daniel Vetter:
On Thu, Oct 08, 2020 at 04:09:14PM +0200, Daniel Vetter wrote:
On Thu, Oct 08, 2020 at 01:23:40PM +0200, Christian König wrote:
We have reoccurring requests on this so better document that this approach doesn't work and dma_buf_mmap() needs to be used instead.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/drm_prime.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 4910c446db83..16fa2bfc271e 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -956,7 +956,7 @@ EXPORT_SYMBOL(drm_gem_prime_import); /**
- drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array
- @sgt: scatter-gather table to convert
- @pages: optional array of page pointers to store the page array in
- @pages: deprecated array of page pointers to store the page array in
- @addrs: optional array to store the dma bus address of each page
- @max_entries: size of both the passed-in arrays
@@ -965,6 +965,11 @@ EXPORT_SYMBOL(drm_gem_prime_import);
- Drivers can use this in their &drm_driver.gem_prime_import_sg_table
- implementation.
- Specifying the pages array is deprecated and strongly discouraged for new
- drivers. The pages array is only useful for page faults and those can
- corrupt fields in the struct page if they are not handled by the exporting
*/
- driver.
I'd make this a _lot_ stronger: Aside from amdgpu and radeon all drivers using this only need it for the pages array. Imo just open-code the sg table walking loop in amdgpu/radeon (it's really not much code), and then drop the dma_addr_t parameter from this function here (it's set to NULL by everyone else).
And then deprecate this entire function here with a big warning that a) dma_buf_map_attachment is allowed to leave the struct page pointers NULL and b) this breaks mmap, users must call dma_buf_mmap instead.
Also maybe make it an uppercase DEPRECATED or something like that :-)
OK I just realized I missed nouveau. That would be 3 places where we need to stuff the dma_addr_t list into something ttm can take. Still feels better than this half-deprecated function kludge ...
Mhm, I don't see a reason why nouveau would need the struct page either.
How about we split that up into two function?
One for converting the sg_table into a linear dma_addr array.
And one for converting the sg_table into a linear struct page array with a __deprecated attribute on it?
Christian.
-Daniel
-Daniel
int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages, dma_addr_t *addrs, int max_entries) -- 2.17.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Fri, Oct 09, 2020 at 09:36:41AM +0200, Christian König wrote:
Am 08.10.20 um 16:14 schrieb Daniel Vetter:
On Thu, Oct 08, 2020 at 04:09:14PM +0200, Daniel Vetter wrote:
On Thu, Oct 08, 2020 at 01:23:40PM +0200, Christian König wrote:
We have reoccurring requests on this so better document that this approach doesn't work and dma_buf_mmap() needs to be used instead.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/drm_prime.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 4910c446db83..16fa2bfc271e 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -956,7 +956,7 @@ EXPORT_SYMBOL(drm_gem_prime_import); /**
- drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array
- @sgt: scatter-gather table to convert
- @pages: optional array of page pointers to store the page array in
- @pages: deprecated array of page pointers to store the page array in
- @addrs: optional array to store the dma bus address of each page
- @max_entries: size of both the passed-in arrays
@@ -965,6 +965,11 @@ EXPORT_SYMBOL(drm_gem_prime_import);
- Drivers can use this in their &drm_driver.gem_prime_import_sg_table
- implementation.
- Specifying the pages array is deprecated and strongly discouraged for new
- drivers. The pages array is only useful for page faults and those can
- corrupt fields in the struct page if they are not handled by the exporting
*/
- driver.
I'd make this a _lot_ stronger: Aside from amdgpu and radeon all drivers using this only need it for the pages array. Imo just open-code the sg table walking loop in amdgpu/radeon (it's really not much code), and then drop the dma_addr_t parameter from this function here (it's set to NULL by everyone else).
And then deprecate this entire function here with a big warning that a) dma_buf_map_attachment is allowed to leave the struct page pointers NULL and b) this breaks mmap, users must call dma_buf_mmap instead.
Also maybe make it an uppercase DEPRECATED or something like that :-)
OK I just realized I missed nouveau. That would be 3 places where we need to stuff the dma_addr_t list into something ttm can take. Still feels better than this half-deprecated function kludge ...
Mhm, I don't see a reason why nouveau would need the struct page either.
How about we split that up into two function?
One for converting the sg_table into a linear dma_addr array.
And one for converting the sg_table into a linear struct page array with a __deprecated attribute on it?
Yeah makes sense, since converting ttm to just use sgt iterations directly everywhere is probably a bit too much. Maybe keep that converter in ttm code, since outside of ttm the rough consensus is to converge on sgt for handling buffers. Well, for those drivers not stuck on struct page arrays :-) -Daniel
Christian.
-Daniel
-Daniel
int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages, dma_addr_t *addrs, int max_entries) -- 2.17.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
This is deprecated.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/radeon/radeon_ttm.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index 63e38b05a5bc..4b92cdbcd29b 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -474,8 +474,8 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_bo_device *bdev, struct ttm_tt * if (r) goto release_sg;
- drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages, - gtt->ttm.dma_address, ttm->num_pages); + drm_prime_sg_to_page_addr_arrays(ttm->sg, NULL, gtt->ttm.dma_address, + ttm->num_pages);
return 0;
@@ -642,8 +642,9 @@ static int radeon_ttm_tt_populate(struct ttm_bo_device *bdev, }
if (slave && ttm->sg) { - drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages, - gtt->ttm.dma_address, ttm->num_pages); + drm_prime_sg_to_page_addr_arrays(ttm->sg, NULL, + gtt->ttm.dma_address, + ttm->num_pages); ttm_tt_set_populated(ttm); return 0; }
This is deprecated.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 399961035ae6..ac463e706b19 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -1011,8 +1011,8 @@ static int amdgpu_ttm_tt_pin_userptr(struct ttm_bo_device *bdev, goto release_sg;
/* convert SG to linear array of pages and dma addresses */ - drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages, - gtt->ttm.dma_address, ttm->num_pages); + drm_prime_sg_to_page_addr_arrays(ttm->sg, NULL, gtt->ttm.dma_address, + ttm->num_pages);
return 0;
@@ -1345,7 +1345,7 @@ static int amdgpu_ttm_tt_populate(struct ttm_bo_device *bdev, ttm->sg = sgt; }
- drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages, + drm_prime_sg_to_page_addr_arrays(ttm->sg, NULL, gtt->ttm.dma_address, ttm->num_pages); ttm_tt_set_populated(ttm);
On Thu, Oct 08, 2020 at 01:23:39PM +0200, Christian König wrote:
drivers/dma-buf/dma-buf.c | 16 +++++----------- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 4 +--- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 3 +-- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 4 ++-- drivers/gpu/drm/msm/msm_gem.c | 4 +--- drivers/gpu/drm/omapdrm/omap_gem.c | 3 +-- drivers/gpu/drm/vgem/vgem_drv.c | 3 +-- drivers/staging/android/ashmem.c | 5 ++---
...
+++ b/mm/mmap.c @@ -136,6 +136,22 @@ void vma_set_page_prot(struct vm_area_struct *vma) WRITE_ONCE(vma->vm_page_prot, vm_page_prot); }
+/*
- Change backing file, only valid to use during initial VMA setup.
- */
+struct file *vma_set_file(struct vm_area_struct *vma, struct file *file) +{
- if (file)
get_file(file);- swap(vma->vm_file, file);
- if (file)
fput(file);- return file;
+}
These users are all potentially modules. You need an EXPORT_SYMBOL()?
Am 08.10.20 um 13:39 schrieb Matthew Wilcox:
On Thu, Oct 08, 2020 at 01:23:39PM +0200, Christian König wrote:
drivers/dma-buf/dma-buf.c | 16 +++++----------- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 4 +--- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 3 +-- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 4 ++-- drivers/gpu/drm/msm/msm_gem.c | 4 +--- drivers/gpu/drm/omapdrm/omap_gem.c | 3 +-- drivers/gpu/drm/vgem/vgem_drv.c | 3 +-- drivers/staging/android/ashmem.c | 5 ++---
...
+++ b/mm/mmap.c @@ -136,6 +136,22 @@ void vma_set_page_prot(struct vm_area_struct *vma) WRITE_ONCE(vma->vm_page_prot, vm_page_prot); }
+/*
- Change backing file, only valid to use during initial VMA setup.
- */
+struct file *vma_set_file(struct vm_area_struct *vma, struct file *file) +{
- if (file)
get_file(file);- swap(vma->vm_file, file);
- if (file)
fput(file);- return file;
+}
These users are all potentially modules. You need an EXPORT_SYMBOL()?
Oh, good point. Yeah I totally missed that. The initial DMA-buf use case was not inside a module.
Thanks, Christian.
On Thu, Oct 08, 2020 at 01:23:39PM +0200, Christian König wrote:
Add the new vma_set_file() function to allow changing vma->vm_file with the necessary refcount dance.
v2: add more users of this.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/dma-buf/dma-buf.c | 16 +++++----------- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 4 +--- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 3 +-- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 4 ++-- drivers/gpu/drm/msm/msm_gem.c | 4 +--- drivers/gpu/drm/omapdrm/omap_gem.c | 3 +-- drivers/gpu/drm/vgem/vgem_drv.c | 3 +-- drivers/staging/android/ashmem.c | 5 ++--- include/linux/mm.h | 2 ++ mm/mmap.c | 16 ++++++++++++++++ 10 files changed, 32 insertions(+), 28 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index a6ba4d598f0e..e4316aa7e0f4 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1163,20 +1163,14 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, return -EINVAL;
/* readjust the vma */
- get_file(dmabuf->file);
- oldfile = vma->vm_file;
- vma->vm_file = dmabuf->file;
oldfile = vma_set_file(vma, dmabuf->file); vma->vm_pgoff = pgoff;
ret = dmabuf->ops->mmap(dmabuf, vma);
- if (ret) {
/* restore old parameters on failure */vma->vm_file = oldfile;fput(dmabuf->file);- } else {
if (oldfile)fput(oldfile);- }
- /* restore old parameters on failure */
- if (ret)
vma_set_file(vma, oldfile);
I think these two lines here are cargo-cult: If this fails, the mmap fails and therefore the vma structure is kfreed. No point at all in restoring anything.
With that: Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
- return ret;
} diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index 312e9d58d5a7..10ce267c0947 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -145,10 +145,8 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, * address_space (so unmap_mapping_range does what we want, * in particular in the case of mmap'd dmabufs) */
fput(vma->vm_file);get_file(etnaviv_obj->base.filp);vma->vm_file = etnaviv_obj->base.filp;
vma_set_file(vma, etnaviv_obj->base.filp);vma->vm_page_prot = vm_page_prot; }
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index fec0e1e3dc3e..8ce4c9e28b87 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -119,8 +119,7 @@ static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct * if (ret) return ret;
- fput(vma->vm_file);
- vma->vm_file = get_file(obj->base.filp);
vma_set_file(vma, obj->base.filp);
return 0;
} diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index 3d69e51f3e4d..c9d5f1a38af3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -893,8 +893,8 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma) * requires avoiding extraneous references to their filp, hence why * we prefer to use an anonymous file for their mmaps. */
- fput(vma->vm_file);
- vma->vm_file = anon;
vma_set_file(vma, anon);
fput(anon);
switch (mmo->mmap_type) { case I915_MMAP_TYPE_WC:
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index de915ff6f4b4..a71f42870d5e 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -223,10 +223,8 @@ int msm_gem_mmap_obj(struct drm_gem_object *obj, * address_space (so unmap_mapping_range does what we want, * in particular in the case of mmap'd dmabufs) */
fput(vma->vm_file);get_file(obj->filp);vma->vm_file = obj->filp;
vma_set_file(vma, obj->filp);vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); }
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 979d53a93c2b..0d4542ff1d7d 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -564,9 +564,8 @@ int omap_gem_mmap_obj(struct drm_gem_object *obj, * address_space (so unmap_mapping_range does what we want, * in particular in the case of mmap'd dmabufs) */
fput(vma->vm_file);vma->vm_file = get_file(obj->filp);
vma_set_file(vma, obj->filp);vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); }
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index fa54a6d1403d..ea0eecae5153 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -397,8 +397,7 @@ static int vgem_prime_mmap(struct drm_gem_object *obj, if (ret) return ret;
- fput(vma->vm_file);
- vma->vm_file = get_file(obj->filp);
- vma_set_file(vma, obj->filp); vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP; vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c index 10b4be1f3e78..a51dc089896e 100644 --- a/drivers/staging/android/ashmem.c +++ b/drivers/staging/android/ashmem.c @@ -450,9 +450,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma) vma_set_anonymous(vma); }
- if (vma->vm_file)
fput(vma->vm_file);- vma->vm_file = asma->file;
- vma_set_file(vma, asma->file);
- fput(asma->file);
out: mutex_unlock(&ashmem_mutex); diff --git a/include/linux/mm.h b/include/linux/mm.h index ca6e6a81576b..a558602afe1b 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2693,6 +2693,8 @@ static inline void vma_set_page_prot(struct vm_area_struct *vma) } #endif
+struct file *vma_set_file(struct vm_area_struct *vma, struct file *file);
#ifdef CONFIG_NUMA_BALANCING unsigned long change_prot_numa(struct vm_area_struct *vma, unsigned long start, unsigned long end); diff --git a/mm/mmap.c b/mm/mmap.c index 40248d84ad5f..d3c3c510f643 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -136,6 +136,22 @@ void vma_set_page_prot(struct vm_area_struct *vma) WRITE_ONCE(vma->vm_page_prot, vm_page_prot); }
+/*
- Change backing file, only valid to use during initial VMA setup.
- */
+struct file *vma_set_file(struct vm_area_struct *vma, struct file *file) +{
- if (file)
get_file(file);- swap(vma->vm_file, file);
- if (file)
fput(file);- return file;
+}
/*
- Requires inode->i_mapping->i_mmap_rwsem
*/
2.17.1
Am 08.10.20 um 16:12 schrieb Daniel Vetter:
On Thu, Oct 08, 2020 at 01:23:39PM +0200, Christian König wrote:
Add the new vma_set_file() function to allow changing vma->vm_file with the necessary refcount dance.
v2: add more users of this.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/dma-buf/dma-buf.c | 16 +++++----------- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 4 +--- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 3 +-- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 4 ++-- drivers/gpu/drm/msm/msm_gem.c | 4 +--- drivers/gpu/drm/omapdrm/omap_gem.c | 3 +-- drivers/gpu/drm/vgem/vgem_drv.c | 3 +-- drivers/staging/android/ashmem.c | 5 ++--- include/linux/mm.h | 2 ++ mm/mmap.c | 16 ++++++++++++++++ 10 files changed, 32 insertions(+), 28 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index a6ba4d598f0e..e4316aa7e0f4 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1163,20 +1163,14 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, return -EINVAL;
/* readjust the vma */
- get_file(dmabuf->file);
- oldfile = vma->vm_file;
- vma->vm_file = dmabuf->file;
oldfile = vma_set_file(vma, dmabuf->file); vma->vm_pgoff = pgoff;
ret = dmabuf->ops->mmap(dmabuf, vma);
- if (ret) {
/* restore old parameters on failure */vma->vm_file = oldfile;fput(dmabuf->file);- } else {
if (oldfile)fput(oldfile);- }
- /* restore old parameters on failure */
- if (ret)
vma_set_file(vma, oldfile);I think these two lines here are cargo-cult: If this fails, the mmap fails and therefore the vma structure is kfreed. No point at all in restoring anything.
This was explicitly added with this patch to fix a problem:
commit 495c10cc1c0c359871d5bef32dd173252fc17995 Author: John Sheu sheu@google.com Date: Mon Feb 11 17:50:24 2013 -0800
CHROMIUM: dma-buf: restore args on failure of dma_buf_mmap
Callers to dma_buf_mmap expect to fput() the vma struct's vm_file themselves on failure. Not restoring the struct's data on failure causes a double-decrement of the vm_file's refcount.
With that: Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Can I keep that even with the error handling working? :)
Christian.
return ret;
}
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index 312e9d58d5a7..10ce267c0947 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -145,10 +145,8 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, * address_space (so unmap_mapping_range does what we want, * in particular in the case of mmap'd dmabufs) */
fput(vma->vm_file);get_file(etnaviv_obj->base.filp);vma->vm_file = etnaviv_obj->base.filp;
vma_set_file(vma, etnaviv_obj->base.filp);vma->vm_page_prot = vm_page_prot; }
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index fec0e1e3dc3e..8ce4c9e28b87 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -119,8 +119,7 @@ static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct * if (ret) return ret;
- fput(vma->vm_file);
- vma->vm_file = get_file(obj->base.filp);
vma_set_file(vma, obj->base.filp);
return 0; }
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index 3d69e51f3e4d..c9d5f1a38af3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -893,8 +893,8 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma) * requires avoiding extraneous references to their filp, hence why * we prefer to use an anonymous file for their mmaps. */
- fput(vma->vm_file);
- vma->vm_file = anon;
vma_set_file(vma, anon);
fput(anon);
switch (mmo->mmap_type) { case I915_MMAP_TYPE_WC:
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index de915ff6f4b4..a71f42870d5e 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -223,10 +223,8 @@ int msm_gem_mmap_obj(struct drm_gem_object *obj, * address_space (so unmap_mapping_range does what we want, * in particular in the case of mmap'd dmabufs) */
fput(vma->vm_file);get_file(obj->filp);vma->vm_file = obj->filp;
vma_set_file(vma, obj->filp);vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); }
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 979d53a93c2b..0d4542ff1d7d 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -564,9 +564,8 @@ int omap_gem_mmap_obj(struct drm_gem_object *obj, * address_space (so unmap_mapping_range does what we want, * in particular in the case of mmap'd dmabufs) */
fput(vma->vm_file);vma->vm_file = get_file(obj->filp);
vma_set_file(vma, obj->filp);vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); }
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index fa54a6d1403d..ea0eecae5153 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -397,8 +397,7 @@ static int vgem_prime_mmap(struct drm_gem_object *obj, if (ret) return ret;
- fput(vma->vm_file);
- vma->vm_file = get_file(obj->filp);
- vma_set_file(vma, obj->filp); vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP; vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c index 10b4be1f3e78..a51dc089896e 100644 --- a/drivers/staging/android/ashmem.c +++ b/drivers/staging/android/ashmem.c @@ -450,9 +450,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma) vma_set_anonymous(vma); }
- if (vma->vm_file)
fput(vma->vm_file);- vma->vm_file = asma->file;
vma_set_file(vma, asma->file);
fput(asma->file);
out: mutex_unlock(&ashmem_mutex);
diff --git a/include/linux/mm.h b/include/linux/mm.h index ca6e6a81576b..a558602afe1b 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2693,6 +2693,8 @@ static inline void vma_set_page_prot(struct vm_area_struct *vma) } #endif
+struct file *vma_set_file(struct vm_area_struct *vma, struct file *file);
- #ifdef CONFIG_NUMA_BALANCING unsigned long change_prot_numa(struct vm_area_struct *vma, unsigned long start, unsigned long end);
diff --git a/mm/mmap.c b/mm/mmap.c index 40248d84ad5f..d3c3c510f643 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -136,6 +136,22 @@ void vma_set_page_prot(struct vm_area_struct *vma) WRITE_ONCE(vma->vm_page_prot, vm_page_prot); }
+/*
- Change backing file, only valid to use during initial VMA setup.
- */
+struct file *vma_set_file(struct vm_area_struct *vma, struct file *file) +{
- if (file)
get_file(file);- swap(vma->vm_file, file);
- if (file)
fput(file);- return file;
+}
- /*
*/
- Requires inode->i_mapping->i_mmap_rwsem
-- 2.17.1
On Fri, Oct 09, 2020 at 09:16:49AM +0200, Christian König wrote:
Am 08.10.20 um 16:12 schrieb Daniel Vetter:
On Thu, Oct 08, 2020 at 01:23:39PM +0200, Christian König wrote:
Add the new vma_set_file() function to allow changing vma->vm_file with the necessary refcount dance.
v2: add more users of this.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/dma-buf/dma-buf.c | 16 +++++----------- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 4 +--- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 3 +-- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 4 ++-- drivers/gpu/drm/msm/msm_gem.c | 4 +--- drivers/gpu/drm/omapdrm/omap_gem.c | 3 +-- drivers/gpu/drm/vgem/vgem_drv.c | 3 +-- drivers/staging/android/ashmem.c | 5 ++--- include/linux/mm.h | 2 ++ mm/mmap.c | 16 ++++++++++++++++ 10 files changed, 32 insertions(+), 28 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index a6ba4d598f0e..e4316aa7e0f4 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1163,20 +1163,14 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, return -EINVAL; /* readjust the vma */
- get_file(dmabuf->file);
- oldfile = vma->vm_file;
- vma->vm_file = dmabuf->file;
- oldfile = vma_set_file(vma, dmabuf->file); vma->vm_pgoff = pgoff; ret = dmabuf->ops->mmap(dmabuf, vma);
- if (ret) {
/* restore old parameters on failure */vma->vm_file = oldfile;fput(dmabuf->file);- } else {
if (oldfile)fput(oldfile);- }
- /* restore old parameters on failure */
- if (ret)
vma_set_file(vma, oldfile);I think these two lines here are cargo-cult: If this fails, the mmap fails and therefore the vma structure is kfreed. No point at all in restoring anything.
This was explicitly added with this patch to fix a problem:
commit 495c10cc1c0c359871d5bef32dd173252fc17995 Author: John Sheu sheu@google.com Date: Mon Feb 11 17:50:24 2013 -0800
CHROMIUM: dma-buf: restore args on failure of dma_buf_mmap
Callers to dma_buf_mmap expect to fput() the vma struct's vm_file themselves on failure. Not restoring the struct's data on failure causes a double-decrement of the vm_file's refcount.
With that: Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Can I keep that even with the error handling working? :)
Hm good find, I should have looked at git history myself.
I just noticed this here in the patch because everyone else does not do this. But looking at the mmap_region() code in mmap.c we seem to indeed have this problem for the error path:
unmap_and_free_vma: vma->vm_file = NULL; fput(file);
Note that the success path does things correctly (a bit above):
file = vma->vm_file; out:
So it indeed looks like dma-buf is the only one that does this fully correctly. So maybe we should do a follow-up patch to change the mmap_region exit code to pick up whatever vma->vm_file was set instead, and fput that?
Anyway I correct, r-b: as-is.
Cheers, Daniel
Christian.
- return ret; }
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index 312e9d58d5a7..10ce267c0947 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -145,10 +145,8 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, * address_space (so unmap_mapping_range does what we want, * in particular in the case of mmap'd dmabufs) */
fput(vma->vm_file);get_file(etnaviv_obj->base.filp);vma->vm_file = etnaviv_obj->base.filp;
vma_set_file(vma, etnaviv_obj->base.filp);diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index fec0e1e3dc3e..8ce4c9e28b87 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -119,8 +119,7 @@ static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct * if (ret) return ret;
- fput(vma->vm_file);
- vma->vm_file = get_file(obj->base.filp);
- vma_set_file(vma, obj->base.filp); return 0; }
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index 3d69e51f3e4d..c9d5f1a38af3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -893,8 +893,8 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma) * requires avoiding extraneous references to their filp, hence why * we prefer to use an anonymous file for their mmaps. */
- fput(vma->vm_file);
- vma->vm_file = anon;
- vma_set_file(vma, anon);
- fput(anon); switch (mmo->mmap_type) { case I915_MMAP_TYPE_WC:
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index de915ff6f4b4..a71f42870d5e 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -223,10 +223,8 @@ int msm_gem_mmap_obj(struct drm_gem_object *obj, * address_space (so unmap_mapping_range does what we want, * in particular in the case of mmap'd dmabufs) */
fput(vma->vm_file);get_file(obj->filp);vma->vm_file = obj->filp;
vma_set_file(vma, obj->filp);diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 979d53a93c2b..0d4542ff1d7d 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -564,9 +564,8 @@ int omap_gem_mmap_obj(struct drm_gem_object *obj, * address_space (so unmap_mapping_range does what we want, * in particular in the case of mmap'd dmabufs) */
fput(vma->vm_file);vma->vm_file = get_file(obj->filp);
vma_set_file(vma, obj->filp);diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index fa54a6d1403d..ea0eecae5153 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -397,8 +397,7 @@ static int vgem_prime_mmap(struct drm_gem_object *obj, if (ret) return ret;
- fput(vma->vm_file);
- vma->vm_file = get_file(obj->filp);
- vma_set_file(vma, obj->filp); vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP; vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c index 10b4be1f3e78..a51dc089896e 100644 --- a/drivers/staging/android/ashmem.c +++ b/drivers/staging/android/ashmem.c @@ -450,9 +450,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma) vma_set_anonymous(vma); }
- if (vma->vm_file)
fput(vma->vm_file);- vma->vm_file = asma->file;
- vma_set_file(vma, asma->file);
- fput(asma->file); out: mutex_unlock(&ashmem_mutex);
diff --git a/include/linux/mm.h b/include/linux/mm.h index ca6e6a81576b..a558602afe1b 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2693,6 +2693,8 @@ static inline void vma_set_page_prot(struct vm_area_struct *vma) } #endif +struct file *vma_set_file(struct vm_area_struct *vma, struct file *file);
- #ifdef CONFIG_NUMA_BALANCING unsigned long change_prot_numa(struct vm_area_struct *vma, unsigned long start, unsigned long end);
diff --git a/mm/mmap.c b/mm/mmap.c index 40248d84ad5f..d3c3c510f643 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -136,6 +136,22 @@ void vma_set_page_prot(struct vm_area_struct *vma) WRITE_ONCE(vma->vm_page_prot, vm_page_prot); } +/*
- Change backing file, only valid to use during initial VMA setup.
- */
+struct file *vma_set_file(struct vm_area_struct *vma, struct file *file) +{
- if (file)
get_file(file);- swap(vma->vm_file, file);
- if (file)
fput(file);- return file;
+}
- /*
*/
- Requires inode->i_mapping->i_mmap_rwsem
-- 2.17.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Oct 09, 2020 at 09:39:00AM +0200, Daniel Vetter wrote:
I just noticed this here in the patch because everyone else does not do this. But looking at the mmap_region() code in mmap.c we seem to indeed have this problem for the error path:
unmap_and_free_vma: vma->vm_file = NULL; fput(file);
Note that the success path does things correctly (a bit above):
file = vma->vm_file; out:
So it indeed looks like dma-buf is the only one that does this fully correctly. So maybe we should do a follow-up patch to change the mmap_region exit code to pick up whatever vma->vm_file was set instead, and fput that?
Given that this new vma_set_file() should be the only way to manipulate vm_file from the mmap op, I think this reflects a bug in mm/mmap.c.. Should be:
unmap_and_free_vma: fput(vma->vm_file); vma->vm_file = NULL;
Then everything works the way you'd expect without tricky error handling
Jason
Am 09.10.20 um 14:12 schrieb Jason Gunthorpe:
On Fri, Oct 09, 2020 at 09:39:00AM +0200, Daniel Vetter wrote:
I just noticed this here in the patch because everyone else does not do this. But looking at the mmap_region() code in mmap.c we seem to indeed have this problem for the error path:
unmap_and_free_vma: vma->vm_file = NULL; fput(file);
Note that the success path does things correctly (a bit above):
file = vma->vm_file; out:
So it indeed looks like dma-buf is the only one that does this fully correctly. So maybe we should do a follow-up patch to change the mmap_region exit code to pick up whatever vma->vm_file was set instead, and fput that?
Given that this new vma_set_file() should be the only way to manipulate vm_file from the mmap op, I think this reflects a bug in mm/mmap.c.. Should be:
unmap_and_free_vma: fput(vma->vm_file); vma->vm_file = NULL;
Then everything works the way you'd expect without tricky error handling
That's what Daniel suggested as well, yes.
Going to craft a separate patch for this.
Thanks, Christian.
Jason
Hi "Christian,
I love your patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next] [also build test ERROR on hnaz-linux-mm/master staging/staging-testing linux/master linus/master v5.9-rc8 next-20201008] [cannot apply to mmotm/master] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Christian-K-nig/mm-introduce-vma_se... base: git://anongit.freedesktop.org/drm-intel for-linux-next config: h8300-randconfig-r036-20201008 (attached as .config) compiler: h8300-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/839555b050d42ba052565bb71a11223e3d64... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Christian-K-nig/mm-introduce-vma_set_file-function-v2/20201008-192433 git checkout 839555b050d42ba052565bb71a11223e3d649c7a # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=h8300
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
h8300-linux-ld: section .init.text LMA [000000000048e500,00000000004cd3ef] overlaps section .text LMA [000000000000025c,0000000000b6acbf] h8300-linux-ld: section .data VMA [0000000000400000,000000000048e4ff] overlaps section .text VMA [000000000000025c,0000000000b6acbf] h8300-linux-ld: drivers/gpu/drm/vgem/vgem_drv.o: in function `vgem_prime_mmap':
drivers/gpu/drm/vgem/vgem_drv.c:396: undefined reference to `vma_set_file'
h8300-linux-ld: drivers/dma-buf/dma-buf.o: in function `dma_buf_mmap': drivers/dma-buf/dma-buf.c:1166: undefined reference to `vma_set_file'
h8300-linux-ld: drivers/dma-buf/dma-buf.c:1172: undefined reference to `vma_set_file'
vim +396 drivers/gpu/drm/vgem/vgem_drv.c
380 381 static int vgem_prime_mmap(struct drm_gem_object *obj, 382 struct vm_area_struct *vma) 383 { 384 int ret; 385 386 if (obj->size < vma->vm_end - vma->vm_start) 387 return -EINVAL; 388 389 if (!obj->filp) 390 return -ENODEV; 391 392 ret = call_mmap(obj->filp, vma); 393 if (ret) 394 return ret; 395
396 vma_set_file(vma, obj->filp);
397 vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP; 398 vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags)); 399 400 return 0; 401 } 402
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi "Christian,
I love your patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next] [also build test ERROR on staging/staging-testing linux/master linus/master v5.9-rc8 next-20201008] [cannot apply to mmotm/master] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Christian-K-nig/mm-introduce-vma_se... base: git://anongit.freedesktop.org/drm-intel for-linux-next config: arm-randconfig-r021-20201008 (attached as .config) compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/839555b050d42ba052565bb71a11223e3d64... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Christian-K-nig/mm-introduce-vma_set_file-function-v2/20201008-192433 git checkout 839555b050d42ba052565bb71a11223e3d649c7a # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
arm-linux-gnueabi-ld: drivers/dma-buf/dma-buf.o: in function `dma_buf_mmap': drivers/dma-buf/dma-buf.c:1166: undefined reference to `vma_set_file'
arm-linux-gnueabi-ld: drivers/dma-buf/dma-buf.c:1172: undefined reference to `vma_set_file'
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 10/8/20 4:23 AM, Christian König wrote:
Add the new vma_set_file() function to allow changing vma->vm_file with the necessary refcount dance.
v2: add more users of this.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/dma-buf/dma-buf.c | 16 +++++----------- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 4 +--- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 3 +-- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 4 ++-- drivers/gpu/drm/msm/msm_gem.c | 4 +--- drivers/gpu/drm/omapdrm/omap_gem.c | 3 +-- drivers/gpu/drm/vgem/vgem_drv.c | 3 +-- drivers/staging/android/ashmem.c | 5 ++--- include/linux/mm.h | 2 ++ mm/mmap.c | 16 ++++++++++++++++ 10 files changed, 32 insertions(+), 28 deletions(-)
Looks like a nice cleanup. Two comments below.
...
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index 3d69e51f3e4d..c9d5f1a38af3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -893,8 +893,8 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma) * requires avoiding extraneous references to their filp, hence why * we prefer to use an anonymous file for their mmaps. */
- fput(vma->vm_file);
- vma->vm_file = anon;
- vma_set_file(vma, anon);
- fput(anon);
That's one fput() too many, isn't it?
...
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c index 10b4be1f3e78..a51dc089896e 100644 --- a/drivers/staging/android/ashmem.c +++ b/drivers/staging/android/ashmem.c @@ -450,9 +450,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma) vma_set_anonymous(vma); }
- if (vma->vm_file)
fput(vma->vm_file);- vma->vm_file = asma->file;
- vma_set_file(vma, asma->file);
- fput(asma->file);
Same here: that fput() seems wrong, as it was already done within vma_set_file().
thanks,
Am 08.10.20 um 23:49 schrieb John Hubbard:
On 10/8/20 4:23 AM, Christian König wrote:
Add the new vma_set_file() function to allow changing vma->vm_file with the necessary refcount dance.
v2: add more users of this.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/dma-buf/dma-buf.c | 16 +++++----------- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 4 +--- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 3 +-- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 4 ++-- drivers/gpu/drm/msm/msm_gem.c | 4 +--- drivers/gpu/drm/omapdrm/omap_gem.c | 3 +-- drivers/gpu/drm/vgem/vgem_drv.c | 3 +-- drivers/staging/android/ashmem.c | 5 ++--- include/linux/mm.h | 2 ++ mm/mmap.c | 16 ++++++++++++++++ 10 files changed, 32 insertions(+), 28 deletions(-)
Looks like a nice cleanup. Two comments below.
...
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index 3d69e51f3e4d..c9d5f1a38af3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -893,8 +893,8 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma) * requires avoiding extraneous references to their filp, hence why * we prefer to use an anonymous file for their mmaps. */ - fput(vma->vm_file); - vma->vm_file = anon; + vma_set_file(vma, anon); + fput(anon);
That's one fput() too many, isn't it?
No, the other cases were replacing the vm_file with something pre-allocated and also grabbed a new reference.
But this case here uses the freshly allocated anon file and so vma_set_file() grabs another extra reference which we need to drop.
The alternative is to just keep it as it is. Opinions?
...
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c index 10b4be1f3e78..a51dc089896e 100644 --- a/drivers/staging/android/ashmem.c +++ b/drivers/staging/android/ashmem.c @@ -450,9 +450,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma) vma_set_anonymous(vma); } - if (vma->vm_file) - fput(vma->vm_file); - vma->vm_file = asma->file; + vma_set_file(vma, asma->file); + fput(asma->file);
Same here: that fput() seems wrong, as it was already done within vma_set_file().
No, that case is correct as well. The Android code here has the matching get_file() a few lines up, see the surrounding code.
I didn't wanted to replace that since it does some strange error handling here, so the result is that we need to drop the extra reference as again.
We could also keep it like it is or maybe better put a TODO comment on it.
Regards, Christian.
thanks,
On 10/9/20 12:33 AM, Christian König wrote:
Am 08.10.20 um 23:49 schrieb John Hubbard:
On 10/8/20 4:23 AM, Christian König wrote: ...
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index 3d69e51f3e4d..c9d5f1a38af3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -893,8 +893,8 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma) * requires avoiding extraneous references to their filp, hence why * we prefer to use an anonymous file for their mmaps. */ - fput(vma->vm_file); - vma->vm_file = anon; + vma_set_file(vma, anon); + fput(anon);
That's one fput() too many, isn't it?
No, the other cases were replacing the vm_file with something pre-allocated and also grabbed a new reference.
But this case here uses the freshly allocated anon file and so vma_set_file() grabs another extra reference which we need to drop.
The alternative is to just keep it as it is. Opinions?
I think just a small comment for these cases, is probably about right.
...
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c index 10b4be1f3e78..a51dc089896e 100644 --- a/drivers/staging/android/ashmem.c +++ b/drivers/staging/android/ashmem.c @@ -450,9 +450,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma) vma_set_anonymous(vma); } - if (vma->vm_file) - fput(vma->vm_file); - vma->vm_file = asma->file; + vma_set_file(vma, asma->file); + fput(asma->file);
Same here: that fput() seems wrong, as it was already done within vma_set_file().
No, that case is correct as well. The Android code here has the matching get_file() a few lines up, see the surrounding code.
I didn't wanted to replace that since it does some strange error handling here, so the result is that we need to drop the extra reference as again.
We could also keep it like it is or maybe better put a TODO comment on it.
Yeah, I think a comment is a good way to go.
thanks,
dri-devel@lists.freedesktop.org
-
Christian König -
Christian König -
Daniel Vetter -
Jason Gunthorpe -
John Hubbard -
kernel test robot -
Matthew Wilcox