On Mon, Jul 01, 2024 at 09:29:48PM -0700, Leah Rumancik wrote:
From: Miaohe Lin <linmiaohe@xxxxxxxxxx>
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
