Re: general protection fault in unlink_file_vma

[Hillf Danton <hdanton@sina.com>]

On Sun, 13 Sep 2020 09:17:26 +0000 linmiaohe wrote:
> 
> I reviewed the code carefully these days and I found vma_merge() do only fput() the vm_file of the linked vma in remove_next cases.
> This gpf is much likely because the ->mmap() callback can change vma->vm_file and fput the original file. But my previous commit
> failed to catch this case and always fput() the original file, hence add an extra fput().
> The below patch would make the things right:
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 080f44bcf7a8..80ea11bf12fa 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1816,7 +1816,11 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>                         merge = vma_merge(mm, prev, vma->vm_start, vma->vm_end, vma->vm_flags,
>                                 NULL, vma->vm_file, vma->vm_pgoff, NULL, NULL_VM_UFFD_CTX);
>                         if (merge) {
> -                               fput(file);
> +                               /* ->mmap() can change vma->vm_file and fput the original file. So
> +                                * fput the vma->vm_file here or we would add an extra fput for file
> +                                * and cause general protection fault ultimately.
> +                                */
> +                               fput(vma->vm_file);
>                                 vm_area_free(vma);
>                                 vma = merge;
>                                 /* Update vm_flags and possible addr to pick up the change. We don't
> 
> It's very nice of you if you could help test this patch as I can't reproduce it in my product environment. And many thanks
> for a possible Tested-by tag.

Take another look at the Cc list and the link below.

https://lore.kernel.org/lkml/20200911120222.GT87483@ziepe.ca/