hugetlb: Acquire the i_mmap_lock before walking the prio_tree to unmap a page
Subsystems (2)
PATCH 1/1 hugetlb: Acquire the i_mmap_lock before walking the prio_tree to unmap a page
When the owner of a mapping fails COW because a child process is holding a reference and no pages are available, the children VMAs are walked and the page is unmapped. The i_mmap_lock is taken for the unmapping of the page but not the walking of the prio_tree. In theory, that tree could be changing while the lock is released although in practice it is protected by the hugetlb_instantiation_mutex. This patch takes the i_mmap_lock properly for the duration of the prio_tree walk in case the hugetlb_instantiation_mutex ever goes away.
--- mm/hugetlb.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)
mm/hugetlb.c +8 -1
@@ -1906,6 +1906,12 @@ static int unmap_ref_private(struct mm_struct *mm, struct vm_area_struct *vma, + (vma->vm_pgoff >>PAGE_SHIFT ); mapping = (struct address_space *)page_private (page); +/* +* Take the mapping lock for the duration of the table walk. As +* this mapping should be shared between all the VMAs, +* __unmap_hugepage_range() is called as the lock is already held +*/ +spin_lock (&mapping->i_mmap_lock); vma_prio_tree_foreach(iter_vma, &iter, &mapping->i_mmap, pgoff, pgoff) {/* Do not unmap the current VMA */ if (iter_vma == vma)
@@ -1919,10 +1925,11 @@ static int unmap_ref_private(struct mm_struct *mm, struct vm_area_struct *vma,* from the time of fork. This would look like data corruption */ if (!is_vma_resv_set (iter_vma,HPAGE_RESV_OWNER )) -unmap_hugepage_range (iter_vma, +__unmap_hugepage_range (iter_vma, address, address +huge_page_size (h), page); } +spin_unlock (&mapping->i_mmap_lock);return 1 ; }
Quoted 11 lines · Mel Gorman
When the owner of a mapping fails COW because a child process is holding a reference and no pages are available, the children VMAs are walked and the page is unmapped. The
i_mmap_lockis taken for the unmapping of the page but not the walking of theprio_tree. In theory, that tree could be changing while the lock is released although in practice it is protected by thehugetlb_instantiation_mutex. This patch takes thei_mmap_lockproperly for the duration of theprio_treewalk in case thehugetlb_instantiation_mutexever goes away.[[email protected]: Spotted the problem in the first place]Signed-off-by Mel Gorman <[email protected]>
The patch looks good - thanks for taking care of that, Mel.
But the comment seems wrong to me: hugetlb_instantiation_mutex guards against concurrent hugetlb_fault()s; but the structure of the prio_tree shifts as vmas based on that inode are inserted into (mmap'ed) and removed from (munmap'ed) that tree (always while holding i_mmap_lock). I don't see hugetlb_instantiation_mutex giving us any protection against this at present.
Hugh
Quoted 34 lines
--- mm/hugetlb.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)mm/hugetlb.c +8
-1@@ -1906,6 +1906,12 @@ static int unmap_ref_private(struct mm_struct *mm, struct vm_area_struct *vma, + (vma->vm_pgoff >>PAGE_SHIFT ); mapping = (struct address_space *)page_private (page); +/* +* Take the mapping lock for the duration of the table walk. As +* this mapping should be shared between all the VMAs, +* __unmap_hugepage_range() is called as the lock is already held +*/ +spin_lock (&mapping->i_mmap_lock); vma_prio_tree_foreach(iter_vma, &iter, &mapping->i_mmap, pgoff, pgoff) {/* Do not unmap the current VMA */ if (iter_vma == vma)@@ -1919,10 +1925,11 @@ static int unmap_ref_private(struct mm_struct *mm, struct vm_area_struct *vma,* from the time of fork. This would look like data corruption */ if (!is_vma_resv_set (iter_vma,HPAGE_RESV_OWNER )) -unmap_hugepage_range (iter_vma, +__unmap_hugepage_range (iter_vma, address, address +huge_page_size (h), page); } +spin_unlock (&mapping->i_mmap_lock);return 1 ; }
Quoted 14 lines · Hugh Dickins
Quoted 11 lines · Mel Gorman
When the owner of a mapping fails COW because a child process is holding a reference and no pages are available, the children VMAs are walked and the page is unmapped. The
i_mmap_lockis taken for the unmapping of the page but not the walking of theprio_tree. In theory, that tree could be changing while the lock is released although in practice it is protected by thehugetlb_instantiation_mutex. This patch takes thei_mmap_lockproperly for the duration of theprio_treewalk in case thehugetlb_instantiation_mutexever goes away.[[email protected]: Spotted the problem in the first place]Signed-off-by Mel Gorman <[email protected]>
Quoted 9 lines
The patch looks good - thanks for taking care of that, Mel.
But the comment seems wrong to me:
hugetlb_instantiation_mutexguards against concurrent hugetlb_fault()s; but the structure of theprio_treeshifts as vmas based on that inode are inserted into (mmap'ed) and removed from (munmap'ed) that tree (always while holdingi_mmap_lock). I don't seehugetlb_instantiation_mutexgiving us any protection against this at present.
You're right of course. I'll report without that nonsense included.
Thanks
Quoted 36 lines
Quoted 35 lines
--- mm/hugetlb.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)mm/hugetlb.c +8
-1@@ -1906,6 +1906,12 @@ static int unmap_ref_private(struct mm_struct *mm, struct vm_area_struct *vma, + (vma->vm_pgoff >>PAGE_SHIFT ); mapping = (struct address_space *)page_private (page); +/* +* Take the mapping lock for the duration of the table walk. As +* this mapping should be shared between all the VMAs, +* __unmap_hugepage_range() is called as the lock is already held +*/ +spin_lock (&mapping->i_mmap_lock); vma_prio_tree_foreach(iter_vma, &iter, &mapping->i_mmap, pgoff, pgoff) {/* Do not unmap the current VMA */ if (iter_vma == vma)@@ -1919,10 +1925,11 @@ static int unmap_ref_private(struct mm_struct *mm, struct vm_area_struct *vma,* from the time of fork. This would look like data corruption */ if (!is_vma_resv_set (iter_vma,HPAGE_RESV_OWNER )) -unmap_hugepage_range (iter_vma, +__unmap_hugepage_range (iter_vma, address, address +huge_page_size (h), page); } +spin_unlock (&mapping->i_mmap_lock);return 1 ; }
Signature · 3 lines
Mel Gorman Part-time Phd Student Linux Technology Center University of Limerick IBM Dublin Software Lab
Quoted 25 lines · Mel Gorman
Quoted 14 lines · Hugh Dickins
Quoted 11 lines · Mel Gorman
When the owner of a mapping fails COW because a child process is holding a reference and no pages are available, the children VMAs are walked and the page is unmapped. The
i_mmap_lockis taken for the unmapping of the page but not the walking of theprio_tree. In theory, that tree could be changing while the lock is released although in practice it is protected by thehugetlb_instantiation_mutex. This patch takes thei_mmap_lockproperly for the duration of theprio_treewalk in case thehugetlb_instantiation_mutexever goes away.[[email protected]: Spotted the problem in the first place]Signed-off-by Mel Gorman <[email protected]>Quoted 9 lines
The patch looks good - thanks for taking care of that, Mel.
But the comment seems wrong to me:
hugetlb_instantiation_mutexguards against concurrent hugetlb_fault()s; but the structure of theprio_treeshifts as vmas based on that inode are inserted into (mmap'ed) and removed from (munmap'ed) that tree (always while holdingi_mmap_lock). I don't seehugetlb_instantiation_mutexgiving us any protection against this at present.
Quoted 2 lines
You're right of course. I'll report without that nonsense included.
Actually, shouldn't the mmap_sem be protecting against concurrent mmap and munmap altering the tree? The comment is still bogus of course.
Signature · 3 lines
Mel Gorman Part-time Phd Student Linux Technology Center University of Limerick IBM Dublin Software Lab
Quoted 11 lines · Mel Gorman
Quoted 10 lines · Mel Gorman
Quoted 8 lines · Hugh Dickins
But the comment seems wrong to me:
hugetlb_instantiation_mutexguards against concurrent hugetlb_fault()s; but the structure of theprio_treeshifts as vmas based on that inode are inserted into (mmap'ed) and removed from (munmap'ed) that tree (always while holdingi_mmap_lock). I don't seehugetlb_instantiation_mutexgiving us any protection against this at present.
Quoted 5 lines
Quoted 2 lines
You're right of course. I'll report without that nonsense included.
Actually, shouldn't the
mmap_sembe protecting against concurrent mmap and munmap altering the tree? The comment is still bogus of course.
No, the mmap_sem can only protect against other threads sharing that same mm: whereas the prio_tree can shift around according to concurrent mmaps and munmaps of the same file in other mms.
Hugh