Re: [PATCH 1/5] mm: Introduce mm_struct.has_pinned

[Peter Xu]

On Tue, Sep 22, 2020 at 04:11:16PM -0300, Jason Gunthorpe wrote:
> 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.

Now I understand your point, I think..  So I guess it's not only about
has_pinned, but it should be a race between the fast-gup and the fork() code,
even if has_pinned is always set.

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

Is this get_page() a must to be after ptep_set_wrprotect() explicitly?  IIUC
what we need is to order ptep_set_wrprotect() and page_maybe_dma_pinned() here.
E.g., would a "mb()" work?

Another thing is, do we need similar thing for e.g. gup_pte_range(), so that
to guarantee ordering of try_grab_compound_head() and the pte change check?

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

Yes, iiuc do_wp_page() should be able to handle spurious write page faults like
this already, as below:

	vmf->ptl = pte_lockptr(vmf->vma->vm_mm, vmf->pmd);
	spin_lock(vmf->ptl);
        ...
	if (vmf->flags & FAULT_FLAG_WRITE) {
		if (!pte_write(entry))
			return do_wp_page(vmf);
		entry = pte_mkdirty(entry);
	}

So when spin_lock() returns:

  - When it's a real cow (not pinned pages; we write-protected it and it keeps
    write-protected), we should do cow here as usual.

  - When it's a fake cow (pinned pages), the write bit should have been
    recovered before the page table lock released, and we'll skip do_wp_page()
    and retry the page fault immediately.

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

Another question is, how about read fast-gup for pinning?  Because we can't use
the write-protect mechanism to block a read gup.  I remember we've discussed
similar things and iirc your point is "pinned pages should always be with
WRITE".  However now I still doubt it...  Because I feel like read gup is still
legal (as I mentioned previously - when device purely writes to the page and
the processor only reads from it).

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

Tricky!  I'm still thinking about some easier way but no much clue so far.
Hopefully we'll figure out something solid soon.

Thanks,

-- 
Peter Xu