Re: [PATCH v3 25/28] userfaultfd: wp: fixup swap entries in change_pte_range

[Jerome Glisse]

On Wed, Mar 20, 2019 at 10:06:39AM +0800, Peter Xu wrote:
> In change_pte_range() we do nothing for uffd if the PTE is a swap
> entry.  That can lead to data mismatch if the page that we are going
> to write protect is swapped out when sending the UFFDIO_WRITEPROTECT.
> This patch applies/removes the uffd-wp bit even for the swap entries.
> 
> Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>

This one seems to address some of the comments i made on patch 17
not all thought. Maybe squash them together ?

> ---
> 
> I kept this patch a standalone one majorly to make review easier.  The
> patch can be considered as standalone or to squash into the patch
> "userfaultfd: wp: support swap and page migration".
> ---
>  mm/mprotect.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 96c0f521099d..a23e03053787 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -183,11 +183,11 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
>  			}
>  			ptep_modify_prot_commit(mm, addr, pte, ptent);
>  			pages++;
> -		} else if (IS_ENABLED(CONFIG_MIGRATION)) {
> +		} else if (is_swap_pte(oldpte)) {
>  			swp_entry_t entry = pte_to_swp_entry(oldpte);
> +			pte_t newpte;
>  
>  			if (is_write_migration_entry(entry)) {
> -				pte_t newpte;
>  				/*
>  				 * A protection check is difficult so
>  				 * just be safe and disable write
> @@ -198,22 +198,24 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
>  					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++;
> -			}
> -
> -			if (is_write_device_private_entry(entry)) {
> -				pte_t newpte;
> -
> +			} else if (is_write_device_private_entry(entry)) {
>  				/*
>  				 * We do not preserve soft-dirtiness. See
>  				 * copy_one_pte() for explanation.
>  				 */
>  				make_device_private_entry_read(&entry);
>  				newpte = swp_entry_to_pte(entry);
> -				set_pte_at(mm, addr, pte, newpte);
> +			} else {
> +				newpte = oldpte;
> +			}
>  
> +			if (uffd_wp)
> +				newpte = pte_swp_mkuffd_wp(newpte);
> +			else if (uffd_wp_resolve)
> +				newpte = pte_swp_clear_uffd_wp(newpte);
> +
> +			if (!pte_same(oldpte, newpte)) {
> +				set_pte_at(mm, addr, pte, newpte);
>  				pages++;
>  			}
>  		}
> -- 
> 2.17.1
>