Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 22 Sep 2020 16:11:16 -0300
From:   Jason Gunthorpe <jgg@ziepe.ca>
To:     Peter Xu <peterx@redhat.com>
Cc:     John Hubbard <jhubbard@nvidia.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Jan Kara <jack@suse.cz>, Michal Hocko <mhocko@suse.com>,
        Kirill Tkhai <ktkhai@virtuozzo.com>,
        Kirill Shutemov <kirill@shutemov.name>,
        Hugh Dickins <hughd@google.com>,
        Christoph Hellwig <hch@lst.de>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Oleg Nesterov <oleg@redhat.com>,
        Leon Romanovsky <leonro@nvidia.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Jann Horn <jannh@google.com>
Subject: Re: [PATCH 1/5] mm: Introduce mm_struct.has_pinned
Message-ID: <20200922191116.GK8409@ziepe.ca>
References: <20200921211744.24758-1-peterx@redhat.com>
 <20200921211744.24758-2-peterx@redhat.com>
 <224908c1-5d0f-8e01-baa9-94ec2374971f@nvidia.com>
 <20200922151736.GD19098@xz-x1>
 <20200922161046.GB731578@ziepe.ca>
 <20200922175415.GI19098@xz-x1>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200922175415.GI19098@xz-x1>
Precedence: bulk

On Tue, Sep 22, 2020 at 01:54:15PM -0400, Peter Xu wrote:
> diff --git a/mm/memory.c b/mm/memory.c
> index 8f3521be80ca..6591f3f33299 100644
> +++ b/mm/memory.c
> @@ -888,8 +888,8 @@ copy_one_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>                  * Because we'll need to release the locks before doing cow,
>                  * pass this work to upper layer.
>                  */
> -               if (READ_ONCE(src_mm->has_pinned) && wp &&
> -                   page_maybe_dma_pinned(page)) {
> +               if (wp && page_maybe_dma_pinned(page) &&
> +                   READ_ONCE(src_mm->has_pinned)) {
>                         /* We've got the page already; we're safe */
>                         data->cow_old_page = page;
>                         data->cow_oldpte = *src_pte;
> 
> I can also add some more comment to emphasize this.

It is not just that, but the ptep_set_wrprotect() has to be done
earlier.

Otherwise it races like:

   pin_user_pages_fast()                   fork()
    atomic_set(has_pinned, 1);
    [..]
                                           atomic_read(page->_refcount) //false
                                           // skipped atomic_read(has_pinned)
    atomic_add(page->_refcount)
    ordered check write protect()
                                           ordered set write protect()

And now have a write protect on a DMA pinned page, which is the
invarient we are trying to create.

The best algorithm I've thought of is something like:

 pte_map_lock()
  if (page) {
      if (wp) {
	  ptep_set_wrprotect()
	  /* Order with try_grab_compound_head(), either we see
	   * page_maybe_dma_pinned(), or they see the wrprotect */
	  get_page();

	  if (page_maybe_dma_pinned() && READ_ONCE(src_mm->has_pinned)) {
	       put_page();
	       ptep_clear_wrprotect()

	       // do copy
	       return
	  }
      } else {
	  get_page();
      }
      page_dup_rmap()
 pte_unmap_lock()

Then the do_wp_page() path would have to detect that the page is not
write protected under the pte lock inside the fault handler and just
do nothing. Ie the set/clear could be visible to the CPU and trigger a
spurious fault, but never trigger a COW.

Thus 'wp' becomes a 'lock' that prevents GUP from returning this page.

Very tricky, deserves a huge comment near the ptep_clear_wrprotect()

Consider the above algorithm beside the gup_fast() algorithm:

		if (!pte_access_permitted(pte, flags & FOLL_WRITE))
			goto pte_unmap;
                [..]
		head = try_grab_compound_head(page, 1, flags);
		if (!head)
			goto pte_unmap;
		if (unlikely(pte_val(pte) != pte_val(*ptep))) {
			put_compound_head(head, 1, flags);
			goto pte_unmap;

That last *ptep will check that the WP is not set after making
page_maybe_dma_pinned() true.

It still looks reasonable, the extra work is still just the additional
atomic in page_maybe_dma_pinned(), just everything else has to be very
carefully sequenced due to unlocked page table accessors.

> I think the WRITE_ONCE/READ_ONCE can actually be kept, because atomic ops
> should contain proper memory barriers already so the memory access orders
> should be guaranteed 

I always have to carefully check ORDERING in
Documentation/atomic_t.txt when asking those questions..

It seems very subtle to me, but yes, try_grab_compound_head() and
page_maybe_dma_pinned() are already paired ordering barriers, so both
the pte_val() on the GUP side and the READ_ONCE(has_pinned) look OK.

Jason