| Message ID | 20201203085350.22624-1-liuzixian4@huawei.com |
|---|---|
| State | Accepted |
| Commit | 309d08d9b3a3659ab3f239d27d4e38b670b08fc9 |
| Headers | show |
| Series | [v2] fix mmap return value when vma is merged after call_mmap() | expand |
On Thu, Dec 03, 2020 at 04:53:50PM +0800, Liu Zixian wrote: > On success, mmap should return the begin address of newly mapped area, > but patch "mm: mmap: merge vma after call_mmap() if possible" > set vm_start of newly merged vma to return value addr. > Users of mmap will get wrong address if vma is merged after call_mmap(). > We fix this by moving the assignment to addr before merging vma. > > Fixes: d70cec898324 ("mm: mmap: merge vma after call_mmap() if possible") > Signed-off-by: Liu Zixian <liuzixian4@huawei.com> > --- > v2: > We want to do "addr = vma->vm_start;" unconditionally, > so move assignment to addr before if(unlikely) block. > --- > mm/mmap.c | 26 ++++++++++++-------------- > 1 file changed, 12 insertions(+), 14 deletions(-) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Fri, Dec 04, 2020 at 03:25:35PM +0000, Matthew Wilcox wrote: > This commit makes no sense. I know it's eight years old, so maybe the > device driver which did this has long been removed from the tree, but > davem's comment was (iirc) related to a device driver for a graphics > card that would 256MB-align the user address. Another possibility is > that userspace always asks for a 256MB-aligned address these days. Presumably the latter, otherwise people would be complaining about the WARN_ON. With some grep I could only find this: static int mc68x328fb_mmap(struct fb_info *info, struct vm_area_struct *vma) { #ifndef MMU /* this is uClinux (no MMU) specific code */ vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP; vma->vm_start = videomemory; return 0; #else return -EINVAL; #endif } So it does seem gone > I don't understand why prev/rb_link/rb_parent would need to be changed > in this case. It's going to be inserted at the exact same location in > the rbtree, just at a slightly shifted address. If the driver adjust the address, and it doesn't collide with another vma, and it doesn't change the tree position, then it could work But if the driver radically changes the vm_start all bets are off and you end up with an unsorted rb_tree at worst. Banning drivers from adjusting the vm_start/end makes sense to me, at least. How could a driver do this correctly anyhow? Jason
diff --git a/mm/mmap.c b/mm/mmap.c index d91ecb00d38c..5c8b4485860d 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1808,6 +1808,17 @@ unsigned long mmap_region(struct file *file, unsigned long addr, if (error) goto unmap_and_free_vma; + /* Can addr have changed?? + * + * Answer: Yes, several device drivers can do it in their + * f_op->mmap method. -DaveM + * Bug: If addr is changed, prev, rb_link, rb_parent should + * be updated for vma_link() + */ + WARN_ON_ONCE(addr != vma->vm_start); + + addr = vma->vm_start; + /* If vm_flags changed after call_mmap(), we should try merge vma again * as we may succeed this time. */ @@ -1822,25 +1833,12 @@ unsigned long mmap_region(struct file *file, unsigned long addr, fput(vma->vm_file); vm_area_free(vma); vma = merge; - /* Update vm_flags and possible addr to pick up the change. We don't - * warn here if addr changed as the vma is not linked by vma_link(). - */ - addr = vma->vm_start; + /* Update vm_flags to pick up the change. */ vm_flags = vma->vm_flags; goto unmap_writable; } } - /* Can addr have changed?? - * - * Answer: Yes, several device drivers can do it in their - * f_op->mmap method. -DaveM - * Bug: If addr is changed, prev, rb_link, rb_parent should - * be updated for vma_link() - */ - WARN_ON_ONCE(addr != vma->vm_start); - - addr = vma->vm_start; vm_flags = vma->vm_flags; } else if (vm_flags & VM_SHARED) { error = shmem_zero_setup(vma);
On success, mmap should return the begin address of newly mapped area, but patch "mm: mmap: merge vma after call_mmap() if possible" set vm_start of newly merged vma to return value addr. Users of mmap will get wrong address if vma is merged after call_mmap(). We fix this by moving the assignment to addr before merging vma. Fixes: d70cec898324 ("mm: mmap: merge vma after call_mmap() if possible") Signed-off-by: Liu Zixian <liuzixian4@huawei.com> --- v2: We want to do "addr = vma->vm_start;" unconditionally, so move assignment to addr before if(unlikely) block. --- mm/mmap.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)