Re: [PATCH v3 17/28] userfaultfd: wp: support swap and page migration

[Peter Xu]

On Thu, Apr 18, 2019 at 04:59:07PM -0400, Jerome Glisse wrote:
> On Wed, Mar 20, 2019 at 10:06:31AM +0800, Peter Xu wrote:
> > For either swap and page migration, we all use the bit 2 of the entry to
> > identify whether this entry is uffd write-protected.  It plays a similar
> > role as the existing soft dirty bit in swap entries but only for keeping
> > the uffd-wp tracking for a specific PTE/PMD.
> > 
> > Something special here is that when we want to recover the uffd-wp bit
> > from a swap/migration entry to the PTE bit we'll also need to take care
> > of the _PAGE_RW bit and make sure it's cleared, otherwise even with the
> > _PAGE_UFFD_WP bit we can't trap it at all.
> > 
> > Note that this patch removed two lines from "userfaultfd: wp: hook
> > userfault handler to write protection fault" where we try to remove the
> > VM_FAULT_WRITE from vmf->flags when uffd-wp is set for the VMA.  This
> > patch will still keep the write flag there.
> > 
> > Reviewed-by: Mike Rapoport <rppt@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> 
> Some missing thing see below.
> 
> [...]
> 
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 6405d56debee..c3d57fa890f2 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -736,6 +736,8 @@ copy_one_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
> >  				pte = swp_entry_to_pte(entry);
> >  				if (pte_swp_soft_dirty(*src_pte))
> >  					pte = pte_swp_mksoft_dirty(pte);
> > +				if (pte_swp_uffd_wp(*src_pte))
> > +					pte = pte_swp_mkuffd_wp(pte);
> >  				set_pte_at(src_mm, addr, src_pte, pte);
> >  			}
> >  		} else if (is_device_private_entry(entry)) {
> 
> You need to handle the is_device_private_entry() as the migration case
> too.

Hi, Jerome,

Yes I can simply add the handling, but I'd confess I haven't thought
clearly yet on how userfault-wp will be used with HMM (and that's
mostly because my unfamiliarity so far with HMM).  Could you give me
some hint on a most general and possible scenario?

> 
> 
> 
> > @@ -2825,6 +2827,10 @@ vm_fault_t 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);
> > +	if (pte_swp_uffd_wp(vmf->orig_pte)) {
> > +		pte = pte_mkuffd_wp(pte);
> > +		pte = pte_wrprotect(pte);
> > +	}
> >  	set_pte_at(vma->vm_mm, vmf->address, vmf->pte, pte);
> >  	arch_do_swap_page(vma->vm_mm, vma, vmf->address, pte, vmf->orig_pte);
> >  	vmf->orig_pte = pte;
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 181f5d2718a9..72cde187d4a1 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -241,6 +241,8 @@ static bool remove_migration_pte(struct page *page, struct vm_area_struct *vma,
> >  		entry = pte_to_swp_entry(*pvmw.pte);
> >  		if (is_write_migration_entry(entry))
> >  			pte = maybe_mkwrite(pte, vma);
> > +		else if (pte_swp_uffd_wp(*pvmw.pte))
> > +			pte = pte_mkuffd_wp(pte);
> >  
> >  		if (unlikely(is_zone_device_page(new))) {
> >  			if (is_device_private_page(new)) {
> 
> You need to handle is_device_private_page() case ie mark its swap
> as uffd_wp

Yes I can do this too.

> 
> > @@ -2301,6 +2303,8 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
> >  			swp_pte = swp_entry_to_pte(entry);
> >  			if (pte_soft_dirty(pte))
> >  				swp_pte = pte_swp_mksoft_dirty(swp_pte);
> > +			if (pte_uffd_wp(pte))
> > +				swp_pte = pte_swp_mkuffd_wp(swp_pte);
> >  			set_pte_at(mm, addr, ptep, swp_pte);
> >
> >  			/*
> > diff --git a/mm/mprotect.c b/mm/mprotect.c
> > index 855dddb07ff2..96c0f521099d 100644
> > --- a/mm/mprotect.c
> > +++ b/mm/mprotect.c
> > @@ -196,6 +196,8 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
> >  				newpte = swp_entry_to_pte(entry);
> >  				if (pte_swp_soft_dirty(oldpte))
> >  					newpte = pte_swp_mksoft_dirty(newpte);
> > +				if (pte_swp_uffd_wp(oldpte))
> > +					newpte = pte_swp_mkuffd_wp(newpte);
> >  				set_pte_at(mm, addr, pte, newpte);
> >  
> >  				pages++;
> 
> Need to handle is_write_device_private_entry() case just below
> that chunk.

This one is a bit special - because it's not only the private entries
that are missing but also all swap/migration entries, which is
explicitly handled by patch 25.  But I think I can just squash it into
this patch as you suggested.

Thanks,

-- 
Peter Xu