| Message ID | 20201012085203.56119-1-christian.koenig@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] mm: mmap: fix fput in error path v2 | expand |
On Mon, Oct 12, 2020 at 10:52:02AM +0200, Christian König wrote: > Patch "495c10cc1c0c CHROMIUM: dma-buf: restore args..." > adds a workaround for a bug in mmap_region. > > As the comment states ->mmap() callback can change > vma->vm_file and so we might call fput() on the wrong file. > > Revert the workaround and proper fix this in mmap_region. > > v2: drop the extra if in dma_buf_mmap as well > > Signed-off-by: Christian König <christian.koenig@amd.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/dma-buf/dma-buf.c | 20 +++----------------- > mm/mmap.c | 2 +- > 2 files changed, 4 insertions(+), 18 deletions(-) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Probably should Fixes that other patch Andrew pointed at Jason
On Mon, Oct 12, 2020 at 10:52:03AM +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. > v3: add missing EXPORT_SYMBOL, rebase on mmap cleanup, > add comments why we drop the reference on two occasions. > v4: make it clear that changing an anonymous vma is illegal. > > Signed-off-by: Christian König <christian.koenig@amd.com> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> (v2) > --- > drivers/dma-buf/dma-buf.c | 3 +-- > 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 | 5 +++-- > 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 | 6 +++--- > include/linux/mm.h | 2 ++ > mm/mmap.c | 12 ++++++++++++ > 10 files changed, 26 insertions(+), 19 deletions(-) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
If nobody comes up with an objections I'm going to merge that through drm-misc-next. Thanks, Christian. Am 12.10.20 um 10:52 schrieb Christian König: > Patch "495c10cc1c0c CHROMIUM: dma-buf: restore args..." > adds a workaround for a bug in mmap_region. > > As the comment states ->mmap() callback can change > vma->vm_file and so we might call fput() on the wrong file. > > Revert the workaround and proper fix this in mmap_region. > > v2: drop the extra if in dma_buf_mmap as well > > Signed-off-by: Christian König <christian.koenig@amd.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/dma-buf/dma-buf.c | 20 +++----------------- > mm/mmap.c | 2 +- > 2 files changed, 4 insertions(+), 18 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index a6ba4d598f0e..08630d057cf2 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1143,9 +1143,6 @@ EXPORT_SYMBOL_GPL(dma_buf_end_cpu_access); > int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, > unsigned long pgoff) > { > - struct file *oldfile; > - int ret; > - > if (WARN_ON(!dmabuf || !vma)) > return -EINVAL; > > @@ -1163,22 +1160,11 @@ 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; > + fput(vma->vm_file); > + vma->vm_file = get_file(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); > - } > - return ret; > - > + return dmabuf->ops->mmap(dmabuf, vma); > } > EXPORT_SYMBOL_GPL(dma_buf_mmap); > > diff --git a/mm/mmap.c b/mm/mmap.c > index 40248d84ad5f..3a2670d73355 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1852,8 +1852,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > return addr; > > unmap_and_free_vma: > + fput(vma->vm_file); > vma->vm_file = NULL; > - fput(file); > > /* Undo any partial mapping done by a device driver. */ > unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end);
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index a6ba4d598f0e..08630d057cf2 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1143,9 +1143,6 @@ EXPORT_SYMBOL_GPL(dma_buf_end_cpu_access); int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, unsigned long pgoff) { - struct file *oldfile; - int ret; - if (WARN_ON(!dmabuf || !vma)) return -EINVAL; @@ -1163,22 +1160,11 @@ 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; + fput(vma->vm_file); + vma->vm_file = get_file(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); - } - return ret; - + return dmabuf->ops->mmap(dmabuf, vma); } EXPORT_SYMBOL_GPL(dma_buf_mmap); diff --git a/mm/mmap.c b/mm/mmap.c index 40248d84ad5f..3a2670d73355 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1852,8 +1852,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr, return addr; unmap_and_free_vma: + fput(vma->vm_file); vma->vm_file = NULL; - fput(file); /* Undo any partial mapping done by a device driver. */ unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end);