On Fri, Dec 04, 2020 at 11:10:28AM -0400, Jason Gunthorpe wrote:
On Fri, Dec 04, 2020 at 03:11:54PM +0100, David Hildenbrand wrote:
On 03.12.20 09:53, 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(-)
diff --git a/mm/mmap.c b/mm/mmap.c index d91ecb00d38c..5c8b4485860d 100644 +++ 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()*/Why do we tolerate device drivers doing such stuff at all? WARN_ON_ONCE() is just as good as BUG_ON() in many environments. If we support it, then no warning. If we don't support it, then why tolerate it?
The commit that introduced this seemed pretty clear it is to catch possibly wrong drivers. I suppose the idea was to give a migration time where things would "work" and drivers could be fixed. Since it has now been 8 years it should be either dropped or turned into:
/* Drivers are not permitted to change vm_start */ if (WARN_ON(addr != vma->vm_start)) { err = EINVAL goto unmap_and_free_vma }
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.
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.