Date:   Mon, 17 Jan 2022 10:11:08 +0800
From:   "zhangliang (AG)" <zhangliang5@...wei.com>
To:     David Hildenbrand <david@...hat.com>
CC:     Andrew Morton <akpm@...ux-foundation.org>,
        Linux-MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        <wangzhigang17@...wei.com>, Matthew Wilcox <willy@...radead.org>,
        "Linus Torvalds" <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] mm: reuse the unshared swapcache page in do_wp_page

Hi, thank you very much for your patient reply, and I completely agree with the viewpoint from you and Linus about COW and reuse_swap_page(). :)

On 2022/1/14 19:23, David Hildenbrand wrote:
> On 14.01.22 06:00, zhangliang (AG) wrote:
>>
>>
>> On 2022/1/14 0:48, David Hildenbrand wrote:
>>> On 13.01.22 17:37, Linus Torvalds wrote:
>>>> On Thu, Jan 13, 2022 at 6:39 AM Matthew Wilcox <willy@...radead.org> wrote:
>>>>>
>>>>> Let's bring Linus in on this, but I think this reintroduces all of the
>>>>> mapcount problems that we've been discussing recently.
>>>>>
>>>>> How about this as an alternative?
>>>>
>>>> No, at that point reuse_swap_page() is the better thing to do.
>>>>
>>>> Don't play games with page_count() (or even worse games with
>>>> swap_count). The page count is only stable if it's 1. Any other value
>>>> means that it can fluctuate due to concurrent lookups, some of which
>>>> can be done locklessly under RCU.
>>>
>>> I'm pretty sure the patch as is will reintroduce the CVE. So I think in
>>
>> Actually, I wonder how reuse_swap_page() in this patch can reintroduce CVE,
>> I think the invoking logic here is as same as that in do_swap_page(). 
>> So, could you give me some hint about this? Thanks :)
> 
> The CVE essentially is
> 
> a) Process A maps an anon page
> -> refcount = 1, mapcount = 1
> b) Process A forks process B
> -> Page mapped R/O
> -> refcount = 2, mapcount = 2
> c) Process B pins the page R/O (e.g., vmsplice)
> -> refcount > 2, mapcount = 2
> d) Process B unmaps the page
> -> refcount > 1, mapcount = 1
> e) Process A writes to the page
> 
> If e) ends up reusing the page for process A (e.g., because mapcount == 1), 
> process B can observe the memory modifications via the
> page pinning
> 
> As I previously said, our COW logic is completely inconsistent
> and should be streamlined. Regarding the CVE, what would have 
> to happen is that we put the page into the swapcache and unmap
> from both processes after c). Then, do_swap_page() would reuse
> the page during e) similarly.
> 
> We do have a check in place that protects against pinned pages
> not being able to enter the swapcache, but IIRC it can race with
> GUP, so it might race with c) eventually, meaning that with swap
> there might still be a small chance for the CVE in current upstream IIRC.
> 
> I think with your patch we could trick do_wp_page() do go down the 
> reuse_swap_page() path and reuse the page, even though there is a pending
> pin on the page from process B.
> 
> 
> a) Process A maps an anon page
> -> refcount = 1, mapcount = 1
> b) Page is added to the swapcache and unmapped
> -> refcount = 1, mapcount = 0
> c) Process A reads page
> -> do_swap_page() will map the page R/O
> -> the page might remain in the swapcache
> -> refcount = 2, mapcount = 1
> d) Process A forks process B
> -> Page mapped R/O
> -> refcount = 3, mapcount = 2
> e) Process B pins the page R/O (e.g., vmsplice)
> -> refcount > 3, mapcount = 2
> d) Process B unmaps the page
> -> refcount > 2, mapcount = 1
> e) Process A writes to the page
> 
> In e), we would go into reuse_swap_page() and could succeed, being left with
> refcount > 1 and mapcount = 1.
> 
> But maybe I am missing something :)
> 
> 
> 
> There are only two real ways I know to handle the scenario above:
> (1) Rely on page_count() == 1 in COW logic to decide whether we can
>     reuse a page. This comes with false negatives, some being harmful,
>     some just being a performance problem.
> (2) Rely on mapcount(), swapcount() ... when deciding to reuse a page
>     and when it's safe to take a GUP pin.
> 
> (2) was discussed in 
>   https://lkml.kernel.org/r/20211217113049.23850-1-david@redhat.com
> but was essentially NAKed by Linus in the current form (well, it didn't
> consider the swapcount so it was just buggy, fortunately Linus pointed that
> out).
> 
> To handle problematic false negatives with (1) I'm looking into marking
> anon pages as exclusively owned by a single process, such that we can simply
> reuse them in the COW path -- PageAnonExclusive() also discussed inside that
> posting. That posting also contains a test case which tests all different
> variants of the "child can observe MAP_PRIVAT  modifications of the parent"
> we know of (e.g., THP, hugetlb), which you might use to verify if the issue is
> reintroduced by your patch or not.
> 
> 
> Yesterday, I played with removing reuse_swap_page() completely
> and streamlining our COW logic like so:
> 
> (based on https://www.spinics.net/lists/linux-mm/msg279460.html)
> 
> 
> commit f1fa31a40b13ade5cd93ceb40daadf1196526145
> Author: David Hildenbrand <david@...hat.com>
> Date:   Fri Jan 14 09:29:52 2022 +0100
> 
>     mm: optimize do_wp_page() for exclusive pages in the swapcache
>     
>     Let's optimize for a page with a single user that has been added to the
>     swapcache. Try removing the swapcache reference if there is hope that
>     we're the exclusive user, but keep the page_count(page) == 1 check in
>     place.
>     
>     Avoid using reuse_swap_page(), we'll streamline all reuse_swap_page()
>     users next.
>     
>     While at it, remove the superfluous page_mapcount() check: it's
>     implicitly covered by the page_count() for ordinary anon pages.
>     
>     Signed-off-by: David Hildenbrand <david@...hat.com>
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index e8e2144cbfa6..bd2af7a36791 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3290,12 +3290,21 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
>         if (PageAnon(vmf->page)) {
>                 struct page *page = vmf->page;
>  
> -               /* PageKsm() doesn't necessarily raise the page refcount */
> -               if (PageKsm(page) || page_count(page) != 1)
> +               /*
> +                * PageKsm() doesn't necessarily raise the page refcount.
> +                *
> +                * These checks are racy as long as we haven't locked the page;
> +                * they are a pure optimization to avoid trying to lock the page
> +                * and trying to free the swap cache when there is little hope
> +                * it will actually result in a refcount of 1.
> +                */
> +               if (PageKsm(page) || page_count(page) <= 1 + PageSwapCache(page))
>                         goto copy;
>                 if (!trylock_page(page))
>                         goto copy;
> -               if (PageKsm(page) || page_mapcount(page) != 1 || page_count(page) != 1) {
> +               if (PageSwapCache(page))
> +                       try_to_free_swap(page);
> +               if (PageKsm(page) || page_count(page) != 1) {
>                         unlock_page(page);
>                         goto copy;
>                 }
> 
> 
> 
> Followed by
> 
> 
> 
> commit 754aa8f04f7b6d1ffc2b9844be63bbf7b527929f (HEAD)
> Author: David Hildenbrand <david@...hat.com>
> Date:   Fri Jan 14 09:53:20 2022 +0100
> 
>     mm: streamline COW logic in do_swap_page()
>     
>     Let's apply the same COW logic as in do_wp_page(), conditionally trying to
>     remove the page from the swapcache after freeing the swap entry, however,
>     before actually mapping our page. We can change the order now that
>     reuse_swap_page() is no longer used.
>     
>     Signed-off-by: David Hildenbrand <david@...hat.com>
> 
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index bd2af7a36791..a1bd2b5c818a 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3489,6 +3489,24 @@ static vm_fault_t remove_device_exclusive_entry(struct vm_fault *vmf)
>         return 0;
>  }
>  
> +static inline bool should_try_to_free_swap(struct page *page,
> +                                          struct vm_area_struct *vma,
> +                                          unsigned int fault_flags)
> +{
> +       if (!PageSwapCache(page))
> +               return false;
> +       if (mem_cgroup_swap_full(page) || (vma->vm_flags & VM_LOCKED) ||
> +           PageMlocked(page))
> +               return true;
> +       /*
> +        * We must only reuse the page if the page_count() is 1. Try to
> +        * free the swapcache to get rid of the swapcache reference in case
> +        * it's likely that we will succeed.
> +        */
> +       return (fault_flags & FAULT_FLAG_WRITE) && !PageKsm(page) &&
> +               page_count(page) == 2;
> +}
> +
>  /*
>   * We enter with non-exclusive mmap_lock (to exclude vma changes,
>   * but allow concurrent faults), and pte mapped but not yet locked.
> @@ -3612,7 +3630,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>         }
>  
>         /*
> -        * Make sure try_to_free_swap or reuse_swap_page or swapoff did not
> +        * Make sure try_to_free_swap or swapoff did not
>          * release the swapcache from under us.  The page pin, and pte_same
>          * test below, are not enough to exclude that.  Even if it is still
>          * swapcache, we need to check that the page's swap has not changed.
> @@ -3644,19 +3662,22 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>         }
>  
>         /*
> -        * The page isn't present yet, go ahead with the fault.
> -        *
> -        * Be careful about the sequence of operations here.
> -        * To get its accounting right, reuse_swap_page() must be called
> -        * while the page is counted on swap but not yet in mapcount i.e.
> -        * before page_add_anon_rmap() and swap_free(); try_to_free_swap()
> -        * must be called after the swap_free(), or it will never succeed.
> +        * Remove the swap entry and conditionally try to free up the swapcache.
> +        * We're already holding a reference on the page but haven't mapped it
> +        * yet. We won't be waiting on writeback, so if there is concurrent
> +        * writeback we won't map the page writable just now.
>          */
> +       swap_free(entry);
> +       if (should_try_to_free_swap(page, vma, vmf->flags))
> +               try_to_free_swap(page);
>  
>         inc_mm_counter_fast(vma->vm_mm, MM_ANONPAGES);
>         dec_mm_counter_fast(vma->vm_mm, MM_SWAPENTS);
>         pte = mk_pte(page, vma->vm_page_prot);
> -       if ((vmf->flags & FAULT_FLAG_WRITE) && reuse_swap_page(page)) {
> +
> +       /* Same logic as in do_wp_page(). */
> +       if ((vmf->flags & FAULT_FLAG_WRITE) &&
> +           !PageKsm(page) && page_count(page) == 1) {
>                 pte = maybe_mkwrite(pte_mkdirty(pte), vma);
>                 vmf->flags &= ~FAULT_FLAG_WRITE;
>                 ret |= VM_FAULT_WRITE;
> @@ -3681,10 +3702,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>                 do_page_add_anon_rmap(page, vma, vmf->address, exclusive);
>         }
>  
> -       swap_free(entry);
> -       if (mem_cgroup_swap_full(page) ||
> -           (vma->vm_flags & VM_LOCKED) || PageMlocked(page))
> -               try_to_free_swap(page);
>         unlock_page(page);
>         if (page != swapcache && swapcache) {
>                 /*
> 
> 
> 
> 
> And followed by more patches that rip out reuse_swap_page() completely.
> 
> 
> But I still have to understand what's safe to do and not do in do_swap_page(),
> and which effect it would have. Especially PTE-mapped THP are a head-scratcher
> for me, and the interaction with THP in the swapcache before/after actual swap.
> 
> So consider above a WIP prototype that's mostly untested.
> 

-- 
Best Regards,
Liang Zhang

Powered by blists - more mailing lists