Messages in this thread | | | Date | Wed, 19 Oct 2022 16:08:28 +0200 | Subject | Re: [PATCH] mm/mmap: Fix MAP_FIXED address return on VMA merge | From | David Hildenbrand <> |
| |
>>> mm/mmap.c | 15 +++++++-------- >>> 1 file changed, 7 insertions(+), 8 deletions(-) >>> >>> diff --git a/mm/mmap.c b/mm/mmap.c >>> index 42cd2c260898..22010e13f1a1 100644 >>> --- a/mm/mmap.c >>> +++ b/mm/mmap.c >>> @@ -2625,14 +2625,14 @@ 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 >>> + /* >>> + * Expansion is handled above, merging is handled below. >>> + * Drivers should not alter the address of the VMA. >>> */ >>> - WARN_ON_ONCE(addr != vma->vm_start); >>> - >>> - addr = vma->vm_start; >>> + if (WARN_ON((addr != vma->vm_start))) { >>> + error = -EINVAL; >>> + goto close_and_free_vma; >>> + } >> >> If this is something that user space can trigger, WARN_* is the wrong >> choice. But what I understand from the comment change is that this must not >> happen at that point unless there is a real issue. > > The VMA start address could be changed in call_mmap() which is a driver > call. I guess someone could write a driver to mmap by a users action? > I don't think it can be reached other ways. In any case, I'm changing a > WARN_ON_ONCE() to a WARN_ON() and undoing the badness instead of > marching forwards.
WARN_ON_ONCE() can also be used in conditionals if that's what you were concerned about, but ...
> >> >> Why not "if (WARN_ON_ONCE)" ? > > I was thinking it was harder to ignore if it happen more frequently? > There isn't a driver that does this now, but I'm not picky over which > variant to call.
.. I think the assumption really is that we won't see (m)any these calls. And if we do, it's a bad bad driver. So WARN_ON() might be just fine.
If this would be easy to trigger by any user space, WARN* would have been the wrong choice, that's why I was asking.
-- Thanks,
David / dhildenb
| |