* [PATCH v3 1/4] mm: remove unnecessary smp_wmb() in collapse_huge_page()
@ 2019-09-24 23:24 ` Yu Zhao
0 siblings, 0 replies; 44+ messages in thread
From: Yu Zhao @ 2019-09-24 23:24 UTC (permalink / raw)
To: Andrew Morton, Michal Hocko, Kirill A . Shutemov
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Vlastimil Babka,
Hugh Dickins, Jérôme Glisse, Andrea Arcangeli,
Aneesh Kumar K . V, David Rientjes, Matthew Wilcox, Lance Roy,
Ralph Campbell, Jason Gunthorpe, Dave Airlie, Thomas Hellstrom,
Souptick Joarder, Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying,
Aaron Lu, Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm, Yu Zhao
__SetPageUptodate() always has a built-in smp_wmb() to make sure
user data copied to a new page appears before set_pmd_at().
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
mm/khugepaged.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index ccede2425c3f..70ff98e1414d 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1067,13 +1067,6 @@ static void collapse_huge_page(struct mm_struct *mm,
_pmd = mk_huge_pmd(new_page, vma->vm_page_prot);
_pmd = maybe_pmd_mkwrite(pmd_mkdirty(_pmd), vma);
- /*
- * spin_lock() below is not the equivalent of smp_wmb(), so
- * this is needed to avoid the copy_huge_page writes to become
- * visible after the set_pmd_at() write.
- */
- smp_wmb();
-
spin_lock(pmd_ptl);
BUG_ON(!pmd_none(*pmd));
page_add_new_anon_rmap(new_page, vma, address, true);
--
2.23.0.351.gc4317032e6-goog
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v3 2/4] mm: don't expose hugetlb page to fast gup prematurely
2019-09-24 23:24 ` Yu Zhao
@ 2019-09-24 23:24 ` Yu Zhao
-1 siblings, 0 replies; 44+ messages in thread
From: Yu Zhao @ 2019-09-24 23:24 UTC (permalink / raw)
To: Andrew Morton, Michal Hocko, Kirill A . Shutemov
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Vlastimil Babka,
Hugh Dickins, Jérôme Glisse, Andrea Arcangeli,
Aneesh Kumar K . V, David Rientjes, Matthew Wilcox, Lance Roy,
Ralph Campbell, Jason Gunthorpe, Dave Airlie, Thomas Hellstrom,
Souptick Joarder, Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying,
Aaron Lu, Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm, Yu Zhao
We don't want to expose a hugetlb page to the fast gup running on a
remote CPU before the local non-atomic op __SetPageUptodate() is
visible first.
For a hugetlb page, there is no memory barrier between the non-atomic
op and set_huge_pte_at(). Therefore, the page can appear to the fast
gup before the flag does. There is no evidence this would cause any
problem, but there is no point risking the race either.
This patch simply replace 3 uses of the non-atomic op with its atomic
version though out mm/hugetlb.c. The only one left in
hugetlbfs_fallocate() is safe because huge_add_to_page_cache() serves
as a valid write barrier.
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
mm/hugetlb.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 6d7296dd11b8..0be5b7937085 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3693,7 +3693,7 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma,
copy_user_huge_page(new_page, old_page, address, vma,
pages_per_huge_page(h));
- __SetPageUptodate(new_page);
+ SetPageUptodate(new_page);
mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, mm, haddr,
haddr + huge_page_size(h));
@@ -3879,7 +3879,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
goto out;
}
clear_huge_page(page, address, pages_per_huge_page(h));
- __SetPageUptodate(page);
+ SetPageUptodate(page);
new_page = true;
if (vma->vm_flags & VM_MAYSHARE) {
@@ -4180,11 +4180,11 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
}
/*
- * The memory barrier inside __SetPageUptodate makes sure that
+ * The memory barrier inside SetPageUptodate makes sure that
* preceding stores to the page contents become visible before
* the set_pte_at() write.
*/
- __SetPageUptodate(page);
+ SetPageUptodate(page);
mapping = dst_vma->vm_file->f_mapping;
idx = vma_hugecache_offset(h, dst_vma, dst_addr);
--
2.23.0.351.gc4317032e6-goog
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 2/4] mm: don't expose hugetlb page to fast gup prematurely
@ 2019-09-24 23:24 ` Yu Zhao
0 siblings, 0 replies; 44+ messages in thread
From: Yu Zhao @ 2019-09-24 23:24 UTC (permalink / raw)
To: Andrew Morton, Michal Hocko, Kirill A . Shutemov
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Vlastimil Babka,
Hugh Dickins, Jérôme Glisse, Andrea Arcangeli,
Aneesh Kumar K . V, David Rientjes, Matthew Wilcox, Lance Roy,
Ralph Campbell, Jason Gunthorpe, Dave Airlie, Thomas Hellstrom,
Souptick Joarder, Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying,
Aaron Lu, Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm, Yu Zhao
We don't want to expose a hugetlb page to the fast gup running on a
remote CPU before the local non-atomic op __SetPageUptodate() is
visible first.
For a hugetlb page, there is no memory barrier between the non-atomic
op and set_huge_pte_at(). Therefore, the page can appear to the fast
gup before the flag does. There is no evidence this would cause any
problem, but there is no point risking the race either.
This patch simply replace 3 uses of the non-atomic op with its atomic
version though out mm/hugetlb.c. The only one left in
hugetlbfs_fallocate() is safe because huge_add_to_page_cache() serves
as a valid write barrier.
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
mm/hugetlb.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 6d7296dd11b8..0be5b7937085 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3693,7 +3693,7 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma,
copy_user_huge_page(new_page, old_page, address, vma,
pages_per_huge_page(h));
- __SetPageUptodate(new_page);
+ SetPageUptodate(new_page);
mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, mm, haddr,
haddr + huge_page_size(h));
@@ -3879,7 +3879,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
goto out;
}
clear_huge_page(page, address, pages_per_huge_page(h));
- __SetPageUptodate(page);
+ SetPageUptodate(page);
new_page = true;
if (vma->vm_flags & VM_MAYSHARE) {
@@ -4180,11 +4180,11 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
}
/*
- * The memory barrier inside __SetPageUptodate makes sure that
+ * The memory barrier inside SetPageUptodate makes sure that
* preceding stores to the page contents become visible before
* the set_pte_at() write.
*/
- __SetPageUptodate(page);
+ SetPageUptodate(page);
mapping = dst_vma->vm_file->f_mapping;
idx = vma_hugecache_offset(h, dst_vma, dst_addr);
--
2.23.0.351.gc4317032e6-goog
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-09-24 23:24 ` Yu Zhao
@ 2019-09-24 23:24 ` Yu Zhao
-1 siblings, 0 replies; 44+ messages in thread
From: Yu Zhao @ 2019-09-24 23:24 UTC (permalink / raw)
To: Andrew Morton, Michal Hocko, Kirill A . Shutemov
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Vlastimil Babka,
Hugh Dickins, Jérôme Glisse, Andrea Arcangeli,
Aneesh Kumar K . V, David Rientjes, Matthew Wilcox, Lance Roy,
Ralph Campbell, Jason Gunthorpe, Dave Airlie, Thomas Hellstrom,
Souptick Joarder, Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying,
Aaron Lu, Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm, Yu Zhao
We don't want to expose a non-hugetlb page to the fast gup running
on a remote CPU before all local non-atomic ops on the page flags
are visible first.
For an anon page that isn't in swap cache, we need to make sure all
prior non-atomic ops, especially __SetPageSwapBacked() in
page_add_new_anon_rmap(), are ordered before set_pte_at() to prevent
the following race:
CPU 1 CPU1
set_pte_at() get_user_pages_fast()
page_add_new_anon_rmap() gup_pte_range()
__SetPageSwapBacked() SetPageReferenced()
This demonstrates a non-fatal scenario. Though haven't been directly
observed, the fatal ones can exist, e.g., PG_lock set by fast gup
caller and then overwritten by __SetPageSwapBacked().
For an anon page that is already in swap cache or a file page, we
don't need smp_wmb() before set_pte_at() because adding to swap or
file cach serves as a valid write barrier. Using non-atomic ops
thereafter is a bug, obviously.
smp_wmb() is added following 11 of total 12 page_add_new_anon_rmap()
call sites, with the only exception being
do_huge_pmd_wp_page_fallback() because of an existing smp_wmb().
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
kernel/events/uprobes.c | 2 ++
mm/huge_memory.c | 6 ++++++
mm/khugepaged.c | 2 ++
mm/memory.c | 10 +++++++++-
mm/migrate.c | 2 ++
mm/swapfile.c | 6 ++++--
mm/userfaultfd.c | 2 ++
7 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 84fa00497c49..7069785e2e52 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -194,6 +194,8 @@ static int __replace_page(struct vm_area_struct *vma, unsigned long addr,
flush_cache_page(vma, addr, pte_pfn(*pvmw.pte));
ptep_clear_flush_notify(vma, addr, pvmw.pte);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
set_pte_at_notify(mm, addr, pvmw.pte,
mk_pte(new_page, vma->vm_page_prot));
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index de1f15969e27..21d271a29d96 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -616,6 +616,8 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
mem_cgroup_commit_charge(page, memcg, false, true);
lru_cache_add_active_or_unevictable(page, vma);
pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
set_pmd_at(vma->vm_mm, haddr, vmf->pmd, entry);
add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
mm_inc_nr_ptes(vma->vm_mm);
@@ -1276,7 +1278,9 @@ static vm_fault_t do_huge_pmd_wp_page_fallback(struct vm_fault *vmf,
}
kfree(pages);
+ /* commit non-atomic ops before exposing to fast gup */
smp_wmb(); /* make pte visible before pmd */
+
pmd_populate(vma->vm_mm, vmf->pmd, pgtable);
page_remove_rmap(page, true);
spin_unlock(vmf->ptl);
@@ -1423,6 +1427,8 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf, pmd_t orig_pmd)
page_add_new_anon_rmap(new_page, vma, haddr, true);
mem_cgroup_commit_charge(new_page, memcg, false, true);
lru_cache_add_active_or_unevictable(new_page, vma);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
set_pmd_at(vma->vm_mm, haddr, vmf->pmd, entry);
update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
if (!page) {
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 70ff98e1414d..f2901edce6de 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1074,6 +1074,8 @@ static void collapse_huge_page(struct mm_struct *mm,
count_memcg_events(memcg, THP_COLLAPSE_ALLOC, 1);
lru_cache_add_active_or_unevictable(new_page, vma);
pgtable_trans_huge_deposit(mm, pmd, pgtable);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
set_pmd_at(mm, address, pmd, _pmd);
update_mmu_cache_pmd(vma, address, pmd);
spin_unlock(pmd_ptl);
diff --git a/mm/memory.c b/mm/memory.c
index aa86852d9ec2..6dabbc3cd3b7 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2367,6 +2367,8 @@ static vm_fault_t wp_page_copy(struct vm_fault *vmf)
* mmu page tables (such as kvm shadow page tables), we want the
* new page to be mapped directly into the secondary page table.
*/
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
set_pte_at_notify(mm, vmf->address, vmf->pte, entry);
update_mmu_cache(vma, vmf->address, vmf->pte);
if (old_page) {
@@ -2877,7 +2879,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
flush_icache_page(vma, page);
if (pte_swp_soft_dirty(vmf->orig_pte))
pte = pte_mksoft_dirty(pte);
- set_pte_at(vma->vm_mm, vmf->address, vmf->pte, pte);
arch_do_swap_page(vma->vm_mm, vma, vmf->address, pte, vmf->orig_pte);
vmf->orig_pte = pte;
@@ -2886,12 +2887,15 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
page_add_new_anon_rmap(page, vma, vmf->address, false);
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, vma);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
} else {
do_page_add_anon_rmap(page, vma, vmf->address, exclusive);
mem_cgroup_commit_charge(page, memcg, true, false);
activate_page(page);
}
+ set_pte_at(vma->vm_mm, vmf->address, vmf->pte, pte);
swap_free(entry);
if (mem_cgroup_swap_full(page) ||
(vma->vm_flags & VM_LOCKED) || PageMlocked(page))
@@ -3034,6 +3038,8 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
page_add_new_anon_rmap(page, vma, vmf->address, false);
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, vma);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
setpte:
set_pte_at(vma->vm_mm, vmf->address, vmf->pte, entry);
@@ -3297,6 +3303,8 @@ vm_fault_t alloc_set_pte(struct vm_fault *vmf, struct mem_cgroup *memcg,
page_add_new_anon_rmap(page, vma, vmf->address, false);
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, vma);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
} else {
inc_mm_counter_fast(vma->vm_mm, mm_counter_file(page));
page_add_file_rmap(page, false);
diff --git a/mm/migrate.c b/mm/migrate.c
index 9f4ed4e985c1..943d147ecc3e 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2783,6 +2783,8 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate,
lru_cache_add_active_or_unevictable(page, vma);
get_page(page);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
if (flush) {
flush_cache_page(vma, addr, pte_pfn(*ptep));
ptep_clear_flush_notify(vma, addr, ptep);
diff --git a/mm/swapfile.c b/mm/swapfile.c
index dab43523afdd..5c5547053ee0 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1880,8 +1880,6 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
dec_mm_counter(vma->vm_mm, MM_SWAPENTS);
inc_mm_counter(vma->vm_mm, MM_ANONPAGES);
get_page(page);
- set_pte_at(vma->vm_mm, addr, pte,
- pte_mkold(mk_pte(page, vma->vm_page_prot)));
if (page == swapcache) {
page_add_anon_rmap(page, vma, addr, false);
mem_cgroup_commit_charge(page, memcg, true, false);
@@ -1889,7 +1887,11 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
page_add_new_anon_rmap(page, vma, addr, false);
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, vma);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
}
+ set_pte_at(vma->vm_mm, addr, pte,
+ pte_mkold(mk_pte(page, vma->vm_page_prot)));
swap_free(entry);
/*
* Move the page to the active list so it is not
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index c7ae74ce5ff3..4f92913242a1 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -92,6 +92,8 @@ static int mcopy_atomic_pte(struct mm_struct *dst_mm,
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, dst_vma);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
set_pte_at(dst_mm, dst_addr, dst_pte, _dst_pte);
/* No need to invalidate - it was non-present before */
--
2.23.0.351.gc4317032e6-goog
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
@ 2019-09-24 23:24 ` Yu Zhao
0 siblings, 0 replies; 44+ messages in thread
From: Yu Zhao @ 2019-09-24 23:24 UTC (permalink / raw)
To: Andrew Morton, Michal Hocko, Kirill A . Shutemov
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Vlastimil Babka,
Hugh Dickins, Jérôme Glisse, Andrea Arcangeli,
Aneesh Kumar K . V, David Rientjes, Matthew Wilcox, Lance Roy,
Ralph Campbell, Jason Gunthorpe, Dave Airlie, Thomas Hellstrom,
Souptick Joarder, Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying,
Aaron Lu, Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm, Yu Zhao
We don't want to expose a non-hugetlb page to the fast gup running
on a remote CPU before all local non-atomic ops on the page flags
are visible first.
For an anon page that isn't in swap cache, we need to make sure all
prior non-atomic ops, especially __SetPageSwapBacked() in
page_add_new_anon_rmap(), are ordered before set_pte_at() to prevent
the following race:
CPU 1 CPU1
set_pte_at() get_user_pages_fast()
page_add_new_anon_rmap() gup_pte_range()
__SetPageSwapBacked() SetPageReferenced()
This demonstrates a non-fatal scenario. Though haven't been directly
observed, the fatal ones can exist, e.g., PG_lock set by fast gup
caller and then overwritten by __SetPageSwapBacked().
For an anon page that is already in swap cache or a file page, we
don't need smp_wmb() before set_pte_at() because adding to swap or
file cach serves as a valid write barrier. Using non-atomic ops
thereafter is a bug, obviously.
smp_wmb() is added following 11 of total 12 page_add_new_anon_rmap()
call sites, with the only exception being
do_huge_pmd_wp_page_fallback() because of an existing smp_wmb().
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
kernel/events/uprobes.c | 2 ++
mm/huge_memory.c | 6 ++++++
mm/khugepaged.c | 2 ++
mm/memory.c | 10 +++++++++-
mm/migrate.c | 2 ++
mm/swapfile.c | 6 ++++--
mm/userfaultfd.c | 2 ++
7 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 84fa00497c49..7069785e2e52 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -194,6 +194,8 @@ static int __replace_page(struct vm_area_struct *vma, unsigned long addr,
flush_cache_page(vma, addr, pte_pfn(*pvmw.pte));
ptep_clear_flush_notify(vma, addr, pvmw.pte);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
set_pte_at_notify(mm, addr, pvmw.pte,
mk_pte(new_page, vma->vm_page_prot));
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index de1f15969e27..21d271a29d96 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -616,6 +616,8 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
mem_cgroup_commit_charge(page, memcg, false, true);
lru_cache_add_active_or_unevictable(page, vma);
pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
set_pmd_at(vma->vm_mm, haddr, vmf->pmd, entry);
add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
mm_inc_nr_ptes(vma->vm_mm);
@@ -1276,7 +1278,9 @@ static vm_fault_t do_huge_pmd_wp_page_fallback(struct vm_fault *vmf,
}
kfree(pages);
+ /* commit non-atomic ops before exposing to fast gup */
smp_wmb(); /* make pte visible before pmd */
+
pmd_populate(vma->vm_mm, vmf->pmd, pgtable);
page_remove_rmap(page, true);
spin_unlock(vmf->ptl);
@@ -1423,6 +1427,8 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf, pmd_t orig_pmd)
page_add_new_anon_rmap(new_page, vma, haddr, true);
mem_cgroup_commit_charge(new_page, memcg, false, true);
lru_cache_add_active_or_unevictable(new_page, vma);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
set_pmd_at(vma->vm_mm, haddr, vmf->pmd, entry);
update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
if (!page) {
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 70ff98e1414d..f2901edce6de 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1074,6 +1074,8 @@ static void collapse_huge_page(struct mm_struct *mm,
count_memcg_events(memcg, THP_COLLAPSE_ALLOC, 1);
lru_cache_add_active_or_unevictable(new_page, vma);
pgtable_trans_huge_deposit(mm, pmd, pgtable);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
set_pmd_at(mm, address, pmd, _pmd);
update_mmu_cache_pmd(vma, address, pmd);
spin_unlock(pmd_ptl);
diff --git a/mm/memory.c b/mm/memory.c
index aa86852d9ec2..6dabbc3cd3b7 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2367,6 +2367,8 @@ static vm_fault_t wp_page_copy(struct vm_fault *vmf)
* mmu page tables (such as kvm shadow page tables), we want the
* new page to be mapped directly into the secondary page table.
*/
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
set_pte_at_notify(mm, vmf->address, vmf->pte, entry);
update_mmu_cache(vma, vmf->address, vmf->pte);
if (old_page) {
@@ -2877,7 +2879,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
flush_icache_page(vma, page);
if (pte_swp_soft_dirty(vmf->orig_pte))
pte = pte_mksoft_dirty(pte);
- set_pte_at(vma->vm_mm, vmf->address, vmf->pte, pte);
arch_do_swap_page(vma->vm_mm, vma, vmf->address, pte, vmf->orig_pte);
vmf->orig_pte = pte;
@@ -2886,12 +2887,15 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
page_add_new_anon_rmap(page, vma, vmf->address, false);
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, vma);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
} else {
do_page_add_anon_rmap(page, vma, vmf->address, exclusive);
mem_cgroup_commit_charge(page, memcg, true, false);
activate_page(page);
}
+ set_pte_at(vma->vm_mm, vmf->address, vmf->pte, pte);
swap_free(entry);
if (mem_cgroup_swap_full(page) ||
(vma->vm_flags & VM_LOCKED) || PageMlocked(page))
@@ -3034,6 +3038,8 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
page_add_new_anon_rmap(page, vma, vmf->address, false);
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, vma);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
setpte:
set_pte_at(vma->vm_mm, vmf->address, vmf->pte, entry);
@@ -3297,6 +3303,8 @@ vm_fault_t alloc_set_pte(struct vm_fault *vmf, struct mem_cgroup *memcg,
page_add_new_anon_rmap(page, vma, vmf->address, false);
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, vma);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
} else {
inc_mm_counter_fast(vma->vm_mm, mm_counter_file(page));
page_add_file_rmap(page, false);
diff --git a/mm/migrate.c b/mm/migrate.c
index 9f4ed4e985c1..943d147ecc3e 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2783,6 +2783,8 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate,
lru_cache_add_active_or_unevictable(page, vma);
get_page(page);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
if (flush) {
flush_cache_page(vma, addr, pte_pfn(*ptep));
ptep_clear_flush_notify(vma, addr, ptep);
diff --git a/mm/swapfile.c b/mm/swapfile.c
index dab43523afdd..5c5547053ee0 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1880,8 +1880,6 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
dec_mm_counter(vma->vm_mm, MM_SWAPENTS);
inc_mm_counter(vma->vm_mm, MM_ANONPAGES);
get_page(page);
- set_pte_at(vma->vm_mm, addr, pte,
- pte_mkold(mk_pte(page, vma->vm_page_prot)));
if (page == swapcache) {
page_add_anon_rmap(page, vma, addr, false);
mem_cgroup_commit_charge(page, memcg, true, false);
@@ -1889,7 +1887,11 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
page_add_new_anon_rmap(page, vma, addr, false);
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, vma);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
}
+ set_pte_at(vma->vm_mm, addr, pte,
+ pte_mkold(mk_pte(page, vma->vm_page_prot)));
swap_free(entry);
/*
* Move the page to the active list so it is not
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index c7ae74ce5ff3..4f92913242a1 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -92,6 +92,8 @@ static int mcopy_atomic_pte(struct mm_struct *dst_mm,
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, dst_vma);
+ /* commit non-atomic ops before exposing to fast gup */
+ smp_wmb();
set_pte_at(dst_mm, dst_addr, dst_pte, _dst_pte);
/* No need to invalidate - it was non-present before */
--
2.23.0.351.gc4317032e6-goog
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-09-24 23:24 ` Yu Zhao
(?)
@ 2019-09-25 8:25 ` Peter Zijlstra
2019-09-25 22:26 ` Yu Zhao
-1 siblings, 1 reply; 44+ messages in thread
From: Peter Zijlstra @ 2019-09-25 8:25 UTC (permalink / raw)
To: Yu Zhao
Cc: Andrew Morton, Michal Hocko, Kirill A . Shutemov, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Vlastimil Babka, Hugh Dickins,
Jérôme Glisse, Andrea Arcangeli, Aneesh Kumar K . V,
David Rientjes, Matthew Wilcox, Lance Roy, Ralph Campbell,
Jason Gunthorpe, Dave Airlie, Thomas Hellstrom, Souptick Joarder,
Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying, Aaron Lu,
Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm
On Tue, Sep 24, 2019 at 05:24:58PM -0600, Yu Zhao wrote:
> We don't want to expose a non-hugetlb page to the fast gup running
> on a remote CPU before all local non-atomic ops on the page flags
> are visible first.
>
> For an anon page that isn't in swap cache, we need to make sure all
> prior non-atomic ops, especially __SetPageSwapBacked() in
> page_add_new_anon_rmap(), are ordered before set_pte_at() to prevent
> the following race:
>
> CPU 1 CPU1
> set_pte_at() get_user_pages_fast()
> page_add_new_anon_rmap() gup_pte_range()
> __SetPageSwapBacked() SetPageReferenced()
>
> This demonstrates a non-fatal scenario. Though haven't been directly
> observed, the fatal ones can exist, e.g., PG_lock set by fast gup
> caller and then overwritten by __SetPageSwapBacked().
>
> For an anon page that is already in swap cache or a file page, we
> don't need smp_wmb() before set_pte_at() because adding to swap or
> file cach serves as a valid write barrier. Using non-atomic ops
> thereafter is a bug, obviously.
>
> smp_wmb() is added following 11 of total 12 page_add_new_anon_rmap()
> call sites, with the only exception being
> do_huge_pmd_wp_page_fallback() because of an existing smp_wmb().
>
I'm thinking this patch make stuff rather fragile.. Should we instead
stick the barrier in set_p*d_at() instead? Or rather, make that store a
store-release?
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-09-25 8:25 ` Peter Zijlstra
@ 2019-09-25 22:26 ` Yu Zhao
2019-09-26 10:20 ` Kirill A. Shutemov
0 siblings, 1 reply; 44+ messages in thread
From: Yu Zhao @ 2019-09-25 22:26 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Andrew Morton, Michal Hocko, Kirill A . Shutemov, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Vlastimil Babka, Hugh Dickins,
Jérôme Glisse, Andrea Arcangeli, Aneesh Kumar K . V,
David Rientjes, Matthew Wilcox, Lance Roy, Ralph Campbell,
Jason Gunthorpe, Dave Airlie, Thomas Hellstrom, Souptick Joarder,
Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying, Aaron Lu,
Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm
On Wed, Sep 25, 2019 at 10:25:30AM +0200, Peter Zijlstra wrote:
> On Tue, Sep 24, 2019 at 05:24:58PM -0600, Yu Zhao wrote:
> > We don't want to expose a non-hugetlb page to the fast gup running
> > on a remote CPU before all local non-atomic ops on the page flags
> > are visible first.
> >
> > For an anon page that isn't in swap cache, we need to make sure all
> > prior non-atomic ops, especially __SetPageSwapBacked() in
> > page_add_new_anon_rmap(), are ordered before set_pte_at() to prevent
> > the following race:
> >
> > CPU 1 CPU1
> > set_pte_at() get_user_pages_fast()
> > page_add_new_anon_rmap() gup_pte_range()
> > __SetPageSwapBacked() SetPageReferenced()
> >
> > This demonstrates a non-fatal scenario. Though haven't been directly
> > observed, the fatal ones can exist, e.g., PG_lock set by fast gup
> > caller and then overwritten by __SetPageSwapBacked().
> >
> > For an anon page that is already in swap cache or a file page, we
> > don't need smp_wmb() before set_pte_at() because adding to swap or
> > file cach serves as a valid write barrier. Using non-atomic ops
> > thereafter is a bug, obviously.
> >
> > smp_wmb() is added following 11 of total 12 page_add_new_anon_rmap()
> > call sites, with the only exception being
> > do_huge_pmd_wp_page_fallback() because of an existing smp_wmb().
> >
>
> I'm thinking this patch make stuff rather fragile.. Should we instead
> stick the barrier in set_p*d_at() instead? Or rather, make that store a
> store-release?
I prefer it this way too, but I suspected the majority would be
concerned with the performance implications, especially those
looping set_pte_at()s in mm/huge_memory.c.
If we have a consensus that smp_wmb() would be better off wrapped
together with set_p*d_at() in a macro, I'd be glad to make the change.
And it seems to me applying smp_store_release() would have to happen
in each arch, which might be just as fragile.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-09-25 22:26 ` Yu Zhao
@ 2019-09-26 10:20 ` Kirill A. Shutemov
2019-09-27 3:26 ` John Hubbard
0 siblings, 1 reply; 44+ messages in thread
From: Kirill A. Shutemov @ 2019-09-26 10:20 UTC (permalink / raw)
To: Yu Zhao
Cc: Peter Zijlstra, Andrew Morton, Michal Hocko, Kirill A . Shutemov,
Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Vlastimil Babka, Hugh Dickins,
Jérôme Glisse, Andrea Arcangeli, Aneesh Kumar K . V,
David Rientjes, Matthew Wilcox, Lance Roy, Ralph Campbell,
Jason Gunthorpe, Dave Airlie, Thomas Hellstrom, Souptick Joarder,
Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying, Aaron Lu,
Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm
On Wed, Sep 25, 2019 at 04:26:54PM -0600, Yu Zhao wrote:
> On Wed, Sep 25, 2019 at 10:25:30AM +0200, Peter Zijlstra wrote:
> > On Tue, Sep 24, 2019 at 05:24:58PM -0600, Yu Zhao wrote:
> > > We don't want to expose a non-hugetlb page to the fast gup running
> > > on a remote CPU before all local non-atomic ops on the page flags
> > > are visible first.
> > >
> > > For an anon page that isn't in swap cache, we need to make sure all
> > > prior non-atomic ops, especially __SetPageSwapBacked() in
> > > page_add_new_anon_rmap(), are ordered before set_pte_at() to prevent
> > > the following race:
> > >
> > > CPU 1 CPU1
> > > set_pte_at() get_user_pages_fast()
> > > page_add_new_anon_rmap() gup_pte_range()
> > > __SetPageSwapBacked() SetPageReferenced()
> > >
> > > This demonstrates a non-fatal scenario. Though haven't been directly
> > > observed, the fatal ones can exist, e.g., PG_lock set by fast gup
> > > caller and then overwritten by __SetPageSwapBacked().
> > >
> > > For an anon page that is already in swap cache or a file page, we
> > > don't need smp_wmb() before set_pte_at() because adding to swap or
> > > file cach serves as a valid write barrier. Using non-atomic ops
> > > thereafter is a bug, obviously.
> > >
> > > smp_wmb() is added following 11 of total 12 page_add_new_anon_rmap()
> > > call sites, with the only exception being
> > > do_huge_pmd_wp_page_fallback() because of an existing smp_wmb().
> > >
> >
> > I'm thinking this patch make stuff rather fragile.. Should we instead
> > stick the barrier in set_p*d_at() instead? Or rather, make that store a
> > store-release?
>
> I prefer it this way too, but I suspected the majority would be
> concerned with the performance implications, especially those
> looping set_pte_at()s in mm/huge_memory.c.
We can rename current set_pte_at() to __set_pte_at() or something and
leave it in places where barrier is not needed. The new set_pte_at()( will
be used in the rest of the places with the barrier inside.
BTW, have you looked at other levels of page table hierarchy. Do we have
the same issue for PMD/PUD/... pages?
--
Kirill A. Shutemov
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-09-26 10:20 ` Kirill A. Shutemov
@ 2019-09-27 3:26 ` John Hubbard
2019-09-27 5:06 ` Yu Zhao
2019-09-27 12:33 ` Michal Hocko
0 siblings, 2 replies; 44+ messages in thread
From: John Hubbard @ 2019-09-27 3:26 UTC (permalink / raw)
To: Kirill A. Shutemov, Yu Zhao
Cc: Peter Zijlstra, Andrew Morton, Michal Hocko, Kirill A . Shutemov,
Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Vlastimil Babka, Hugh Dickins,
Jérôme Glisse, Andrea Arcangeli, Aneesh Kumar K . V,
David Rientjes, Matthew Wilcox, Lance Roy, Ralph Campbell,
Jason Gunthorpe, Dave Airlie, Thomas Hellstrom, Souptick Joarder,
Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying, Aaron Lu,
Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm
On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:
> On Wed, Sep 25, 2019 at 04:26:54PM -0600, Yu Zhao wrote:
>> On Wed, Sep 25, 2019 at 10:25:30AM +0200, Peter Zijlstra wrote:
>>> On Tue, Sep 24, 2019 at 05:24:58PM -0600, Yu Zhao wrote:
...
>>> I'm thinking this patch make stuff rather fragile.. Should we instead
>>> stick the barrier in set_p*d_at() instead? Or rather, make that store a
>>> store-release?
>>
>> I prefer it this way too, but I suspected the majority would be
>> concerned with the performance implications, especially those
>> looping set_pte_at()s in mm/huge_memory.c.
>
> We can rename current set_pte_at() to __set_pte_at() or something and
> leave it in places where barrier is not needed. The new set_pte_at()( will
> be used in the rest of the places with the barrier inside.
+1, sounds nice. I was unhappy about the wide-ranging changes that would have
to be maintained. So this seems much better.
>
> BTW, have you looked at other levels of page table hierarchy. Do we have
> the same issue for PMD/PUD/... pages?
>
Along the lines of "what other memory barriers might be missing for
get_user_pages_fast(), I'm also concerned that the synchronization between
get_user_pages_fast() and freeing the page tables might be technically broken,
due to missing memory barriers on the get_user_pages_fast() side. Details:
gup_fast() disables interrupts, but I think it also needs some sort of
memory barrier(s), in order to prevent reads of the page table (gup_pgd_range,
etc) from speculatively happening before the interrupts are disabled.
Leonardo Bras's recent patchset brought this to my attention. Here, he's
recommending adding atomic counting inc/dec before and after the gup_fast()
irq disable/enable points:
https://lore.kernel.org/r/20190920195047.7703-4-leonardo@linux.ibm.com
...and that lead to noticing a general lack of barriers there.
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-09-27 3:26 ` John Hubbard
@ 2019-09-27 5:06 ` Yu Zhao
2019-10-01 22:31 ` John Hubbard
2019-09-27 12:33 ` Michal Hocko
1 sibling, 1 reply; 44+ messages in thread
From: Yu Zhao @ 2019-09-27 5:06 UTC (permalink / raw)
To: John Hubbard
Cc: Kirill A. Shutemov, Peter Zijlstra, Andrew Morton, Michal Hocko,
Kirill A . Shutemov, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Vlastimil Babka,
Hugh Dickins, Jérôme Glisse, Andrea Arcangeli,
Aneesh Kumar K . V, David Rientjes, Matthew Wilcox, Lance Roy,
Ralph Campbell, Jason Gunthorpe, Dave Airlie, Thomas Hellstrom,
Souptick Joarder, Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying,
Aaron Lu, Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm
On Thu, Sep 26, 2019 at 08:26:46PM -0700, John Hubbard wrote:
> On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:
> > On Wed, Sep 25, 2019 at 04:26:54PM -0600, Yu Zhao wrote:
> >> On Wed, Sep 25, 2019 at 10:25:30AM +0200, Peter Zijlstra wrote:
> >>> On Tue, Sep 24, 2019 at 05:24:58PM -0600, Yu Zhao wrote:
> ...
> >>> I'm thinking this patch make stuff rather fragile.. Should we instead
> >>> stick the barrier in set_p*d_at() instead? Or rather, make that store a
> >>> store-release?
> >>
> >> I prefer it this way too, but I suspected the majority would be
> >> concerned with the performance implications, especially those
> >> looping set_pte_at()s in mm/huge_memory.c.
> >
> > We can rename current set_pte_at() to __set_pte_at() or something and
> > leave it in places where barrier is not needed. The new set_pte_at()( will
> > be used in the rest of the places with the barrier inside.
>
> +1, sounds nice. I was unhappy about the wide-ranging changes that would have
> to be maintained. So this seems much better.
Just to be clear that doing so will add unnecessary barriers to one
of the two paths that share set_pte_at().
> > BTW, have you looked at other levels of page table hierarchy. Do we have
> > the same issue for PMD/PUD/... pages?
> >
>
> Along the lines of "what other memory barriers might be missing for
> get_user_pages_fast(), I'm also concerned that the synchronization between
> get_user_pages_fast() and freeing the page tables might be technically broken,
> due to missing memory barriers on the get_user_pages_fast() side. Details:
>
> gup_fast() disables interrupts, but I think it also needs some sort of
> memory barrier(s), in order to prevent reads of the page table (gup_pgd_range,
> etc) from speculatively happening before the interrupts are disabled.
I was under impression switching back from interrupt context is a
full barrier (otherwise wouldn't we be vulnerable to some side
channel attacks?), so the reader side wouldn't need explicit rmb.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-09-27 5:06 ` Yu Zhao
@ 2019-10-01 22:31 ` John Hubbard
2019-10-02 0:00 ` Yu Zhao
0 siblings, 1 reply; 44+ messages in thread
From: John Hubbard @ 2019-10-01 22:31 UTC (permalink / raw)
To: Yu Zhao
Cc: Kirill A. Shutemov, Peter Zijlstra, Andrew Morton, Michal Hocko,
Kirill A . Shutemov, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Vlastimil Babka,
Hugh Dickins, Jérôme Glisse, Andrea Arcangeli,
Aneesh Kumar K . V, David Rientjes, Matthew Wilcox, Lance Roy,
Ralph Campbell, Jason Gunthorpe, Dave Airlie, Thomas Hellstrom,
Souptick Joarder, Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying,
Aaron Lu, Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm
On 9/26/19 10:06 PM, Yu Zhao wrote:
> On Thu, Sep 26, 2019 at 08:26:46PM -0700, John Hubbard wrote:
>> On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:
>>> On Wed, Sep 25, 2019 at 04:26:54PM -0600, Yu Zhao wrote:
>>>> On Wed, Sep 25, 2019 at 10:25:30AM +0200, Peter Zijlstra wrote:
>>>>> On Tue, Sep 24, 2019 at 05:24:58PM -0600, Yu Zhao wrote:
>> ...
>>>>> I'm thinking this patch make stuff rather fragile.. Should we instead
>>>>> stick the barrier in set_p*d_at() instead? Or rather, make that store a
>>>>> store-release?
>>>>
>>>> I prefer it this way too, but I suspected the majority would be
>>>> concerned with the performance implications, especially those
>>>> looping set_pte_at()s in mm/huge_memory.c.
>>>
>>> We can rename current set_pte_at() to __set_pte_at() or something and
>>> leave it in places where barrier is not needed. The new set_pte_at()( will
>>> be used in the rest of the places with the barrier inside.
>>
>> +1, sounds nice. I was unhappy about the wide-ranging changes that would have
>> to be maintained. So this seems much better.
>
> Just to be clear that doing so will add unnecessary barriers to one
> of the two paths that share set_pte_at().
Good point, maybe there's a better place to do it...
>
>>> BTW, have you looked at other levels of page table hierarchy. Do we have
>>> the same issue for PMD/PUD/... pages?
>>>
>>
>> Along the lines of "what other memory barriers might be missing for
>> get_user_pages_fast(), I'm also concerned that the synchronization between
>> get_user_pages_fast() and freeing the page tables might be technically broken,
>> due to missing memory barriers on the get_user_pages_fast() side. Details:
>>
>> gup_fast() disables interrupts, but I think it also needs some sort of
>> memory barrier(s), in order to prevent reads of the page table (gup_pgd_range,
>> etc) from speculatively happening before the interrupts are disabled.
>
> I was under impression switching back from interrupt context is a
> full barrier (otherwise wouldn't we be vulnerable to some side
> channel attacks?), so the reader side wouldn't need explicit rmb.
>
Documentation/memory-barriers.txt points out:
INTERRUPT DISABLING FUNCTIONS
-----------------------------
Functions that disable interrupts (ACQUIRE equivalent) and enable interrupts
(RELEASE equivalent) will act as compiler barriers only. So if memory or I/O
barriers are required in such a situation, they must be provided from some
other means.
btw, I'm really sorry I missed your responses over the last 3 or 4 days.
I just tracked down something in our email system that was sometimes
moving some emails to spam (just few enough to escape immediate attention, argghh!).
I think I killed it off for good now. I wasn't ignoring you. :)
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-10-01 22:31 ` John Hubbard
@ 2019-10-02 0:00 ` Yu Zhao
0 siblings, 0 replies; 44+ messages in thread
From: Yu Zhao @ 2019-10-02 0:00 UTC (permalink / raw)
To: John Hubbard, Mark Rutland
Cc: Kirill A. Shutemov, Peter Zijlstra, Andrew Morton, Michal Hocko,
Kirill A . Shutemov, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Vlastimil Babka,
Hugh Dickins, Jérôme Glisse, Andrea Arcangeli,
Aneesh Kumar K . V, David Rientjes, Matthew Wilcox, Lance Roy,
Ralph Campbell, Jason Gunthorpe, Dave Airlie, Thomas Hellstrom,
Souptick Joarder, Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying,
Aaron Lu, Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Alexander Duyck,
Pavel Tatashin, David Hildenbrand, Juergen Gross, Anthony Yznaga,
Johannes Weiner, Darrick J . Wong, linux-kernel, linux-mm
On Tue, Oct 01, 2019 at 03:31:51PM -0700, John Hubbard wrote:
> On 9/26/19 10:06 PM, Yu Zhao wrote:
> > On Thu, Sep 26, 2019 at 08:26:46PM -0700, John Hubbard wrote:
> >> On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:
> >>> On Wed, Sep 25, 2019 at 04:26:54PM -0600, Yu Zhao wrote:
> >>>> On Wed, Sep 25, 2019 at 10:25:30AM +0200, Peter Zijlstra wrote:
> >>>>> On Tue, Sep 24, 2019 at 05:24:58PM -0600, Yu Zhao wrote:
> >> ...
> >>>>> I'm thinking this patch make stuff rather fragile.. Should we instead
> >>>>> stick the barrier in set_p*d_at() instead? Or rather, make that store a
> >>>>> store-release?
> >>>>
> >>>> I prefer it this way too, but I suspected the majority would be
> >>>> concerned with the performance implications, especially those
> >>>> looping set_pte_at()s in mm/huge_memory.c.
> >>>
> >>> We can rename current set_pte_at() to __set_pte_at() or something and
> >>> leave it in places where barrier is not needed. The new set_pte_at()( will
> >>> be used in the rest of the places with the barrier inside.
> >>
> >> +1, sounds nice. I was unhappy about the wide-ranging changes that would have
> >> to be maintained. So this seems much better.
> >
> > Just to be clear that doing so will add unnecessary barriers to one
> > of the two paths that share set_pte_at().
>
> Good point, maybe there's a better place to do it...
>
>
> >
> >>> BTW, have you looked at other levels of page table hierarchy. Do we have
> >>> the same issue for PMD/PUD/... pages?
> >>>
> >>
> >> Along the lines of "what other memory barriers might be missing for
> >> get_user_pages_fast(), I'm also concerned that the synchronization between
> >> get_user_pages_fast() and freeing the page tables might be technically broken,
> >> due to missing memory barriers on the get_user_pages_fast() side. Details:
> >>
> >> gup_fast() disables interrupts, but I think it also needs some sort of
> >> memory barrier(s), in order to prevent reads of the page table (gup_pgd_range,
> >> etc) from speculatively happening before the interrupts are disabled.
> >
> > I was under impression switching back from interrupt context is a
> > full barrier (otherwise wouldn't we be vulnerable to some side
> > channel attacks?), so the reader side wouldn't need explicit rmb.
> >
>
> Documentation/memory-barriers.txt points out:
>
> INTERRUPT DISABLING FUNCTIONS
> -----------------------------
>
> Functions that disable interrupts (ACQUIRE equivalent) and enable interrupts
> (RELEASE equivalent) will act as compiler barriers only. So if memory or I/O
> barriers are required in such a situation, they must be provided from some
> other means.
>
> btw, I'm really sorry I missed your responses over the last 3 or 4 days.
> I just tracked down something in our email system that was sometimes
> moving some emails to spam (just few enough to escape immediate attention, argghh!).
> I think I killed it off for good now. I wasn't ignoring you. :)
Thanks, John. I agree with all you said, including the irq disabling
function not being a sufficient smp_rmb().
I was hoping somebody could clarify whether ipi handlers used by tlb
flush are sufficient to prevent CPU 1 from seeing any stale data from
freed page tables on all supported archs.
CPU 1 CPU 2
flush remote tlb by ipi
wait for the ipi hanlder
<ipi handler>
free page table
disable irq
walk page table
enable irq
I think they should because otherwise tlb flush wouldn't work if CPU 1
still sees stale data from the freed page table, unless there is a
really strange CPU cache design I'm not aware of.
Quoting comments from x86 ipi handler flush_tlb_func_common():
* read active_mm's tlb_gen. We don't need any explicit barriers
* because all x86 flush operations are serializing and the
* atomic64_read operation won't be reordered by the compiler.
For ppc64 ipi hander radix__flush_tlb_range(), there is an "eieio"
instruction:
https://www.ibm.com/support/knowledgecenter/en/ssw_aix_72/assembler/idalangref_eieio_instrs.html
I'm not sure why it's not "sync" -- I'd guess something implicitly
works as "sync" already (or it's a bug).
I didn't find an ipi handler for tlb flush on arm64. There should be
one, otherwise fast gup on arm64 would be broken. Mark?
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-09-27 3:26 ` John Hubbard
2019-09-27 5:06 ` Yu Zhao
@ 2019-09-27 12:33 ` Michal Hocko
2019-09-27 18:31 ` Yu Zhao
2019-09-27 19:31 ` John Hubbard
1 sibling, 2 replies; 44+ messages in thread
From: Michal Hocko @ 2019-09-27 12:33 UTC (permalink / raw)
To: John Hubbard
Cc: Kirill A. Shutemov, Yu Zhao, Peter Zijlstra, Andrew Morton,
Kirill A . Shutemov, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Vlastimil Babka,
Hugh Dickins, Jérôme Glisse, Andrea Arcangeli,
Aneesh Kumar K . V, David Rientjes, Matthew Wilcox, Lance Roy,
Ralph Campbell, Jason Gunthorpe, Dave Airlie, Thomas Hellstrom,
Souptick Joarder, Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying,
Aaron Lu, Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm
On Thu 26-09-19 20:26:46, John Hubbard wrote:
> On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:
> > BTW, have you looked at other levels of page table hierarchy. Do we have
> > the same issue for PMD/PUD/... pages?
> >
>
> Along the lines of "what other memory barriers might be missing for
> get_user_pages_fast(), I'm also concerned that the synchronization between
> get_user_pages_fast() and freeing the page tables might be technically broken,
> due to missing memory barriers on the get_user_pages_fast() side. Details:
>
> gup_fast() disables interrupts, but I think it also needs some sort of
> memory barrier(s), in order to prevent reads of the page table (gup_pgd_range,
> etc) from speculatively happening before the interrupts are disabled.
Could you be more specific about the race scenario please? I thought
that the unmap path will be serialized by the pte lock.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-09-27 12:33 ` Michal Hocko
@ 2019-09-27 18:31 ` Yu Zhao
2019-09-27 19:31 ` John Hubbard
1 sibling, 0 replies; 44+ messages in thread
From: Yu Zhao @ 2019-09-27 18:31 UTC (permalink / raw)
To: Michal Hocko
Cc: John Hubbard, Kirill A. Shutemov, Peter Zijlstra, Andrew Morton,
Kirill A . Shutemov, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Vlastimil Babka,
Hugh Dickins, Jérôme Glisse, Andrea Arcangeli,
Aneesh Kumar K . V, David Rientjes, Matthew Wilcox, Lance Roy,
Ralph Campbell, Jason Gunthorpe, Dave Airlie, Thomas Hellstrom,
Souptick Joarder, Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying,
Aaron Lu, Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm
On Fri, Sep 27, 2019 at 02:33:00PM +0200, Michal Hocko wrote:
> On Thu 26-09-19 20:26:46, John Hubbard wrote:
> > On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:
> > > BTW, have you looked at other levels of page table hierarchy. Do we have
> > > the same issue for PMD/PUD/... pages?
> > >
> >
> > Along the lines of "what other memory barriers might be missing for
> > get_user_pages_fast(), I'm also concerned that the synchronization between
> > get_user_pages_fast() and freeing the page tables might be technically broken,
> > due to missing memory barriers on the get_user_pages_fast() side. Details:
> >
> > gup_fast() disables interrupts, but I think it also needs some sort of
> > memory barrier(s), in order to prevent reads of the page table (gup_pgd_range,
> > etc) from speculatively happening before the interrupts are disabled.
>
> Could you be more specific about the race scenario please? I thought
> that the unmap path will be serialized by the pte lock.
Yes, the unmap path is protected by ptl, but the fast gup isn't.
Please correct me if I'm wrong, John. This is the hypothetical race:
CPU 1 (gup) CPU 2 (zap)
speculatively load a pmd val
zap the pte table pointed by the pmd val
flush tlb by ipi
<handle ipi>
free the pte table
local_irq_disable()
use the stale pmd val
use-after-free the pte table
local_irq_enable()
I don't think it would happen because the interrupt context on CPU 1
would act as a full mb and enforce a reload of the pmd val. But I'm
not entirely sure.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-09-27 12:33 ` Michal Hocko
2019-09-27 18:31 ` Yu Zhao
@ 2019-09-27 19:31 ` John Hubbard
2019-09-29 22:47 ` John Hubbard
2019-09-30 9:20 ` Jan Kara
1 sibling, 2 replies; 44+ messages in thread
From: John Hubbard @ 2019-09-27 19:31 UTC (permalink / raw)
To: Michal Hocko
Cc: Kirill A. Shutemov, Yu Zhao, Peter Zijlstra, Andrew Morton,
Kirill A . Shutemov, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Vlastimil Babka,
Hugh Dickins, Jérôme Glisse, Andrea Arcangeli,
Aneesh Kumar K . V, David Rientjes, Matthew Wilcox, Lance Roy,
Ralph Campbell, Jason Gunthorpe, Dave Airlie, Thomas Hellstrom,
Souptick Joarder, Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying,
Aaron Lu, Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm
On 9/27/19 5:33 AM, Michal Hocko wrote:
> On Thu 26-09-19 20:26:46, John Hubbard wrote:
>> On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:
>>> BTW, have you looked at other levels of page table hierarchy. Do we have
>>> the same issue for PMD/PUD/... pages?
>>>
>>
>> Along the lines of "what other memory barriers might be missing for
>> get_user_pages_fast(), I'm also concerned that the synchronization between
>> get_user_pages_fast() and freeing the page tables might be technically broken,
>> due to missing memory barriers on the get_user_pages_fast() side. Details:
>>
>> gup_fast() disables interrupts, but I think it also needs some sort of
>> memory barrier(s), in order to prevent reads of the page table (gup_pgd_range,
>> etc) from speculatively happening before the interrupts are disabled.
>
> Could you be more specific about the race scenario please? I thought
> that the unmap path will be serialized by the pte lock.
>
I don't see a pte lock anywhere here.
This case is really pretty far out there, but without run-time memory barriers
I don't think we can completely rule it out:
CPU 0 CPU 1
-------- ---------------
get_user_pages_fast()
do_unmap()
unmap_region()
free_pgtables()
/*
* speculative reads, re-ordered
* by the CPU at run time, not
* compile time. The function calls
* are not really in this order, but
* the corresponding reads could be.
*/
gup_pgd_range()
gup_p4d_range()
gup_pud_range()
gup_pmd_range()
pmd = READ_ONCE(*pmdp)
/* This is stale */
tlb_finish_mmu()
tlb_flush_mmu()
tlb_flush_mmu_tlbonly()
tlb_flush()
flush_tlb_mm
flush_tlb_mm_range
flush_tlb_others
native_flush_tlb_others
smp_call_function_many: IPIs
...blocks until CPU1 reenables
interrupts
local_irq_disable()
...continue the page walk based
on stale/freed data...
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-09-27 19:31 ` John Hubbard
@ 2019-09-29 22:47 ` John Hubbard
2019-09-30 9:20 ` Jan Kara
1 sibling, 0 replies; 44+ messages in thread
From: John Hubbard @ 2019-09-29 22:47 UTC (permalink / raw)
To: Michal Hocko
Cc: Kirill A. Shutemov, Yu Zhao, Peter Zijlstra, Andrew Morton,
Kirill A . Shutemov, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Vlastimil Babka,
Hugh Dickins, Jérôme Glisse, Andrea Arcangeli,
Aneesh Kumar K . V, David Rientjes, Matthew Wilcox, Lance Roy,
Ralph Campbell, Jason Gunthorpe, Dave Airlie, Thomas Hellstrom,
Souptick Joarder, Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying,
Aaron Lu, Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm, Leonardo Bras
On 9/27/19 12:31 PM, John Hubbard wrote:
> On 9/27/19 5:33 AM, Michal Hocko wrote:
>> On Thu 26-09-19 20:26:46, John Hubbard wrote:
>>> On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:
>>>> BTW, have you looked at other levels of page table hierarchy. Do we have
>>>> the same issue for PMD/PUD/... pages?
>>>>
>>>
>>> Along the lines of "what other memory barriers might be missing for
>>> get_user_pages_fast(), I'm also concerned that the synchronization between
>>> get_user_pages_fast() and freeing the page tables might be technically broken,
>>> due to missing memory barriers on the get_user_pages_fast() side. Details:
>>>
>>> gup_fast() disables interrupts, but I think it also needs some sort of
>>> memory barrier(s), in order to prevent reads of the page table (gup_pgd_range,
>>> etc) from speculatively happening before the interrupts are disabled.
>>
>> Could you be more specific about the race scenario please? I thought
>> that the unmap path will be serialized by the pte lock.
>>
>
> I don't see a pte lock anywhere here.
>
> This case is really pretty far out there, but without run-time memory barriers
> I don't think we can completely rule it out:
>
> CPU 0 CPU 1
> -------- ---------------
> get_user_pages_fast()
>
> do_unmap()
> unmap_region()
> free_pgtables()
> /*
> * speculative reads, re-ordered
> * by the CPU at run time, not
> * compile time. The function calls
> * are not really in this order, but
> * the corresponding reads could be.
> */
> gup_pgd_range()
> gup_p4d_range()
> gup_pud_range()
> gup_pmd_range()
> pmd = READ_ONCE(*pmdp)
> /* This is stale */
>
> tlb_finish_mmu()
> tlb_flush_mmu()
> tlb_flush_mmu_tlbonly()
> tlb_flush()
> flush_tlb_mm
> flush_tlb_mm_range
> flush_tlb_others
> native_flush_tlb_others
> smp_call_function_many: IPIs
> ...blocks until CPU1 reenables
> interrupts
>
> local_irq_disable()
> ...continue the page walk based
> on stale/freed data...
>
...and note that I just asked Leonardo Bras (+CC) to include "the" fix[1] for that (I proposed
a snippet that attempts a fix anyway), in his upcoming feature that speeds up PPC THP
operations.
So I hope that doesn't muddy this thread too much, but the other one is awfully quiet
and I was worried that the overlapping changes might go unnoticed.
[1] https://lore.kernel.org/r/4ff1e8e8-929b-9cfc-9bf8-ee88e34de888@nvidia.com
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-09-27 19:31 ` John Hubbard
2019-09-29 22:47 ` John Hubbard
@ 2019-09-30 9:20 ` Jan Kara
2019-09-30 17:57 ` John Hubbard
1 sibling, 1 reply; 44+ messages in thread
From: Jan Kara @ 2019-09-30 9:20 UTC (permalink / raw)
To: John Hubbard
Cc: Michal Hocko, Kirill A. Shutemov, Yu Zhao, Peter Zijlstra,
Andrew Morton, Kirill A . Shutemov, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Vlastimil Babka, Hugh Dickins,
Jérôme Glisse, Andrea Arcangeli, Aneesh Kumar K . V,
David Rientjes, Matthew Wilcox, Lance Roy, Ralph Campbell,
Jason Gunthorpe, Dave Airlie, Thomas Hellstrom, Souptick Joarder,
Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying, Aaron Lu,
Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm
On Fri 27-09-19 12:31:41, John Hubbard wrote:
> On 9/27/19 5:33 AM, Michal Hocko wrote:
> > On Thu 26-09-19 20:26:46, John Hubbard wrote:
> > > On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:
> > > > BTW, have you looked at other levels of page table hierarchy. Do we have
> > > > the same issue for PMD/PUD/... pages?
> > > >
> > >
> > > Along the lines of "what other memory barriers might be missing for
> > > get_user_pages_fast(), I'm also concerned that the synchronization between
> > > get_user_pages_fast() and freeing the page tables might be technically broken,
> > > due to missing memory barriers on the get_user_pages_fast() side. Details:
> > >
> > > gup_fast() disables interrupts, but I think it also needs some sort of
> > > memory barrier(s), in order to prevent reads of the page table (gup_pgd_range,
> > > etc) from speculatively happening before the interrupts are disabled.
> >
> > Could you be more specific about the race scenario please? I thought
> > that the unmap path will be serialized by the pte lock.
> >
>
> I don't see a pte lock anywhere here.
>
> This case is really pretty far out there, but without run-time memory barriers
> I don't think we can completely rule it out:
>
> CPU 0 CPU 1
> -------- ---------------
> get_user_pages_fast()
>
> do_unmap()
> unmap_region()
> free_pgtables()
> /*
> * speculative reads, re-ordered
> * by the CPU at run time, not
> * compile time. The function calls
> * are not really in this order, but
> * the corresponding reads could be.
> */
> gup_pgd_range()
> gup_p4d_range()
> gup_pud_range()
> gup_pmd_range()
> pmd = READ_ONCE(*pmdp)
> /* This is stale */
>
> tlb_finish_mmu()
> tlb_flush_mmu()
> tlb_flush_mmu_tlbonly()
> tlb_flush()
> flush_tlb_mm
> flush_tlb_mm_range
> flush_tlb_others
> native_flush_tlb_others
> smp_call_function_many: IPIs
> ...blocks until CPU1 reenables
> interrupts
>
> local_irq_disable()
> ...continue the page walk based
> on stale/freed data...
Yes, but then we have:
head = try_get_compound_head(page, 1);
which has an atomic operation providing barrier in it and then we have:
if (unlikely(pte_val(pte) != pte_val(*ptep))) {
put_page(head);
goto pte_unmap;
}
So we reload PTE again and check the value didn't change. Which should
prevent the race you explain above. Or do I miss anything?
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-09-30 9:20 ` Jan Kara
@ 2019-09-30 17:57 ` John Hubbard
2019-10-01 7:10 ` Jan Kara
0 siblings, 1 reply; 44+ messages in thread
From: John Hubbard @ 2019-09-30 17:57 UTC (permalink / raw)
To: Jan Kara
Cc: Michal Hocko, Kirill A. Shutemov, Yu Zhao, Peter Zijlstra,
Andrew Morton, Kirill A . Shutemov, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Vlastimil Babka, Hugh Dickins,
Jérôme Glisse, Andrea Arcangeli, Aneesh Kumar K . V,
David Rientjes, Matthew Wilcox, Lance Roy, Ralph Campbell,
Jason Gunthorpe, Dave Airlie, Thomas Hellstrom, Souptick Joarder,
Mel Gorman, Mike Kravetz, Huang Ying, Aaron Lu, Omar Sandoval,
Thomas Gleixner, Vineeth Remanan Pillai, Daniel Jordan,
Mike Rapoport, Joel Fernandes, Mark Rutland, Alexander Duyck,
Pavel Tatashin, David Hildenbrand, Juergen Gross, Anthony Yznaga,
Johannes Weiner, Darrick J . Wong, linux-kernel, linux-mm
On 9/30/19 2:20 AM, Jan Kara wrote:
> On Fri 27-09-19 12:31:41, John Hubbard wrote:
>> On 9/27/19 5:33 AM, Michal Hocko wrote:
>>> On Thu 26-09-19 20:26:46, John Hubbard wrote:
>>>> On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:
...
>> CPU 0 CPU 1
>> -------- ---------------
>> get_user_pages_fast()
>>
>> do_unmap()
>> unmap_region()
>> free_pgtables()
>> /*
>> * speculative reads, re-ordered
>> * by the CPU at run time, not
>> * compile time. The function calls
>> * are not really in this order, but
>> * the corresponding reads could be.
>> */
>> gup_pgd_range()
>> gup_p4d_range()
>> gup_pud_range()
>> gup_pmd_range()
>> pmd = READ_ONCE(*pmdp)
>> /* This is stale */
>>
>> tlb_finish_mmu()
>> tlb_flush_mmu()
>> tlb_flush_mmu_tlbonly()
>> tlb_flush()
>> flush_tlb_mm
>> flush_tlb_mm_range
>> flush_tlb_others
>> native_flush_tlb_others
>> smp_call_function_many: IPIs
>> ...blocks until CPU1 reenables
>> interrupts
>>
>> local_irq_disable()
>> ...continue the page walk based
>> on stale/freed data...
>
> Yes, but then we have:
>
> head = try_get_compound_head(page, 1);
>
> which has an atomic operation providing barrier in it and then we have:
>
> if (unlikely(pte_val(pte) != pte_val(*ptep))) {
> put_page(head);
> goto pte_unmap;
> }
>
> So we reload PTE again and check the value didn't change. Which should
> prevent the race you explain above. Or do I miss anything?
Well, a couple of questions:
1. Is there *really* a memory barrier in try_get_compound_head()? Because
I only see a READ_ONCE compile barrier, which would mean that run time
reordering is still possible.
2. Your code above is after the "pmd = READ_ONCE(*pmdp)" line, so by then,
it's already found the pte based on reading a stale pmd. So checking the
pte seems like it's checking the wrong thing--it's too late, for this case,
right?
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-09-30 17:57 ` John Hubbard
@ 2019-10-01 7:10 ` Jan Kara
2019-10-01 8:36 ` Peter Zijlstra
2019-10-01 18:43 ` John Hubbard
0 siblings, 2 replies; 44+ messages in thread
From: Jan Kara @ 2019-10-01 7:10 UTC (permalink / raw)
To: John Hubbard
Cc: Jan Kara, Michal Hocko, Kirill A. Shutemov, Yu Zhao,
Peter Zijlstra, Andrew Morton, Kirill A . Shutemov, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Vlastimil Babka, Hugh Dickins,
Jérôme Glisse, Andrea Arcangeli, Aneesh Kumar K . V,
David Rientjes, Matthew Wilcox, Lance Roy, Ralph Campbell,
Jason Gunthorpe, Dave Airlie, Thomas Hellstrom, Souptick Joarder,
Mel Gorman, Mike Kravetz, Huang Ying, Aaron Lu, Omar Sandoval,
Thomas Gleixner, Vineeth Remanan Pillai, Daniel Jordan,
Mike Rapoport, Joel Fernandes, Mark Rutland, Alexander Duyck,
Pavel Tatashin, David Hildenbrand, Juergen Gross, Anthony Yznaga,
Johannes Weiner, Darrick J . Wong, linux-kernel, linux-mm
On Mon 30-09-19 10:57:08, John Hubbard wrote:
> On 9/30/19 2:20 AM, Jan Kara wrote:
> > On Fri 27-09-19 12:31:41, John Hubbard wrote:
> > > On 9/27/19 5:33 AM, Michal Hocko wrote:
> > > > On Thu 26-09-19 20:26:46, John Hubbard wrote:
> > > > > On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:
> ...
> > > CPU 0 CPU 1
> > > -------- ---------------
> > > get_user_pages_fast()
> > >
> > > do_unmap()
> > > unmap_region()
> > > free_pgtables()
> > > /*
> > > * speculative reads, re-ordered
> > > * by the CPU at run time, not
> > > * compile time. The function calls
> > > * are not really in this order, but
> > > * the corresponding reads could be.
> > > */
> > > gup_pgd_range()
> > > gup_p4d_range()
> > > gup_pud_range()
> > > gup_pmd_range()
> > > pmd = READ_ONCE(*pmdp)
> > > /* This is stale */
> > >
> > > tlb_finish_mmu()
> > > tlb_flush_mmu()
> > > tlb_flush_mmu_tlbonly()
> > > tlb_flush()
> > > flush_tlb_mm
> > > flush_tlb_mm_range
> > > flush_tlb_others
> > > native_flush_tlb_others
> > > smp_call_function_many: IPIs
> > > ...blocks until CPU1 reenables
> > > interrupts
> > >
> > > local_irq_disable()
> > > ...continue the page walk based
> > > on stale/freed data...
> >
> > Yes, but then we have:
> >
> > head = try_get_compound_head(page, 1);
> >
> > which has an atomic operation providing barrier in it and then we have:
> >
> > if (unlikely(pte_val(pte) != pte_val(*ptep))) {
> > put_page(head);
> > goto pte_unmap;
> > }
> >
> > So we reload PTE again and check the value didn't change. Which should
> > prevent the race you explain above. Or do I miss anything?
>
>
> Well, a couple of questions:
>
> 1. Is there *really* a memory barrier in try_get_compound_head()? Because
> I only see a READ_ONCE compile barrier, which would mean that run time
> reordering is still possible.
try_get_compound_head() has page_cache_add_speculative() which is
atomic_add_unless() which is guaranteed to provide ordering.
> 2. Your code above is after the "pmd = READ_ONCE(*pmdp)" line, so by then,
> it's already found the pte based on reading a stale pmd. So checking the
> pte seems like it's checking the wrong thing--it's too late, for this case,
> right?
Well, if PMD is getting freed, all PTEs in it should be cleared by that
time, shouldn't they? So although we read from stale PMD, either we already
see cleared PTE or the check pte_val(pte) != pte_val(*ptep) will fail and
so we never actually succeed in getting stale PTE entry (at least unless
the page table page that used to be PMD can get freed and reused
- which is not the case in the example you've shown above).
So I still don't see a problem. That being said I don't feel being expert
in this particular area. I just always thought GUP prevents races like this
by the scheme I describe so I'd like to see what I'm missing :).
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-10-01 7:10 ` Jan Kara
@ 2019-10-01 8:36 ` Peter Zijlstra
2019-10-01 8:40 ` Jan Kara
2019-10-01 18:43 ` John Hubbard
1 sibling, 1 reply; 44+ messages in thread
From: Peter Zijlstra @ 2019-10-01 8:36 UTC (permalink / raw)
To: Jan Kara
Cc: John Hubbard, Michal Hocko, Kirill A. Shutemov, Yu Zhao,
Andrew Morton, Kirill A . Shutemov, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Vlastimil Babka, Hugh Dickins,
Jérôme Glisse, Andrea Arcangeli, Aneesh Kumar K . V,
David Rientjes, Matthew Wilcox, Lance Roy, Ralph Campbell,
Jason Gunthorpe, Dave Airlie, Thomas Hellstrom, Souptick Joarder,
Mel Gorman, Mike Kravetz, Huang Ying, Aaron Lu, Omar Sandoval,
Thomas Gleixner, Vineeth Remanan Pillai, Daniel Jordan,
Mike Rapoport, Joel Fernandes, Mark Rutland, Alexander Duyck,
Pavel Tatashin, David Hildenbrand, Juergen Gross, Anthony Yznaga,
Johannes Weiner, Darrick J . Wong, linux-kernel, linux-mm
On Tue, Oct 01, 2019 at 09:10:08AM +0200, Jan Kara wrote:
> On Mon 30-09-19 10:57:08, John Hubbard wrote:
> > Well, a couple of questions:
> >
> > 1. Is there *really* a memory barrier in try_get_compound_head()? Because
> > I only see a READ_ONCE compile barrier, which would mean that run time
> > reordering is still possible.
>
> try_get_compound_head() has page_cache_add_speculative() which is
> atomic_add_unless() which is guaranteed to provide ordering.
atomic_add_unless() only provides ordering on SUCCESS, now looking at
the code in question that seems sufficient, so all should be well. Just
wanted to let you know that if atomic_add_unless() FAILs there is no
implied ordering what so ever.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-10-01 8:36 ` Peter Zijlstra
@ 2019-10-01 8:40 ` Jan Kara
0 siblings, 0 replies; 44+ messages in thread
From: Jan Kara @ 2019-10-01 8:40 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Jan Kara, John Hubbard, Michal Hocko, Kirill A. Shutemov,
Yu Zhao, Andrew Morton, Kirill A . Shutemov, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Vlastimil Babka, Hugh Dickins,
Jérôme Glisse, Andrea Arcangeli, Aneesh Kumar K . V,
David Rientjes, Matthew Wilcox, Lance Roy, Ralph Campbell,
Jason Gunthorpe, Dave Airlie, Thomas Hellstrom, Souptick Joarder,
Mel Gorman, Mike Kravetz, Huang Ying, Aaron Lu, Omar Sandoval,
Thomas Gleixner, Vineeth Remanan Pillai, Daniel Jordan,
Mike Rapoport, Joel Fernandes, Mark Rutland, Alexander Duyck,
Pavel Tatashin, David Hildenbrand, Juergen Gross, Anthony Yznaga,
Johannes Weiner, Darrick J . Wong, linux-kernel, linux-mm
On Tue 01-10-19 10:36:04, Peter Zijlstra wrote:
> On Tue, Oct 01, 2019 at 09:10:08AM +0200, Jan Kara wrote:
> > On Mon 30-09-19 10:57:08, John Hubbard wrote:
>
> > > Well, a couple of questions:
> > >
> > > 1. Is there *really* a memory barrier in try_get_compound_head()? Because
> > > I only see a READ_ONCE compile barrier, which would mean that run time
> > > reordering is still possible.
> >
> > try_get_compound_head() has page_cache_add_speculative() which is
> > atomic_add_unless() which is guaranteed to provide ordering.
>
> atomic_add_unless() only provides ordering on SUCCESS, now looking at
> the code in question that seems sufficient, so all should be well. Just
> wanted to let you know that if atomic_add_unless() FAILs there is no
> implied ordering what so ever.
Yeah, right, thanks for correction!
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-10-01 7:10 ` Jan Kara
2019-10-01 8:36 ` Peter Zijlstra
@ 2019-10-01 18:43 ` John Hubbard
2019-10-02 9:24 ` Jan Kara
1 sibling, 1 reply; 44+ messages in thread
From: John Hubbard @ 2019-10-01 18:43 UTC (permalink / raw)
To: Jan Kara
Cc: Michal Hocko, Kirill A. Shutemov, Yu Zhao, Peter Zijlstra,
Andrew Morton, Kirill A . Shutemov, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Vlastimil Babka, Hugh Dickins,
Jérôme Glisse, Andrea Arcangeli, Aneesh Kumar K . V,
David Rientjes, Matthew Wilcox, Lance Roy, Ralph Campbell,
Jason Gunthorpe, Dave Airlie, Thomas Hellstrom, Souptick Joarder,
Mel Gorman, Mike Kravetz, Huang Ying, Aaron Lu, Omar Sandoval,
Thomas Gleixner, Vineeth Remanan Pillai, Daniel Jordan,
Mike Rapoport, Joel Fernandes, Mark Rutland, Alexander Duyck,
Pavel Tatashin, David Hildenbrand, Juergen Gross, Anthony Yznaga,
Johannes Weiner, Darrick J . Wong, linux-kernel, linux-mm
On 10/1/19 12:10 AM, Jan Kara wrote:
> On Mon 30-09-19 10:57:08, John Hubbard wrote:
>> On 9/30/19 2:20 AM, Jan Kara wrote:
>>> On Fri 27-09-19 12:31:41, John Hubbard wrote:
>>>> On 9/27/19 5:33 AM, Michal Hocko wrote:
>>>>> On Thu 26-09-19 20:26:46, John Hubbard wrote:
>>>>>> On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:
>> ...
>> 2. Your code above is after the "pmd = READ_ONCE(*pmdp)" line, so by then,
>> it's already found the pte based on reading a stale pmd. So checking the
>> pte seems like it's checking the wrong thing--it's too late, for this case,
>> right?
>
> Well, if PMD is getting freed, all PTEs in it should be cleared by that
> time, shouldn't they? So although we read from stale PMD, either we already
> see cleared PTE or the check pte_val(pte) != pte_val(*ptep) will fail and
> so we never actually succeed in getting stale PTE entry (at least unless
> the page table page that used to be PMD can get freed and reused
> - which is not the case in the example you've shown above).
>
Right, that's not what the example shows, but there is nothing here to prevent
the page table pages from being freed and re-used.
> So I still don't see a problem. That being said I don't feel being expert
> in this particular area. I just always thought GUP prevents races like this
> by the scheme I describe so I'd like to see what I'm missing :).
>
I'm very much still "in training" here, so I hope I'm not wasting everyone's
time. But I feel confident in stating at least this much:
There are two distinct lockless synchronization mechanisms here, each protecting
against a different issue, and it's important not to conflate them and think that
one protects against the other. I still see a hole in (2) below. The mechanisms
are:
1) Protection against a page (not the page table itself) getting freed
while get_user_pages*() is trying to take a reference to it. This is avoided
by the try-get and the re-checking of pte values that you mention above.
It's an elegant little thing, too. :)
2) Protection against page tables getting freed while a get_user_pages_fast()
call is in progress. This relies on disabling interrupts in gup_fast(), while
firing interrupts in the freeing path (via tlb flushing, IPIs). And on memory
barriers (doh--missing!) to avoid leaking memory references outside of the
irq disabling.
This one has a problem, because Documentation/memory-barriers.txt points out:
INTERRUPT DISABLING FUNCTIONS
-----------------------------
Functions that disable interrupts (ACQUIRE equivalent) and enable interrupts
(RELEASE equivalent) will act as compiler barriers only. So if memory or I/O
barriers are required in such a situation, they must be provided from some
other means.
...and so I'm suggesting that we need something approximately like this:
diff --git a/mm/gup.c b/mm/gup.c
index 23a9f9c9d377..1678d50a2d8b 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2415,7 +2415,9 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
if (IS_ENABLED(CONFIG_HAVE_FAST_GUP) &&
gup_fast_permitted(start, end)) {
local_irq_disable();
+ smp_mb();
gup_pgd_range(addr, end, gup_flags, pages, &nr);
+ smp_mb();
local_irq_enable();
ret = nr;
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-10-01 18:43 ` John Hubbard
@ 2019-10-02 9:24 ` Jan Kara
2019-10-02 17:33 ` John Hubbard
0 siblings, 1 reply; 44+ messages in thread
From: Jan Kara @ 2019-10-02 9:24 UTC (permalink / raw)
To: John Hubbard
Cc: Jan Kara, Michal Hocko, Kirill A. Shutemov, Yu Zhao,
Peter Zijlstra, Andrew Morton, Kirill A . Shutemov, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Vlastimil Babka, Hugh Dickins,
Jérôme Glisse, Andrea Arcangeli, Aneesh Kumar K . V,
David Rientjes, Matthew Wilcox, Lance Roy, Ralph Campbell,
Jason Gunthorpe, Dave Airlie, Thomas Hellstrom, Souptick Joarder,
Mel Gorman, Mike Kravetz, Huang Ying, Aaron Lu, Omar Sandoval,
Thomas Gleixner, Vineeth Remanan Pillai, Daniel Jordan,
Mike Rapoport, Joel Fernandes, Mark Rutland, Alexander Duyck,
Pavel Tatashin, David Hildenbrand, Juergen Gross, Anthony Yznaga,
Johannes Weiner, Darrick J . Wong, linux-kernel, linux-mm,
Paul E. McKenney
On Tue 01-10-19 11:43:30, John Hubbard wrote:
> On 10/1/19 12:10 AM, Jan Kara wrote:
> > On Mon 30-09-19 10:57:08, John Hubbard wrote:
> >> On 9/30/19 2:20 AM, Jan Kara wrote:
> >>> On Fri 27-09-19 12:31:41, John Hubbard wrote:
> >>>> On 9/27/19 5:33 AM, Michal Hocko wrote:
> >>>>> On Thu 26-09-19 20:26:46, John Hubbard wrote:
> >>>>>> On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:
> >> ...
> >> 2. Your code above is after the "pmd = READ_ONCE(*pmdp)" line, so by then,
> >> it's already found the pte based on reading a stale pmd. So checking the
> >> pte seems like it's checking the wrong thing--it's too late, for this case,
> >> right?
> >
> > Well, if PMD is getting freed, all PTEs in it should be cleared by that
> > time, shouldn't they? So although we read from stale PMD, either we already
> > see cleared PTE or the check pte_val(pte) != pte_val(*ptep) will fail and
> > so we never actually succeed in getting stale PTE entry (at least unless
> > the page table page that used to be PMD can get freed and reused
> > - which is not the case in the example you've shown above).
>
> Right, that's not what the example shows, but there is nothing here to prevent
> the page table pages from being freed and re-used.
>
> > So I still don't see a problem. That being said I don't feel being expert
> > in this particular area. I just always thought GUP prevents races like this
> > by the scheme I describe so I'd like to see what I'm missing :).
>
> I'm very much still "in training" here, so I hope I'm not wasting everyone's
> time. But I feel confident in stating at least this much:
>
> There are two distinct lockless synchronization mechanisms here, each
> protecting against a different issue, and it's important not to conflate
> them and think that one protects against the other. I still see a hole in
> (2) below. The mechanisms are:
>
> 1) Protection against a page (not the page table itself) getting freed
> while get_user_pages*() is trying to take a reference to it. This is avoided
> by the try-get and the re-checking of pte values that you mention above.
> It's an elegant little thing, too. :)
>
> 2) Protection against page tables getting freed while a get_user_pages_fast()
> call is in progress. This relies on disabling interrupts in gup_fast(), while
> firing interrupts in the freeing path (via tlb flushing, IPIs). And on memory
> barriers (doh--missing!) to avoid leaking memory references outside of the
> irq disabling.
OK, so you are concerned that the page table walk of pgd->p4d->pud->pmd
will get prefetched by the CPU before interrupts are disabled, somewhere in
the middle of the walk IPI flushing TLBs on the cpu will happen allowing
munmap() on another CPU to proceed and free page tables so the rest of the
walk will happen on freed and possibly reused pages. Do I understand you
right?
Realistically, I don't think this can happen as I'd expect the CPU to throw
away the speculation state on interrupt. But that's just my expectation and
I agree that I don't see anything in Documentation/memory-barriers.txt that
would prevent what you are concerned about. Let's ask Paul :)
Paul, we are discussing here a possible races between
mm/gup.c:__get_user_pages_fast() and mm/mmap.c:unmap_region(). The first
has a code like:
local_irq_save(flags);
load pgd from current->mm
load p4d from pgd
load pud from p4d
load pmd from pud
...
local_irq_restore(flags);
while the second has code like:
unmap_region()
walk pgd
walk p4d
walk pud
walk pmd
clear ptes
flush tlb
free page tables
Now the serialization between these two relies on the fact that flushing
TLBs from unmap_region() requires IPI to be served on each CPU so in naive
understanding unmap_region() shouldn't be able to get to 'free unused page
tables' part until __get_user_pages_fast() enables interrupts again. Now as
John points out we don't see anything in Documentation/memory-barriers.txt
that would actually guarantee this. So do we really need something like
smp_rmb() after disabling interrupts in __get_user_pages_fast() or is the
race John is concerned about impossible? Thanks!
> This one has a problem, because Documentation/memory-barriers.txt points out:
>
> INTERRUPT DISABLING FUNCTIONS
> -----------------------------
>
> Functions that disable interrupts (ACQUIRE equivalent) and enable interrupts
> (RELEASE equivalent) will act as compiler barriers only. So if memory or I/O
> barriers are required in such a situation, they must be provided from some
> other means.
>
>
> ...and so I'm suggesting that we need something approximately like this:
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 23a9f9c9d377..1678d50a2d8b 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2415,7 +2415,9 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
> if (IS_ENABLED(CONFIG_HAVE_FAST_GUP) &&
> gup_fast_permitted(start, end)) {
> local_irq_disable();
> + smp_mb();
> gup_pgd_range(addr, end, gup_flags, pages, &nr);
> + smp_mb();
> local_irq_enable();
> ret = nr;
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
2019-10-02 9:24 ` Jan Kara
@ 2019-10-02 17:33 ` John Hubbard
0 siblings, 0 replies; 44+ messages in thread
From: John Hubbard @ 2019-10-02 17:33 UTC (permalink / raw)
To: Jan Kara
Cc: Michal Hocko, Kirill A. Shutemov, Yu Zhao, Peter Zijlstra,
Andrew Morton, Kirill A . Shutemov, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Vlastimil Babka, Hugh Dickins,
Jérôme Glisse, Andrea Arcangeli, Aneesh Kumar K . V,
David Rientjes, Matthew Wilcox, Lance Roy, Ralph Campbell,
Jason Gunthorpe, Dave Airlie, Thomas Hellstrom, Souptick Joarder,
Mel Gorman, Mike Kravetz, Huang Ying, Aaron Lu, Omar Sandoval,
Thomas Gleixner, Vineeth Remanan Pillai, Daniel Jordan,
Mike Rapoport, Joel Fernandes, Mark Rutland, Alexander Duyck,
Pavel Tatashin, David Hildenbrand, Juergen Gross, Anthony Yznaga,
Johannes Weiner, Darrick J . Wong, linux-kernel, linux-mm,
Paul E. McKenney
On 10/2/19 2:24 AM, Jan Kara wrote:
>
> OK, so you are concerned that the page table walk of pgd->p4d->pud->pmd
> will get prefetched by the CPU before interrupts are disabled, somewhere in
> the middle of the walk IPI flushing TLBs on the cpu will happen allowing
> munmap() on another CPU to proceed and free page tables so the rest of the
> walk will happen on freed and possibly reused pages. Do I understand you
> right?
>
Yes, that's it.
thanks,
--
John Hubbard
NVIDIA
> Realistically, I don't think this can happen as I'd expect the CPU to throw
> away the speculation state on interrupt. But that's just my expectation and
> I agree that I don't see anything in Documentation/memory-barriers.txt that
> would prevent what you are concerned about. Let's ask Paul :)
>
> Paul, we are discussing here a possible races between
> mm/gup.c:__get_user_pages_fast() and mm/mmap.c:unmap_region(). The first
> has a code like:
>
> local_irq_save(flags);
> load pgd from current->mm
> load p4d from pgd
> load pud from p4d
> load pmd from pud
> ...
> local_irq_restore(flags);
>
> while the second has code like:
>
> unmap_region()
> walk pgd
> walk p4d
> walk pud
> walk pmd
> clear ptes
> flush tlb
> free page tables
>
> Now the serialization between these two relies on the fact that flushing
> TLBs from unmap_region() requires IPI to be served on each CPU so in naive
> understanding unmap_region() shouldn't be able to get to 'free unused page
> tables' part until __get_user_pages_fast() enables interrupts again. Now as
> John points out we don't see anything in Documentation/memory-barriers.txt
> that would actually guarantee this. So do we really need something like
> smp_rmb() after disabling interrupts in __get_user_pages_fast() or is the
> race John is concerned about impossible? Thanks!
>
>> This one has a problem, because Documentation/memory-barriers.txt points out:
>>
>> INTERRUPT DISABLING FUNCTIONS
>> -----------------------------
>>
>> Functions that disable interrupts (ACQUIRE equivalent) and enable interrupts
>> (RELEASE equivalent) will act as compiler barriers only. So if memory or I/O
>> barriers are required in such a situation, they must be provided from some
>> other means.
>>
>>
>> ...and so I'm suggesting that we need something approximately like this:
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 23a9f9c9d377..1678d50a2d8b 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -2415,7 +2415,9 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
>> if (IS_ENABLED(CONFIG_HAVE_FAST_GUP) &&
>> gup_fast_permitted(start, end)) {
>> local_irq_disable();
>> + smp_mb();
>> gup_pgd_range(addr, end, gup_flags, pages, &nr);
>> + smp_mb();
>> local_irq_enable();
>> ret = nr;
>
> Honza
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 4/4] mm: remove unnecessary smp_wmb() in __SetPageUptodate()
2019-09-24 23:24 ` Yu Zhao
@ 2019-09-24 23:24 ` Yu Zhao
-1 siblings, 0 replies; 44+ messages in thread
From: Yu Zhao @ 2019-09-24 23:24 UTC (permalink / raw)
To: Andrew Morton, Michal Hocko, Kirill A . Shutemov
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Vlastimil Babka,
Hugh Dickins, Jérôme Glisse, Andrea Arcangeli,
Aneesh Kumar K . V, David Rientjes, Matthew Wilcox, Lance Roy,
Ralph Campbell, Jason Gunthorpe, Dave Airlie, Thomas Hellstrom,
Souptick Joarder, Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying,
Aaron Lu, Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm, Yu Zhao
smp_wmb()s added in the previous patch guarantee that the user data
appears before a page is exposed by set_pte_at(). So there is no
need for __SetPageUptodate() to have a built-in one.
There are total 13 __SetPageUptodate() for the non-hugetlb case. 12
of them reuse smp_wmb()s added in the previous patch.
The one in shmem_mfill_atomic_pte() doesn't need a explicit write
barrier because of the following shmem_add_to_page_cache().
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
include/linux/page-flags.h | 6 +++++-
kernel/events/uprobes.c | 2 +-
mm/huge_memory.c | 11 +++--------
mm/khugepaged.c | 2 +-
mm/memory.c | 13 ++++---------
mm/migrate.c | 7 +------
mm/swapfile.c | 2 +-
mm/userfaultfd.c | 7 +------
8 files changed, 17 insertions(+), 33 deletions(-)
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index f91cb8898ff0..2481f9ad5f5b 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -508,10 +508,14 @@ static inline int PageUptodate(struct page *page)
return ret;
}
+/*
+ * Only use this function when there is a following write barrier, e.g.,
+ * an explicit smp_wmb() and/or the page will be added to page or swap
+ * cache locked.
+ */
static __always_inline void __SetPageUptodate(struct page *page)
{
VM_BUG_ON_PAGE(PageTail(page), page);
- smp_wmb();
__set_bit(PG_uptodate, &page->flags);
}
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 7069785e2e52..6ceae92afcc0 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -194,7 +194,7 @@ static int __replace_page(struct vm_area_struct *vma, unsigned long addr,
flush_cache_page(vma, addr, pte_pfn(*pvmw.pte));
ptep_clear_flush_notify(vma, addr, pvmw.pte);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
set_pte_at_notify(mm, addr, pvmw.pte,
mk_pte(new_page, vma->vm_page_prot));
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 21d271a29d96..101e7bd61e8f 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -580,11 +580,6 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
}
clear_huge_page(page, vmf->address, HPAGE_PMD_NR);
- /*
- * The memory barrier inside __SetPageUptodate makes sure that
- * clear_huge_page writes become visible before the set_pmd_at()
- * write.
- */
__SetPageUptodate(page);
vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
@@ -616,7 +611,7 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
mem_cgroup_commit_charge(page, memcg, false, true);
lru_cache_add_active_or_unevictable(page, vma);
pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
set_pmd_at(vma->vm_mm, haddr, vmf->pmd, entry);
add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
@@ -1278,7 +1273,7 @@ static vm_fault_t do_huge_pmd_wp_page_fallback(struct vm_fault *vmf,
}
kfree(pages);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb(); /* make pte visible before pmd */
pmd_populate(vma->vm_mm, vmf->pmd, pgtable);
@@ -1427,7 +1422,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf, pmd_t orig_pmd)
page_add_new_anon_rmap(new_page, vma, haddr, true);
mem_cgroup_commit_charge(new_page, memcg, false, true);
lru_cache_add_active_or_unevictable(new_page, vma);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
set_pmd_at(vma->vm_mm, haddr, vmf->pmd, entry);
update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index f2901edce6de..668918842712 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1074,7 +1074,7 @@ static void collapse_huge_page(struct mm_struct *mm,
count_memcg_events(memcg, THP_COLLAPSE_ALLOC, 1);
lru_cache_add_active_or_unevictable(new_page, vma);
pgtable_trans_huge_deposit(mm, pmd, pgtable);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
set_pmd_at(mm, address, pmd, _pmd);
update_mmu_cache_pmd(vma, address, pmd);
diff --git a/mm/memory.c b/mm/memory.c
index 6dabbc3cd3b7..db001d919e60 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2367,7 +2367,7 @@ static vm_fault_t wp_page_copy(struct vm_fault *vmf)
* mmu page tables (such as kvm shadow page tables), we want the
* new page to be mapped directly into the secondary page table.
*/
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
set_pte_at_notify(mm, vmf->address, vmf->pte, entry);
update_mmu_cache(vma, vmf->address, vmf->pte);
@@ -2887,7 +2887,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
page_add_new_anon_rmap(page, vma, vmf->address, false);
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, vma);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
} else {
do_page_add_anon_rmap(page, vma, vmf->address, exclusive);
@@ -3006,11 +3006,6 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
false))
goto oom_free_page;
- /*
- * The memory barrier inside __SetPageUptodate makes sure that
- * preceeding stores to the page contents become visible before
- * the set_pte_at() write.
- */
__SetPageUptodate(page);
entry = mk_pte(page, vma->vm_page_prot);
@@ -3038,7 +3033,7 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
page_add_new_anon_rmap(page, vma, vmf->address, false);
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, vma);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
setpte:
set_pte_at(vma->vm_mm, vmf->address, vmf->pte, entry);
@@ -3303,7 +3298,7 @@ vm_fault_t alloc_set_pte(struct vm_fault *vmf, struct mem_cgroup *memcg,
page_add_new_anon_rmap(page, vma, vmf->address, false);
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, vma);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
} else {
inc_mm_counter_fast(vma->vm_mm, mm_counter_file(page));
diff --git a/mm/migrate.c b/mm/migrate.c
index 943d147ecc3e..dc0ab9fbe36e 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2729,11 +2729,6 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate,
if (mem_cgroup_try_charge(page, vma->vm_mm, GFP_KERNEL, &memcg, false))
goto abort;
- /*
- * The memory barrier inside __SetPageUptodate makes sure that
- * preceding stores to the page contents become visible before
- * the set_pte_at() write.
- */
__SetPageUptodate(page);
if (is_zone_device_page(page)) {
@@ -2783,7 +2778,7 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate,
lru_cache_add_active_or_unevictable(page, vma);
get_page(page);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
if (flush) {
flush_cache_page(vma, addr, pte_pfn(*ptep));
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 5c5547053ee0..dc9f1b1ba1a6 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1887,7 +1887,7 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
page_add_new_anon_rmap(page, vma, addr, false);
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, vma);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
}
set_pte_at(vma->vm_mm, addr, pte,
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 4f92913242a1..34083680869e 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -58,11 +58,6 @@ static int mcopy_atomic_pte(struct mm_struct *dst_mm,
*pagep = NULL;
}
- /*
- * The memory barrier inside __SetPageUptodate makes sure that
- * preceeding stores to the page contents become visible before
- * the set_pte_at() write.
- */
__SetPageUptodate(page);
ret = -ENOMEM;
@@ -92,7 +87,7 @@ static int mcopy_atomic_pte(struct mm_struct *dst_mm,
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, dst_vma);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
set_pte_at(dst_mm, dst_addr, dst_pte, _dst_pte);
--
2.23.0.351.gc4317032e6-goog
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 4/4] mm: remove unnecessary smp_wmb() in __SetPageUptodate()
@ 2019-09-24 23:24 ` Yu Zhao
0 siblings, 0 replies; 44+ messages in thread
From: Yu Zhao @ 2019-09-24 23:24 UTC (permalink / raw)
To: Andrew Morton, Michal Hocko, Kirill A . Shutemov
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Vlastimil Babka,
Hugh Dickins, Jérôme Glisse, Andrea Arcangeli,
Aneesh Kumar K . V, David Rientjes, Matthew Wilcox, Lance Roy,
Ralph Campbell, Jason Gunthorpe, Dave Airlie, Thomas Hellstrom,
Souptick Joarder, Mel Gorman, Jan Kara, Mike Kravetz, Huang Ying,
Aaron Lu, Omar Sandoval, Thomas Gleixner, Vineeth Remanan Pillai,
Daniel Jordan, Mike Rapoport, Joel Fernandes, Mark Rutland,
Alexander Duyck, Pavel Tatashin, David Hildenbrand,
Juergen Gross, Anthony Yznaga, Johannes Weiner, Darrick J . Wong,
linux-kernel, linux-mm, Yu Zhao
smp_wmb()s added in the previous patch guarantee that the user data
appears before a page is exposed by set_pte_at(). So there is no
need for __SetPageUptodate() to have a built-in one.
There are total 13 __SetPageUptodate() for the non-hugetlb case. 12
of them reuse smp_wmb()s added in the previous patch.
The one in shmem_mfill_atomic_pte() doesn't need a explicit write
barrier because of the following shmem_add_to_page_cache().
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
include/linux/page-flags.h | 6 +++++-
kernel/events/uprobes.c | 2 +-
mm/huge_memory.c | 11 +++--------
mm/khugepaged.c | 2 +-
mm/memory.c | 13 ++++---------
mm/migrate.c | 7 +------
mm/swapfile.c | 2 +-
mm/userfaultfd.c | 7 +------
8 files changed, 17 insertions(+), 33 deletions(-)
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index f91cb8898ff0..2481f9ad5f5b 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -508,10 +508,14 @@ static inline int PageUptodate(struct page *page)
return ret;
}
+/*
+ * Only use this function when there is a following write barrier, e.g.,
+ * an explicit smp_wmb() and/or the page will be added to page or swap
+ * cache locked.
+ */
static __always_inline void __SetPageUptodate(struct page *page)
{
VM_BUG_ON_PAGE(PageTail(page), page);
- smp_wmb();
__set_bit(PG_uptodate, &page->flags);
}
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 7069785e2e52..6ceae92afcc0 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -194,7 +194,7 @@ static int __replace_page(struct vm_area_struct *vma, unsigned long addr,
flush_cache_page(vma, addr, pte_pfn(*pvmw.pte));
ptep_clear_flush_notify(vma, addr, pvmw.pte);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
set_pte_at_notify(mm, addr, pvmw.pte,
mk_pte(new_page, vma->vm_page_prot));
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 21d271a29d96..101e7bd61e8f 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -580,11 +580,6 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
}
clear_huge_page(page, vmf->address, HPAGE_PMD_NR);
- /*
- * The memory barrier inside __SetPageUptodate makes sure that
- * clear_huge_page writes become visible before the set_pmd_at()
- * write.
- */
__SetPageUptodate(page);
vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
@@ -616,7 +611,7 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
mem_cgroup_commit_charge(page, memcg, false, true);
lru_cache_add_active_or_unevictable(page, vma);
pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
set_pmd_at(vma->vm_mm, haddr, vmf->pmd, entry);
add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
@@ -1278,7 +1273,7 @@ static vm_fault_t do_huge_pmd_wp_page_fallback(struct vm_fault *vmf,
}
kfree(pages);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb(); /* make pte visible before pmd */
pmd_populate(vma->vm_mm, vmf->pmd, pgtable);
@@ -1427,7 +1422,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf, pmd_t orig_pmd)
page_add_new_anon_rmap(new_page, vma, haddr, true);
mem_cgroup_commit_charge(new_page, memcg, false, true);
lru_cache_add_active_or_unevictable(new_page, vma);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
set_pmd_at(vma->vm_mm, haddr, vmf->pmd, entry);
update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index f2901edce6de..668918842712 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1074,7 +1074,7 @@ static void collapse_huge_page(struct mm_struct *mm,
count_memcg_events(memcg, THP_COLLAPSE_ALLOC, 1);
lru_cache_add_active_or_unevictable(new_page, vma);
pgtable_trans_huge_deposit(mm, pmd, pgtable);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
set_pmd_at(mm, address, pmd, _pmd);
update_mmu_cache_pmd(vma, address, pmd);
diff --git a/mm/memory.c b/mm/memory.c
index 6dabbc3cd3b7..db001d919e60 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2367,7 +2367,7 @@ static vm_fault_t wp_page_copy(struct vm_fault *vmf)
* mmu page tables (such as kvm shadow page tables), we want the
* new page to be mapped directly into the secondary page table.
*/
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
set_pte_at_notify(mm, vmf->address, vmf->pte, entry);
update_mmu_cache(vma, vmf->address, vmf->pte);
@@ -2887,7 +2887,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
page_add_new_anon_rmap(page, vma, vmf->address, false);
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, vma);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
} else {
do_page_add_anon_rmap(page, vma, vmf->address, exclusive);
@@ -3006,11 +3006,6 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
false))
goto oom_free_page;
- /*
- * The memory barrier inside __SetPageUptodate makes sure that
- * preceeding stores to the page contents become visible before
- * the set_pte_at() write.
- */
__SetPageUptodate(page);
entry = mk_pte(page, vma->vm_page_prot);
@@ -3038,7 +3033,7 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
page_add_new_anon_rmap(page, vma, vmf->address, false);
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, vma);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
setpte:
set_pte_at(vma->vm_mm, vmf->address, vmf->pte, entry);
@@ -3303,7 +3298,7 @@ vm_fault_t alloc_set_pte(struct vm_fault *vmf, struct mem_cgroup *memcg,
page_add_new_anon_rmap(page, vma, vmf->address, false);
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, vma);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
} else {
inc_mm_counter_fast(vma->vm_mm, mm_counter_file(page));
diff --git a/mm/migrate.c b/mm/migrate.c
index 943d147ecc3e..dc0ab9fbe36e 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2729,11 +2729,6 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate,
if (mem_cgroup_try_charge(page, vma->vm_mm, GFP_KERNEL, &memcg, false))
goto abort;
- /*
- * The memory barrier inside __SetPageUptodate makes sure that
- * preceding stores to the page contents become visible before
- * the set_pte_at() write.
- */
__SetPageUptodate(page);
if (is_zone_device_page(page)) {
@@ -2783,7 +2778,7 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate,
lru_cache_add_active_or_unevictable(page, vma);
get_page(page);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
if (flush) {
flush_cache_page(vma, addr, pte_pfn(*ptep));
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 5c5547053ee0..dc9f1b1ba1a6 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1887,7 +1887,7 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
page_add_new_anon_rmap(page, vma, addr, false);
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, vma);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
}
set_pte_at(vma->vm_mm, addr, pte,
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 4f92913242a1..34083680869e 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -58,11 +58,6 @@ static int mcopy_atomic_pte(struct mm_struct *dst_mm,
*pagep = NULL;
}
- /*
- * The memory barrier inside __SetPageUptodate makes sure that
- * preceeding stores to the page contents become visible before
- * the set_pte_at() write.
- */
__SetPageUptodate(page);
ret = -ENOMEM;
@@ -92,7 +87,7 @@ static int mcopy_atomic_pte(struct mm_struct *dst_mm,
mem_cgroup_commit_charge(page, memcg, false, false);
lru_cache_add_active_or_unevictable(page, dst_vma);
- /* commit non-atomic ops before exposing to fast gup */
+ /* commit non-atomic ops and user data */
smp_wmb();
set_pte_at(dst_mm, dst_addr, dst_pte, _dst_pte);
--
2.23.0.351.gc4317032e6-goog
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v3 4/4] mm: remove unnecessary smp_wmb() in __SetPageUptodate()
2019-09-24 23:24 ` Yu Zhao
(?)
@ 2019-09-24 23:50 ` Matthew Wilcox
2019-09-25 22:03 ` Yu Zhao
-1 siblings, 1 reply; 44+ messages in thread
From: Matthew Wilcox @ 2019-09-24 23:50 UTC (permalink / raw)
To: Yu Zhao
Cc: Andrew Morton, Michal Hocko, Kirill A . Shutemov, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Vlastimil Babka, Hugh Dickins,
Jérôme Glisse, Andrea Arcangeli, Aneesh Kumar K . V,
David Rientjes, Lance Roy, Ralph Campbell, Jason Gunthorpe,
Dave Airlie, Thomas Hellstrom, Souptick Joarder, Mel Gorman,
Jan Kara, Mike Kravetz, Huang Ying, Aaron Lu, Omar Sandoval,
Thomas Gleixner, Vineeth Remanan Pillai, Daniel Jordan,
Mike Rapoport, Joel Fernandes, Mark Rutland, Alexander Duyck,
Pavel Tatashin, David Hildenbrand, Juergen Gross, Anthony Yznaga,
Johannes Weiner, Darrick J . Wong, linux-kernel, linux-mm
On Tue, Sep 24, 2019 at 05:24:59PM -0600, Yu Zhao wrote:
> +/*
> + * Only use this function when there is a following write barrier, e.g.,
> + * an explicit smp_wmb() and/or the page will be added to page or swap
> + * cache locked.
> + */
> static __always_inline void __SetPageUptodate(struct page *page)
> {
> VM_BUG_ON_PAGE(PageTail(page), page);
> - smp_wmb();
> __set_bit(PG_uptodate, &page->flags);
> }
Isn't this now the same as __SETPAGEFLAG(uptodate, Uptodate, PF_NO_TAIL)?
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v3 4/4] mm: remove unnecessary smp_wmb() in __SetPageUptodate()
2019-09-24 23:50 ` Matthew Wilcox
@ 2019-09-25 22:03 ` Yu Zhao
0 siblings, 0 replies; 44+ messages in thread
From: Yu Zhao @ 2019-09-25 22:03 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Andrew Morton, Michal Hocko, Kirill A . Shutemov, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Vlastimil Babka, Hugh Dickins,
Jérôme Glisse, Andrea Arcangeli, Aneesh Kumar K . V,
David Rientjes, Lance Roy, Ralph Campbell, Jason Gunthorpe,
Dave Airlie, Thomas Hellstrom, Souptick Joarder, Mel Gorman,
Jan Kara, Mike Kravetz, Huang Ying, Aaron Lu, Omar Sandoval,
Thomas Gleixner, Vineeth Remanan Pillai, Daniel Jordan,
Mike Rapoport, Joel Fernandes, Mark Rutland, Alexander Duyck,
Pavel Tatashin, David Hildenbrand, Juergen Gross, Anthony Yznaga,
Johannes Weiner, Darrick J . Wong, linux-kernel, linux-mm
On Tue, Sep 24, 2019 at 04:50:36PM -0700, Matthew Wilcox wrote:
> On Tue, Sep 24, 2019 at 05:24:59PM -0600, Yu Zhao wrote:
> > +/*
> > + * Only use this function when there is a following write barrier, e.g.,
> > + * an explicit smp_wmb() and/or the page will be added to page or swap
> > + * cache locked.
> > + */
> > static __always_inline void __SetPageUptodate(struct page *page)
> > {
> > VM_BUG_ON_PAGE(PageTail(page), page);
> > - smp_wmb();
> > __set_bit(PG_uptodate, &page->flags);
> > }
>
> Isn't this now the same as __SETPAGEFLAG(uptodate, Uptodate, PF_NO_TAIL)?
Indeed. I'll use the macro in the next version.
^ permalink raw reply [flat|nested] 44+ messages in thread