| Message ID | 20181105212315.14125-1-mike.kravetz@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hugetlbfs: fix kernel BUG at fs/hugetlbfs/inode.c:444! | expand |
On Mon, 5 Nov 2018 13:23:15 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote: > This bug has been experienced several times by Oracle DB team. > The BUG is in the routine remove_inode_hugepages() as follows: > /* > * If page is mapped, it was faulted in after being > * unmapped in caller. Unmap (again) now after taking > * the fault mutex. The mutex will prevent faults > * until we finish removing the page. > * > * This race can only happen in the hole punch case. > * Getting here in a truncate operation is a bug. > */ > if (unlikely(page_mapped(page))) { > BUG_ON(truncate_op); > > In this case, the elevated map count is not the result of a race. > Rather it was incorrectly incremented as the result of a bug in the > huge pmd sharing code. Consider the following: > - Process A maps a hugetlbfs file of sufficient size and alignment > (PUD_SIZE) that a pmd page could be shared. > - Process B maps the same hugetlbfs file with the same size and alignment > such that a pmd page is shared. > - Process B then calls mprotect() to change protections for the mapping > with the shared pmd. As a result, the pmd is 'unshared'. > - Process B then calls mprotect() again to chage protections for the > mapping back to their original value. pmd remains unshared. > - Process B then forks and process C is created. During the fork process, > we do dup_mm -> dup_mmap -> copy_page_range to copy page tables. Copying > page tables for hugetlb mappings is done in the routine > copy_hugetlb_page_range. > > In copy_hugetlb_page_range(), the destination pte is obtained by: > dst_pte = huge_pte_alloc(dst, addr, sz); > If pmd sharing is possible, the returned pointer will be to a pte in > an existing page table. In the situation above, process C could share > with either process A or process B. Since process A is first in the > list, the returned pte is a pointer to a pte in process A's page table. > > However, the following check for pmd sharing is in copy_hugetlb_page_range. > /* If the pagetables are shared don't copy or take references */ > if (dst_pte == src_pte) > continue; > > Since process C is sharing with process A instead of process B, the above > test fails. The code in copy_hugetlb_page_range which follows assumes > dst_pte points to a huge_pte_none pte. It copies the pte entry from > src_pte to dst_pte and increments this map count of the associated page. > This is how we end up with an elevated map count. > > To solve, check the dst_pte entry for huge_pte_none. If !none, this > implies PMD sharing so do not copy. > Does it warrant a cc:stable?
On 11/5/18 1:30 PM, Andrew Morton wrote: > On Mon, 5 Nov 2018 13:23:15 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote: > >> This bug has been experienced several times by Oracle DB team. >> The BUG is in the routine remove_inode_hugepages() as follows: >> /* >> * If page is mapped, it was faulted in after being >> * unmapped in caller. Unmap (again) now after taking >> * the fault mutex. The mutex will prevent faults >> * until we finish removing the page. >> * >> * This race can only happen in the hole punch case. >> * Getting here in a truncate operation is a bug. >> */ >> if (unlikely(page_mapped(page))) { >> BUG_ON(truncate_op); >> >> In this case, the elevated map count is not the result of a race. >> Rather it was incorrectly incremented as the result of a bug in the >> huge pmd sharing code. Consider the following: >> - Process A maps a hugetlbfs file of sufficient size and alignment >> (PUD_SIZE) that a pmd page could be shared. >> - Process B maps the same hugetlbfs file with the same size and alignment >> such that a pmd page is shared. >> - Process B then calls mprotect() to change protections for the mapping >> with the shared pmd. As a result, the pmd is 'unshared'. >> - Process B then calls mprotect() again to chage protections for the >> mapping back to their original value. pmd remains unshared. >> - Process B then forks and process C is created. During the fork process, >> we do dup_mm -> dup_mmap -> copy_page_range to copy page tables. Copying >> page tables for hugetlb mappings is done in the routine >> copy_hugetlb_page_range. >> >> In copy_hugetlb_page_range(), the destination pte is obtained by: >> dst_pte = huge_pte_alloc(dst, addr, sz); >> If pmd sharing is possible, the returned pointer will be to a pte in >> an existing page table. In the situation above, process C could share >> with either process A or process B. Since process A is first in the >> list, the returned pte is a pointer to a pte in process A's page table. >> >> However, the following check for pmd sharing is in copy_hugetlb_page_range. >> /* If the pagetables are shared don't copy or take references */ >> if (dst_pte == src_pte) >> continue; >> >> Since process C is sharing with process A instead of process B, the above >> test fails. The code in copy_hugetlb_page_range which follows assumes >> dst_pte points to a huge_pte_none pte. It copies the pte entry from >> src_pte to dst_pte and increments this map count of the associated page. >> This is how we end up with an elevated map count. >> >> To solve, check the dst_pte entry for huge_pte_none. If !none, this >> implies PMD sharing so do not copy. >> > > Does it warrant a cc:stable? My apologies, yes it does. Here are the additional tags: Fixes: c5c99429fa57 ("fix hugepages leak due to pagetable page sharing") Cc: <stable@vger.kernel.org> Let me know if you want me to resend with these.
On Mon, Nov 05, 2018 at 01:23:15PM -0800, Mike Kravetz wrote: > This bug has been experienced several times by Oracle DB team. > The BUG is in the routine remove_inode_hugepages() as follows: > /* > * If page is mapped, it was faulted in after being > * unmapped in caller. Unmap (again) now after taking > * the fault mutex. The mutex will prevent faults > * until we finish removing the page. > * > * This race can only happen in the hole punch case. > * Getting here in a truncate operation is a bug. > */ > if (unlikely(page_mapped(page))) { > BUG_ON(truncate_op); > > In this case, the elevated map count is not the result of a race. > Rather it was incorrectly incremented as the result of a bug in the > huge pmd sharing code. Consider the following: > - Process A maps a hugetlbfs file of sufficient size and alignment > (PUD_SIZE) that a pmd page could be shared. > - Process B maps the same hugetlbfs file with the same size and alignment > such that a pmd page is shared. > - Process B then calls mprotect() to change protections for the mapping > with the shared pmd. As a result, the pmd is 'unshared'. > - Process B then calls mprotect() again to chage protections for the > mapping back to their original value. pmd remains unshared. > - Process B then forks and process C is created. During the fork process, > we do dup_mm -> dup_mmap -> copy_page_range to copy page tables. Copying > page tables for hugetlb mappings is done in the routine > copy_hugetlb_page_range. > > In copy_hugetlb_page_range(), the destination pte is obtained by: > dst_pte = huge_pte_alloc(dst, addr, sz); > If pmd sharing is possible, the returned pointer will be to a pte in > an existing page table. In the situation above, process C could share > with either process A or process B. Since process A is first in the > list, the returned pte is a pointer to a pte in process A's page table. > > However, the following check for pmd sharing is in copy_hugetlb_page_range. > /* If the pagetables are shared don't copy or take references */ > if (dst_pte == src_pte) > continue; > > Since process C is sharing with process A instead of process B, the above > test fails. The code in copy_hugetlb_page_range which follows assumes > dst_pte points to a huge_pte_none pte. It copies the pte entry from > src_pte to dst_pte and increments this map count of the associated page. > This is how we end up with an elevated map count. > > To solve, check the dst_pte entry for huge_pte_none. If !none, this > implies PMD sharing so do not copy. > > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 5c390f5a5207..0b391ef6448c 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -3233,7 +3233,7 @@ static int is_hugetlb_entry_hwpoisoned(pte_t pte) int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src, struct vm_area_struct *vma) { - pte_t *src_pte, *dst_pte, entry; + pte_t *src_pte, *dst_pte, entry, dst_entry; struct page *ptepage; unsigned long addr; int cow; @@ -3261,15 +3261,30 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src, break; } - /* If the pagetables are shared don't copy or take references */ - if (dst_pte == src_pte) + /* + * If the pagetables are shared don't copy or take references. + * dst_pte == src_pte is the common case of src/dest sharing. + * + * However, src could have 'unshared' and dst shares with + * another vma. If dst_pte !none, this implies sharing. + * Check here before taking page table lock, and once again + * after taking the lock below. + */ + dst_entry = huge_ptep_get(dst_pte); + if ((dst_pte == src_pte) || !huge_pte_none(dst_entry)) continue; dst_ptl = huge_pte_lock(h, dst, dst_pte); src_ptl = huge_pte_lockptr(h, src, src_pte); spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING); entry = huge_ptep_get(src_pte); - if (huge_pte_none(entry)) { /* skip none entry */ + dst_entry = huge_ptep_get(dst_pte); + if (huge_pte_none(entry) || !huge_pte_none(dst_entry)) { + /* + * Skip if src entry none. Also, skip in the + * unlikely case dst entry !none as this implies + * sharing with another vma. + */ ; } else if (unlikely(is_hugetlb_entry_migration(entry) || is_hugetlb_entry_hwpoisoned(entry))) {
This bug has been experienced several times by Oracle DB team. The BUG is in the routine remove_inode_hugepages() as follows: /* * If page is mapped, it was faulted in after being * unmapped in caller. Unmap (again) now after taking * the fault mutex. The mutex will prevent faults * until we finish removing the page. * * This race can only happen in the hole punch case. * Getting here in a truncate operation is a bug. */ if (unlikely(page_mapped(page))) { BUG_ON(truncate_op); In this case, the elevated map count is not the result of a race. Rather it was incorrectly incremented as the result of a bug in the huge pmd sharing code. Consider the following: - Process A maps a hugetlbfs file of sufficient size and alignment (PUD_SIZE) that a pmd page could be shared. - Process B maps the same hugetlbfs file with the same size and alignment such that a pmd page is shared. - Process B then calls mprotect() to change protections for the mapping with the shared pmd. As a result, the pmd is 'unshared'. - Process B then calls mprotect() again to chage protections for the mapping back to their original value. pmd remains unshared. - Process B then forks and process C is created. During the fork process, we do dup_mm -> dup_mmap -> copy_page_range to copy page tables. Copying page tables for hugetlb mappings is done in the routine copy_hugetlb_page_range. In copy_hugetlb_page_range(), the destination pte is obtained by: dst_pte = huge_pte_alloc(dst, addr, sz); If pmd sharing is possible, the returned pointer will be to a pte in an existing page table. In the situation above, process C could share with either process A or process B. Since process A is first in the list, the returned pte is a pointer to a pte in process A's page table. However, the following check for pmd sharing is in copy_hugetlb_page_range. /* If the pagetables are shared don't copy or take references */ if (dst_pte == src_pte) continue; Since process C is sharing with process A instead of process B, the above test fails. The code in copy_hugetlb_page_range which follows assumes dst_pte points to a huge_pte_none pte. It copies the pte entry from src_pte to dst_pte and increments this map count of the associated page. This is how we end up with an elevated map count. To solve, check the dst_pte entry for huge_pte_none. If !none, this implies PMD sharing so do not copy. Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> --- mm/hugetlb.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)