| Message ID | 20201106114806.46015-2-christian.koenig@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] mm: mmap: fix fput in error path v2 | expand |
Am 06.11.20 um 23:48 schrieb Andrew Morton: > On Fri, 6 Nov 2020 12:48:05 +0100 "Christian König" <ckoenig.leichtzumerken@gmail.com> 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. >> > Seems correct, best I can tell. Presumably all ->mmap() instances will > correctly fput() to original file* if they're rewriting vma->vm_file. Yes, exactly. Patch #2 provides a helper to make sure that everybody gets the get_file()/fput() correctly while updating vma->vm_file. Can I add your acked-by to the patches and push them upstream through drm-misc-next? Thanks, Christian.
On Wed, 18 Nov 2020 11:57:44 +0100 Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > Am 06.11.20 um 23:48 schrieb Andrew Morton: > > On Fri, 6 Nov 2020 12:48:05 +0100 "Christian König" <ckoenig.leichtzumerken@gmail.com> 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. > >> > > Seems correct, best I can tell. Presumably all ->mmap() instances will > > correctly fput() to original file* if they're rewriting vma->vm_file. > > Yes, exactly. > > Patch #2 provides a helper to make sure that everybody gets the > get_file()/fput() correctly while updating vma->vm_file. > > Can I add your acked-by to the patches and push them upstream through > drm-misc-next? Please go ahead. Acked-by: Andrew Morton <akpm@linux-foundation.org>
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 0eb80c1ecdab..282bd8b84170 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1166,9 +1166,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; @@ -1186,22 +1183,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 d91ecb00d38c..30a4e8412a58 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1899,8 +1899,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);