| Message ID | 20221129193526.3588187-5-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/10] mm/hugetlb: Let vma_offset_start() to return start | expand |
On 11/29/22 14:35, Peter Xu wrote: > In hugetlb_fault(), there used to have a special path to handle swap entry > at the entrance using huge_pte_offset(). That's unsafe because > huge_pte_offset() for a pmd sharable range can access freed pgtables if > without any lock to protect the pgtable from being freed after pmd unshare. Thanks. That special path has been there for a long time. I should have given it more thought when adding lock. However, I was only considering the 'stale' pte case as opposed to freed. > Here the simplest solution to make it safe is to move the swap handling to > be after the vma lock being held. We may need to take the fault mutex on > either migration or hwpoison entries now (also the vma lock, but that's > really needed), however neither of them is hot path. > > Note that the vma lock cannot be released in hugetlb_fault() when the > migration entry is detected, because in migration_entry_wait_huge() the > pgtable page will be used again (by taking the pgtable lock), so that also > need to be protected by the vma lock. Modify migration_entry_wait_huge() > so that it must be called with vma read lock held, and properly release the > lock in __migration_entry_wait_huge(). > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/linux/swapops.h | 6 ++++-- > mm/hugetlb.c | 32 +++++++++++++++----------------- > mm/migrate.c | 25 +++++++++++++++++++++---- > 3 files changed, 40 insertions(+), 23 deletions(-) Looks good with one small change noted below, Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> > > diff --git a/include/linux/swapops.h b/include/linux/swapops.h > index 27ade4f22abb..09b22b169a71 100644 > --- a/include/linux/swapops.h > +++ b/include/linux/swapops.h > @@ -335,7 +335,8 @@ extern void __migration_entry_wait(struct mm_struct *mm, pte_t *ptep, > extern void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd, > unsigned long address); > #ifdef CONFIG_HUGETLB_PAGE > -extern void __migration_entry_wait_huge(pte_t *ptep, spinlock_t *ptl); > +extern void __migration_entry_wait_huge(struct vm_area_struct *vma, > + pte_t *ptep, spinlock_t *ptl); > extern void migration_entry_wait_huge(struct vm_area_struct *vma, pte_t *pte); > #endif /* CONFIG_HUGETLB_PAGE */ > #else /* CONFIG_MIGRATION */ > @@ -364,7 +365,8 @@ static inline void __migration_entry_wait(struct mm_struct *mm, pte_t *ptep, > static inline void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd, > unsigned long address) { } > #ifdef CONFIG_HUGETLB_PAGE > -static inline void __migration_entry_wait_huge(pte_t *ptep, spinlock_t *ptl) { } > +static inline void __migration_entry_wait_huge(struct vm_area_struct *vma, > + pte_t *ptep, spinlock_t *ptl) { } > static inline void migration_entry_wait_huge(struct vm_area_struct *vma, pte_t *pte) { } > #endif /* CONFIG_HUGETLB_PAGE */ > static inline int is_writable_migration_entry(swp_entry_t entry) > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index dfe677fadaf8..776e34ccf029 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -5826,22 +5826,6 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma, > int need_wait_lock = 0; > unsigned long haddr = address & huge_page_mask(h); > > - ptep = huge_pte_offset(mm, haddr, huge_page_size(h)); > - if (ptep) { > - /* > - * Since we hold no locks, ptep could be stale. That is > - * OK as we are only making decisions based on content and > - * not actually modifying content here. > - */ > - entry = huge_ptep_get(ptep); > - if (unlikely(is_hugetlb_entry_migration(entry))) { > - migration_entry_wait_huge(vma, ptep); > - return 0; > - } else if (unlikely(is_hugetlb_entry_hwpoisoned(entry))) > - return VM_FAULT_HWPOISON_LARGE | > - VM_FAULT_SET_HINDEX(hstate_index(h)); > - } > - Before acquiring the vma_lock, there is this comment: /* * Acquire vma lock before calling huge_pte_alloc and hold * until finished with ptep. This prevents huge_pmd_unshare from * being called elsewhere and making the ptep no longer valid. * * ptep could have already be assigned via hugetlb_walk(). That * is OK, as huge_pte_alloc will return the same value unless * something has changed. */ The second sentence in that comment about ptep being already assigned no longer applies and can be deleted.
On Mon, Dec 05, 2022 at 02:14:38PM -0800, Mike Kravetz wrote: > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > index dfe677fadaf8..776e34ccf029 100644 > > --- a/mm/hugetlb.c > > +++ b/mm/hugetlb.c > > @@ -5826,22 +5826,6 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma, > > int need_wait_lock = 0; > > unsigned long haddr = address & huge_page_mask(h); > > > > - ptep = huge_pte_offset(mm, haddr, huge_page_size(h)); > > - if (ptep) { > > - /* > > - * Since we hold no locks, ptep could be stale. That is > > - * OK as we are only making decisions based on content and > > - * not actually modifying content here. > > - */ > > - entry = huge_ptep_get(ptep); > > - if (unlikely(is_hugetlb_entry_migration(entry))) { > > - migration_entry_wait_huge(vma, ptep); > > - return 0; > > - } else if (unlikely(is_hugetlb_entry_hwpoisoned(entry))) > > - return VM_FAULT_HWPOISON_LARGE | > > - VM_FAULT_SET_HINDEX(hstate_index(h)); > > - } > > - > > Before acquiring the vma_lock, there is this comment: > > /* > * Acquire vma lock before calling huge_pte_alloc and hold > * until finished with ptep. This prevents huge_pmd_unshare from > * being called elsewhere and making the ptep no longer valid. > * > * ptep could have already be assigned via hugetlb_walk(). That > * is OK, as huge_pte_alloc will return the same value unless > * something has changed. > */ > > The second sentence in that comment about ptep being already assigned no > longer applies and can be deleted. Correct, this can be removed. One thing to mention is there's an inline touch-up above in the last patch of the series when introducing hugetlb_walk() to s/pte_offset/walk/, but I saw that Andrew has already done the right thing on the fixup one in his local tree, so I think we're all good. Thanks!
diff --git a/include/linux/swapops.h b/include/linux/swapops.h index 27ade4f22abb..09b22b169a71 100644 --- a/include/linux/swapops.h +++ b/include/linux/swapops.h @@ -335,7 +335,8 @@ extern void __migration_entry_wait(struct mm_struct *mm, pte_t *ptep, extern void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd, unsigned long address); #ifdef CONFIG_HUGETLB_PAGE -extern void __migration_entry_wait_huge(pte_t *ptep, spinlock_t *ptl); +extern void __migration_entry_wait_huge(struct vm_area_struct *vma, + pte_t *ptep, spinlock_t *ptl); extern void migration_entry_wait_huge(struct vm_area_struct *vma, pte_t *pte); #endif /* CONFIG_HUGETLB_PAGE */ #else /* CONFIG_MIGRATION */ @@ -364,7 +365,8 @@ static inline void __migration_entry_wait(struct mm_struct *mm, pte_t *ptep, static inline void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd, unsigned long address) { } #ifdef CONFIG_HUGETLB_PAGE -static inline void __migration_entry_wait_huge(pte_t *ptep, spinlock_t *ptl) { } +static inline void __migration_entry_wait_huge(struct vm_area_struct *vma, + pte_t *ptep, spinlock_t *ptl) { } static inline void migration_entry_wait_huge(struct vm_area_struct *vma, pte_t *pte) { } #endif /* CONFIG_HUGETLB_PAGE */ static inline int is_writable_migration_entry(swp_entry_t entry) diff --git a/mm/hugetlb.c b/mm/hugetlb.c index dfe677fadaf8..776e34ccf029 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -5826,22 +5826,6 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma, int need_wait_lock = 0; unsigned long haddr = address & huge_page_mask(h); - ptep = huge_pte_offset(mm, haddr, huge_page_size(h)); - if (ptep) { - /* - * Since we hold no locks, ptep could be stale. That is - * OK as we are only making decisions based on content and - * not actually modifying content here. - */ - entry = huge_ptep_get(ptep); - if (unlikely(is_hugetlb_entry_migration(entry))) { - migration_entry_wait_huge(vma, ptep); - return 0; - } else if (unlikely(is_hugetlb_entry_hwpoisoned(entry))) - return VM_FAULT_HWPOISON_LARGE | - VM_FAULT_SET_HINDEX(hstate_index(h)); - } - /* * Serialize hugepage allocation and instantiation, so that we don't * get spurious allocation failures if two CPUs race to instantiate @@ -5888,8 +5872,22 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma, * fault, and is_hugetlb_entry_(migration|hwpoisoned) check will * properly handle it. */ - if (!pte_present(entry)) + if (!pte_present(entry)) { + if (unlikely(is_hugetlb_entry_migration(entry))) { + /* + * Release fault lock first because the vma lock is + * needed to guard the huge_pte_lockptr() later in + * migration_entry_wait_huge(). The vma lock will + * be released there. + */ + mutex_unlock(&hugetlb_fault_mutex_table[hash]); + migration_entry_wait_huge(vma, ptep); + return 0; + } else if (unlikely(is_hugetlb_entry_hwpoisoned(entry))) + ret = VM_FAULT_HWPOISON_LARGE | + VM_FAULT_SET_HINDEX(hstate_index(h)); goto out_mutex; + } /* * If we are going to COW/unshare the mapping later, we examine the diff --git a/mm/migrate.c b/mm/migrate.c index 267ad0d073ae..c13c828d34f3 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -326,24 +326,41 @@ void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd, } #ifdef CONFIG_HUGETLB_PAGE -void __migration_entry_wait_huge(pte_t *ptep, spinlock_t *ptl) +void __migration_entry_wait_huge(struct vm_area_struct *vma, + pte_t *ptep, spinlock_t *ptl) { pte_t pte; + /* + * The vma read lock must be taken, which will be released before + * the function returns. It makes sure the pgtable page (along + * with its spin lock) not be freed in parallel. + */ + hugetlb_vma_assert_locked(vma); + spin_lock(ptl); pte = huge_ptep_get(ptep); - if (unlikely(!is_hugetlb_entry_migration(pte))) + if (unlikely(!is_hugetlb_entry_migration(pte))) { spin_unlock(ptl); - else + hugetlb_vma_unlock_read(vma); + } else { + /* + * If migration entry existed, safe to release vma lock + * here because the pgtable page won't be freed without the + * pgtable lock released. See comment right above pgtable + * lock release in migration_entry_wait_on_locked(). + */ + hugetlb_vma_unlock_read(vma); migration_entry_wait_on_locked(pte_to_swp_entry(pte), NULL, ptl); + } } void migration_entry_wait_huge(struct vm_area_struct *vma, pte_t *pte) { spinlock_t *ptl = huge_pte_lockptr(hstate_vma(vma), vma->vm_mm, pte); - __migration_entry_wait_huge(pte, ptl); + __migration_entry_wait_huge(vma, pte, ptl); } #endif
In hugetlb_fault(), there used to have a special path to handle swap entry at the entrance using huge_pte_offset(). That's unsafe because huge_pte_offset() for a pmd sharable range can access freed pgtables if without any lock to protect the pgtable from being freed after pmd unshare. Here the simplest solution to make it safe is to move the swap handling to be after the vma lock being held. We may need to take the fault mutex on either migration or hwpoison entries now (also the vma lock, but that's really needed), however neither of them is hot path. Note that the vma lock cannot be released in hugetlb_fault() when the migration entry is detected, because in migration_entry_wait_huge() the pgtable page will be used again (by taking the pgtable lock), so that also need to be protected by the vma lock. Modify migration_entry_wait_huge() so that it must be called with vma read lock held, and properly release the lock in __migration_entry_wait_huge(). Signed-off-by: Peter Xu <peterx@redhat.com> --- include/linux/swapops.h | 6 ++++-- mm/hugetlb.c | 32 +++++++++++++++----------------- mm/migrate.c | 25 +++++++++++++++++++++---- 3 files changed, 40 insertions(+), 23 deletions(-)