| Message ID | 20201012085203.56119-1-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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
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);