Date:   Thu, 24 Jan 2019 12:56:15 +0800
From:   Peter Xu <peterx@...hat.com>
To:     Mike Rapoport <rppt@...ux.ibm.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Hugh Dickins <hughd@...gle.com>,
        Maya Gokhale <gokhale2@...l.gov>,
        Jerome Glisse <jglisse@...hat.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Martin Cracauer <cracauer@...s.org>,
        Denis Plotnikov <dplotnikov@...tuozzo.com>,
        Shaohua Li <shli@...com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Pavel Emelyanov <xemul@...allels.com>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Marty McFadden <mcfadden8@...l.gov>,
        Mike Rapoport <rppt@...ux.vnet.ibm.com>,
        Mel Gorman <mgorman@...e.de>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        "Dr . David Alan Gilbert" <dgilbert@...hat.com>
Subject: Re: [PATCH RFC 07/24] userfaultfd: wp: add the writeprotect API to
 userfaultfd ioctl

On Mon, Jan 21, 2019 at 12:42:33PM +0200, Mike Rapoport wrote:

[...]

> > @@ -1343,7 +1344,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
> > 
> >  		/* check not compatible vmas */
> >  		ret = -EINVAL;
> > -		if (!vma_can_userfault(cur))
> > +		if (!vma_can_userfault(cur, vm_flags))
> >  			goto out_unlock;
> > 
> >  		/*
> > @@ -1371,6 +1372,8 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
> >  			if (end & (vma_hpagesize - 1))
> >  				goto out_unlock;
> >  		}
> > +		if ((vm_flags & VM_UFFD_WP) && !(cur->vm_flags & VM_WRITE))
> > +			goto out_unlock;
> 
> This is problematic for the non-cooperative use-case. Way may still want to
> monitor a read-only area because it may eventually become writable, e.g. if
> the monitored process runs mprotect().

Firstly I think I should be able to change it to VM_MAYWRITE which
seems to suite more.

Meanwhile, frankly speaking I didn't think a lot about how to nest the
usages of uffd-wp and mprotect(), so far I was only considering it as
a replacement of mprotect().  But indeed it can happen that the
monitored process calls mprotect().  Is there an existing scenario of
such usage?

The problem is I'm uncertain about whether this scenario can work
after all.  Say, the monitor process A write protected process B's
page P, so logically A will definitely receive a message before B
writes to page P.  However here if we allow process B to do
mprotect(PROT_WRITE) upon page P and grant write permission to it on
its own, then A will not be able to capture the write operation at
all?  Then I don't know how it can work here... or whether we should
fail the mprotect() at least upon uffd-wp ranges?

> Particularity, for using uffd-wp as a replacement for soft-dirty would
> require it.
> 
> > 
> >  		/*
> >  		 * Check that this vma isn't already owned by a
> > @@ -1400,7 +1403,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
> >  	do {
> >  		cond_resched();
> > 
> > -		BUG_ON(!vma_can_userfault(vma));
> > +		BUG_ON(!vma_can_userfault(vma, vm_flags));
> >  		BUG_ON(vma->vm_userfaultfd_ctx.ctx &&
> >  		       vma->vm_userfaultfd_ctx.ctx != ctx);
> >  		WARN_ON(!(vma->vm_flags & VM_MAYWRITE));
> > @@ -1535,7 +1538,7 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
> >  		 * provides for more strict behavior to notice
> >  		 * unregistration errors.
> >  		 */
> > -		if (!vma_can_userfault(cur))
> > +		if (!vma_can_userfault(cur, cur->vm_flags))
> >  			goto out_unlock;
> > 
> >  		found = true;
> > @@ -1549,7 +1552,7 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
> >  	do {
> >  		cond_resched();
> > 
> > -		BUG_ON(!vma_can_userfault(vma));
> > +		BUG_ON(!vma_can_userfault(vma, vma->vm_flags));
> >  		WARN_ON(!(vma->vm_flags & VM_MAYWRITE));
> > 
> >  		/*
> > @@ -1760,6 +1763,46 @@ static int userfaultfd_zeropage(struct userfaultfd_ctx *ctx,
> >  	return ret;
> >  }
> > 
> > +static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx,
> > +				    unsigned long arg)
> > +{
> > +	int ret;
> > +	struct uffdio_writeprotect uffdio_wp;
> > +	struct uffdio_writeprotect __user *user_uffdio_wp;
> > +	struct userfaultfd_wake_range range;
> > +
> 
> In the non-cooperative mode the userfaultfd_writeprotect() may race with VM
> layout changes, pretty much as uffdio_copy() [1]. My solution for uffdio_copy()
> was to return -EAGAIN if such race is encountered. I think the same would
> apply here.

I tried to understand the problem at [1] but failed... could you help
to clarify it a bit more?

I'm quoting some of the discussions from [1] here directly between you
and Pavel:

  > Since the monitor cannot assume that the process will access all its memory
  > it has to copy some pages "in the background". A simple monitor may look
  > like:
  > 
  > 	for (;;) {
  > 		wait_for_uffd_events(timeout);
  > 		handle_uffd_events();
  > 		uffd_copy(some not faulted pages);
  > 	}
  > 
  > Then, if the "background" uffd_copy() races with fork, the pages we've
  > copied may be already present in parent's mappings before the call to
  > copy_page_range() and may be not.
  > 
  > If the pages were not present, uffd_copy'ing them again to the child's
  > memory would be ok.
  >
  > But if uffd_copy() was first to catch mmap_sem, and we would uffd_copy them
  > again, child process will get memory corruption.

Here I don't understand why the child process will get memory
corruption if uffd_copy() caught the mmap_sem first.

If it did it, then IMHO when uffd_copy() copies the page again it'll
simply get a -EEXIST showing that the page has already been copied.
Could you explain on why there will be a data corruption?

Thanks in advance,

>  
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df2cc96e77011cf7989208b206da9817e0321028
>

-- 
Peter Xu

Powered by blists - more mailing lists