| Message ID | 20201008112342.9394-1-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] mm: introduce vma_set_file function v2 | expand |
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_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 > */ > -- > 2.17.1 >
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_set_file-function-v2/20201008-192433
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/839555b050d42ba052565bb71a11223e3d649c7a
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_set_file-function-v2/20201008-192433
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/839555b050d42ba052565bb71a11223e3d649c7a
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 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_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 >> */ >> -- >> 2.17.1 >>
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,
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_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 > > > */ > > > -- > > > 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
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 */
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(-)