mm: don't expose page to fast gup before it's ready

We don't want to expose page before it's properly setup. During page setup, we may call page_add_new_anon_rmap() which uses non- atomic bit op. If page is exposed before it's done, we could overwrite page flags that are set by get_user_pages_fast() or it's callers. Here is a non-fatal scenario (there might be other fatal problems that I didn't look into):

	CPU 1				CPU1

set_pte_at() get_user_pages_fast()
page_add_new_anon_rmap() gup_pte_range()
__SetPageSwapBacked() SetPageReferenced()

Fix the problem by delaying set_pte_at() until page is ready.

Signed-off-by Yu Zhao <[email protected]>

---
 mm/memory.c   | 2 +-
 mm/swapfile.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

mm/memory.c +1 -1

index ca5674cbaff2..b8be1a4adf93 100644
@@ -3010,7 +3010,6 @@ int 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);
 	vmf->orig_pte = pte;
 
 	/* ksm created a completely new copy */

@@ -3023,6 +3022,7 @@ int do_swap_page(struct vm_fault *vmf)
 		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) ||

mm/swapfile.c +2 -2

index 3074b02eaa09..bd49da2b5221 100644
@@ -1800,8 +1800,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);

@@ -1810,6 +1808,8 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
 		mem_cgroup_commit_charge(page, memcg, false, false);
 		lru_cache_add_active_or_unevictable(page, vma);
 	}
+	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

Signature · 7 lines

2.16.0.rc0.223.g4a4ac83678-goog

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 12 lines · Yu Zhao

We don't want to expose page before it's properly setup. During page setup, we may call page_add_new_anon_rmap() which uses non- atomic bit op. If page is exposed before it's done, we could overwrite page flags that are set by get_user_pages_fast() or it's callers. Here is a non-fatal scenario (there might be other fatal problems that I didn't look into):

	CPU 1				CPU1

set_pte_at() get_user_pages_fast()
page_add_new_anon_rmap() gup_pte_range()
__SetPageSwapBacked() SetPageReferenced()

Quoted 1 line

Fix the problem by delaying set_pte_at() until page is ready.

Have you seen this race happening in real workloads or this is a code review based fix or a theoretical issue? I am primarily asking because the code is like that at least throughout git era and I do not remember any issue like this. If you can really trigger this tiny race window then we should mark the fix for stable.

Also what prevents reordering here? There do not seem to be any barriers to prevent __SetPageSwapBacked leak after set_pte_at with your patch.

Quoted 13 lines · Michal Hocko

Quoted 12 lines · Yu Zhao

We don't want to expose page before it's properly setup. During page setup, we may call page_add_new_anon_rmap() which uses non- atomic bit op. If page is exposed before it's done, we could overwrite page flags that are set by get_user_pages_fast() or it's callers. Here is a non-fatal scenario (there might be other fatal problems that I didn't look into):

	CPU 1				CPU1

set_pte_at() get_user_pages_fast()
page_add_new_anon_rmap() gup_pte_range()
__SetPageSwapBacked() SetPageReferenced()

Quoted 7 lines

Quoted 1 line

Fix the problem by delaying set_pte_at() until page is ready.

Have you seen this race happening in real workloads or this is a code review based fix or a theoretical issue? I am primarily asking because the code is like that at least throughout git era and I do not remember any issue like this. If you can really trigger this tiny race window then we should mark the fix for stable.

I didn't observe the race directly. But I did get few crashes when trying to access mem_cgroup of pages returned by get_user_pages_fast(). Those page were charged and they showed valid mem_cgroup in kdumps. So this led me to think the problem came from premature set_pte_at().

I think the fact that nobody complained about this problem is because the race only happens when using ksm+swap, and it might not cause any fatal problem even so. Nevertheless, it's nice to have set_pte_at() done consistently after rmap is added and page is charged.

Quoted 2 lines

Also what prevents reordering here? There do not seem to be any barriers to prevent __SetPageSwapBacked leak after set_pte_at with your patch.

I assumed mem_cgroup_commit_charge() acted as full barrier. Since you explicitly asked the question, I realized my assumption doesn't hold when memcg is disabled. So we do need something to prevent reordering in my patch. And it brings up the question whether we want to add more barrier to other places that call page_add_new_anon_rmap() and set_pte_at().

Quoted 2 lines

Also what prevents reordering here? There do not seem to be any barriers to prevent __SetPageSwapBacked leak after set_pte_at with your patch.

I assumed mem_cgroup_commit_charge() acted as full barrier. Since you explicitly asked the question, I realized my assumption doesn't hold when memcg is disabled. So we do need something to prevent reordering in my patch. And it brings up the question whether we want to add more barrier to other places that call page_add_new_anon_rmap() and set_pte_at().

Quoted 3 lines · On Tue, 9 Jan 2018 02:10:50 -0800 Yu Zhao <[email protected]> wrote:

Quoted 2 lines

Also what prevents reordering here? There do not seem to be any barriers to prevent __SetPageSwapBacked leak after set_pte_at with your patch.

Quoted 6 lines

I assumed mem_cgroup_commit_charge() acted as full barrier. Since you explicitly asked the question, I realized my assumption doesn't hold when memcg is disabled. So we do need something to prevent reordering in my patch. And it brings up the question whether we want to add more barrier to other places that call page_add_new_anon_rmap() and set_pte_at().

Is a new version of this patch planned?

Quoted 4 lines

I didn't observe the race directly. But I did get few crashes when trying to access mem_cgroup of pages returned by get_user_pages_fast(). Those page were charged and they showed valid mem_cgroup in kdumps. So this led me to think the problem came from premature set_pte_at().

We don't want to expose page to fast gup running on a remote CPU before all local non-atomic ops on page flags are visible first.

For 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 order 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()

Quoted 8 lines · Yu Zhao

We don't want to expose page to fast gup running on a remote CPU before all local non-atomic ops on page flags are visible first.

For 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 order before set_pte_at() to prevent the following race:

Quoted 4 lines

	CPU 1				CPU1

set_pte_at() get_user_pages_fast()
page_add_new_anon_rmap() gup_pte_range()
__SetPageSwapBacked() SetPageReferenced()

Is there a particular codepath that has what you listed for CPU? After quick look, I only saw that we page_add_new_anon_rmap() called before set_pte_at().

Quoted 14 lines · Kirill A. Shutemov

Quoted 12 lines · Yu Zhao

We don't want to expose page to fast gup running on a remote CPU before all local non-atomic ops on page flags are visible first.

For 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 order 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()

Quoted 3 lines

Is there a particular codepath that has what you listed for CPU? After quick look, I only saw that we page_add_new_anon_rmap() called before set_pte_at().

I think so. One in do_swap_page() and another in unuse_pte(). Both are on KSM paths. Am I referencing a stale copy of the source?

Quoted 19 lines · Yu Zhao

Quoted 17 lines · Kirill A. Shutemov

Quoted 8 lines · Yu Zhao

We don't want to expose page to fast gup running on a remote CPU before all local non-atomic ops on page flags are visible first.

For 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 order before set_pte_at() to prevent the following race:

Quoted 4 lines

	CPU 1				CPU1

set_pte_at() get_user_pages_fast()
page_add_new_anon_rmap() gup_pte_range()
__SetPageSwapBacked() SetPageReferenced()

Is there a particular codepath that has what you listed for CPU? After quick look, I only saw that we page_add_new_anon_rmap() called before set_pte_at().

Quoted 2 lines

I think so. One in do_swap_page() and another in unuse_pte(). Both are on KSM paths. Am I referencing a stale copy of the source?

I *think* it is a bug. Setting a pte before adding the page to rmap may lead to rmap (like try_to_unmap() or something) to miss the VMA.

Do I miss something?

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().

Quoted 23 lines · 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.

Quoted 4 lines

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?

Quoted 29 lines · Peter Zijlstra

Quoted 27 lines · 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().

Quoted 3 lines

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.

Quoted 2 lines · Yu Zhao

Quoted 1 line · Peter Zijlstra

On Tue, Sep 24, 2019 at 05:24:58PM -0600, Yu Zhao wrote:

Quoted 7 lines

Quoted 3 lines

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?

Quoted 3 lines · Kirill A. Shutemov

Quoted 2 lines · Yu Zhao

Quoted 1 line · Peter Zijlstra

On Tue, Sep 24, 2019 at 05:24:58PM -0600, Yu Zhao wrote:

...

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 17 lines · John Hubbard

Quoted 3 lines · Kirill A. Shutemov

Quoted 2 lines · Yu Zhao

Quoted 1 line · Peter Zijlstra

On Tue, Sep 24, 2019 at 05:24:58PM -0600, Yu Zhao wrote:

...

Quoted 11 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 2 lines

+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().

Quoted 10 lines

Quoted 4 lines

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:

Quoted 3 lines

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.

Quoted 18 lines · Yu Zhao

Quoted 17 lines · John Hubbard

Quoted 3 lines · Kirill A. Shutemov

Quoted 2 lines · Yu Zhao

Quoted 1 line · Peter Zijlstra

On Tue, Sep 24, 2019 at 05:24:58PM -0600, Yu Zhao wrote:

...

Quoted 11 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

+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...

Quoted 10 lines

Quoted 10 lines

Quoted 4 lines

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:

Quoted 8 lines

Quoted 3 lines

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. :)

Quoted 3 lines · Kirill A. Shutemov

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.

Quoted 10 lines · John Hubbard

Quoted 3 lines · Kirill A. Shutemov

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:

Quoted 3 lines

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.

Quoted 10 lines · John Hubbard

Quoted 3 lines · Kirill A. Shutemov

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:

Quoted 3 lines

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.

Quoted 11 lines · Michal Hocko

Quoted 10 lines · John Hubbard

Quoted 3 lines · Kirill A. Shutemov

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:

Quoted 7 lines

Quoted 3 lines

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...

Quoted 11 lines · Michal Hocko

Quoted 10 lines · John Hubbard

Quoted 3 lines · Kirill A. Shutemov

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:

Quoted 7 lines

Quoted 3 lines

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...

Quoted 3 lines · John Hubbard

Quoted 2 lines · Michal Hocko

Quoted 1 line · John Hubbard

On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:

Quoted 33 lines

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

Quoted 3 lines

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?

Quoted 4 lines · Jan Kara

Quoted 3 lines · John Hubbard

Quoted 2 lines · Michal Hocko

Quoted 1 line · John Hubbard

On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:

...

Quoted 43 lines

Quoted 33 lines

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

Quoted 3 lines

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:

Quoted 7 lines

                 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?

Quoted 5 lines · John Hubbard

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.

Quoted 8 lines · Jan Kara

Quoted 5 lines · John Hubbard

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.

Quoted 2 lines

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.

Quoted 10 lines · John Hubbard

Quoted 4 lines · Jan Kara

Quoted 3 lines · John Hubbard

Quoted 2 lines · Michal Hocko

Quoted 1 line · John Hubbard

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).

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 :).

Quoted 12 lines · Jan Kara

Quoted 10 lines · John Hubbard

Quoted 4 lines · Jan Kara

Quoted 3 lines · John Hubbard

Quoted 2 lines · Michal Hocko

Quoted 1 line · John Hubbard

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?

Quoted 6 lines

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.

Quoted 3 lines

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:

mm/gup.c +0 -0

index 23a9f9c9d377..1678d50a2d8b 100644

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!

Quoted 18 lines

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:

mm/gup.c +0 -0

index 23a9f9c9d377..1678d50a2d8b 100644

mm/gup.c int get_user_pages_fast(unsigned long start, int nr_pages, · line 2422

Quoted 8 lines

mm/gup.c +2 -0

@@ -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

include/linux/page-flags.h static inline int PageUptodate(struct page *page) · line 520

Quoted 11 lines · Yu Zhao

include/linux/page-flags.h +5 -1

+/*
+ * 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)?