| Message ID | 20201009150342.1979-1-christian.koenig@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] mm: mmap: fix fput in error path | expand |
On Fri, Oct 09, 2020 at 05:03:38PM +0200, Christian König wrote: > +/* > + * Change backing file, only valid to use during initial VMA setup. > + */ > +void 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); > +} fput crashes when file is NULL so the error handling after unmap_and_free_vma: can't handle this case, similarly vm_file can't be NULL either. So just simply: swap(vma->vm_file, file); get_file(vma->vm_file); fput(file); Will do? Just let it crash if any of them are wrongly NULL. Jason
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 09.10.20 um 17:14 schrieb Jason Gunthorpe: > On Fri, Oct 09, 2020 at 05:03:38PM +0200, Christian König wrote: >> +/* >> + * Change backing file, only valid to use during initial VMA setup. >> + */ >> +void 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); >> +} > fput crashes when file is NULL so the error handling after > unmap_and_free_vma: can't handle this case, similarly vm_file can't be > NULL either. > > So just simply: > > swap(vma->vm_file, file); > get_file(vma->vm_file); > fput(file); > > Will do? I was considering this as well, yes. > Just let it crash if any of them are wrongly NULL. Mhm, changing from anonymous to file backed or reverse is probably not such a good idea. So yes catching those problems early is probably the best approach we could do. Going to do this in v4 if nobody objects. Regards, 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(-)