Re: [PATCH v2] fix mmap return value when vma is merged after call_mmap()

[David Hildenbrand <david@redhat.com>]

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
> --- 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()
> +		 */


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?

Anyhow, unrelated to your patch

Reviewed-by: David Hildenbrand <david@redhat.com>



-- 
Thanks,

David / dhildenb