| Message ID | 20201009150342.1979-1-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] mm: mmap: fix fput in error path | expand |
On Fri, Oct 09, 2020 at 05:03:37PM +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. > > Signed-off-by: Christian König <christian.koenig@amd.com> > drivers/dma-buf/dma-buf.c | 22 +++++----------------- > mm/mmap.c | 2 +- > 2 files changed, 6 insertions(+), 18 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index a6ba4d598f0e..edd57402a48a 100644 > +++ 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,13 @@ 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; > - vma->vm_pgoff = pgoff; > + if (vma->vm_file) > + fput(vma->vm_file); This if is redundant too But otherwise Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Thanks, Jason
On Fri, 9 Oct 2020 17:03:37 +0200 "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. > Doesn't this patch series address the same thing as https://lkml.kernel.org/r/20200916090733.31427-1-linmiaohe@huawei.com?
On Fri, Oct 09, 2020 at 03:04:20PM -0700, Andrew Morton wrote: > On Fri, 9 Oct 2020 17:03:37 +0200 "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. > > > > Doesn't this patch series address the same thing as > https://lkml.kernel.org/r/20200916090733.31427-1-linmiaohe@huawei.com? Same basic issue, looks like both of these patches should be combined to plug it fully. Jason
Am 10.10.20 um 00:25 schrieb Jason Gunthorpe: > On Fri, Oct 09, 2020 at 03:04:20PM -0700, Andrew Morton wrote: >> On Fri, 9 Oct 2020 17:03:37 +0200 "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. >>> >> Doesn't this patch series address the same thing as >> https://lkml.kernel.org/r/20200916090733.31427-1-linmiaohe@huawei.com? > Same basic issue, looks like both of these patches should be combined > to plug it fully. Yes, agree completely. It's a different error path, but we need to fix both occasions. Christian. > > Jason
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index a6ba4d598f0e..edd57402a48a 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,13 @@ 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; - vma->vm_pgoff = pgoff; + if (vma->vm_file) + fput(vma->vm_file); - 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; + vma->vm_file = get_file(dmabuf->file); + vma->vm_pgoff = pgoff; + 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);
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. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-buf.c | 22 +++++----------------- mm/mmap.c | 2 +- 2 files changed, 6 insertions(+), 18 deletions(-)