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 <[email protected]>
---
mm/hugetlb.c | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
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))) {
--
2.17.2
On Mon, 5 Nov 2018 13:23:15 -0800 Mike Kravetz <[email protected]> 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 <[email protected]> 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: <[email protected]>
Let me know if you want me to resend with these.
--
Mike Kravetz
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 <[email protected]>
Reviewed-by: Naoya Horiguchi <[email protected]>