Re: [PATCH 6.6] fork: defer linking file vma until vma is fully initialized

[Sasha Levin <sashal@kernel.org>]

On Mon, Jul 01, 2024 at 09:29:48PM -0700, Leah Rumancik wrote:
>From: Miaohe Lin <linmiaohe@huawei.com>
>
>commit 35e351780fa9d8240dd6f7e4f245f9ea37e96c19 upstream.
>
>Thorvald reported a WARNING [1]. And the root cause is below race:
>
> CPU 1					CPU 2
> fork					hugetlbfs_fallocate
>  dup_mmap				 hugetlbfs_punch_hole
>   i_mmap_lock_write(mapping);
>   vma_interval_tree_insert_after -- Child vma is visible through i_mmap tree.
>   i_mmap_unlock_write(mapping);
>   hugetlb_dup_vma_private -- Clear vma_lock outside i_mmap_rwsem!
>					 i_mmap_lock_write(mapping);
>   					 hugetlb_vmdelete_list
>					  vma_interval_tree_foreach
>					   hugetlb_vma_trylock_write -- Vma_lock is cleared.
>   tmp->vm_ops->open -- Alloc new vma_lock outside i_mmap_rwsem!
>					   hugetlb_vma_unlock_write -- Vma_lock is assigned!!!
>					 i_mmap_unlock_write(mapping);
>
>hugetlb_dup_vma_private() and hugetlb_vm_op_open() are called outside
>i_mmap_rwsem lock while vma lock can be used in the same time.  Fix this
>by deferring linking file vma until vma is fully initialized.  Those vmas
>should be initialized first before they can be used.
>
>Backport notes:
>
>The first backport attempt (cec11fa2e) was reverted (dd782da4707). This is
>the new backport of the original fix (35e351780fa9).
>
>35e351780f ("fork: defer linking file vma until vma is fully initialized")
>fixed a hugetlb locking race by moving a bunch of intialization code to earlier
>in the function. The call to open() was included in the move but the call to
>copy_page_range was not, effectively inverting their relative ordering. This
>created an issue for the vfio code which assumes copy_page_range happens before
>the call to open() - vfio's open zaps the vma so that the fault handler is
>invoked later, but when we inverted the ordering, copy_page_range can set up
>mappings post-zap which would prevent the fault handler from being invoked
>later. This patch moves the call to copy_page_range to earlier than the call to
>open() to restore the original ordering of the two functions while keeping the
>fix for hugetlb intact.
>
>Commit aac6db75a9 made several changes to vfio_pci_core.c, including
>removing the vfio-pci custom open function. This resolves the issue on
>the main branch and so we only need to apply these changes when
>backporting to stable branches.
>
>35e351780f ("fork: defer linking file vma until vma is fully initialized")-> v6.9-rc5
>aac6db75a9 ("vfio/pci: Use unmap_mapping_range()") -> v6.10-rc4

Is there a strong reason not to take the commit above instead? That way:

a. We stay aligned with upstream, not needed a custom backport.
b. We avoid similar issues in the future.

-- 
Thanks,
Sasha