linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] userfaultfd: Write protect when virtual memory range has no page table entry
@ 2021-03-19 15:24 Bui Quang Minh
  2021-03-20 16:32 ` Andrew Morton
  2021-03-22 10:14 ` Michal Hocko
  0 siblings, 2 replies; 11+ messages in thread
From: Bui Quang Minh @ 2021-03-19 15:24 UTC (permalink / raw)
  To: akpm; +Cc: linux-mm, linux-kernel, minhquangbui99

userfaultfd_writeprotect() use change_protection() to clear write bit in
page table entries (pte/pmd). So, later write to this virtual address
range causes a page fault, which is then handled by userspace program.
However, change_protection() has no effect when there is no page table
entries associated with that virtual memory range (a newly mapped memory
range). As a result, later access to that memory range causes allocating a
page table entry with write bit still set (due to VM_WRITE flag in
vma->vm_flags).

Add checks for VM_UFFD_WP in vma->vm_flags when allocating new page table
entry in missing page table entry page fault path.

Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 mm/huge_memory.c | 12 ++++++++++++
 mm/memory.c      | 10 ++++++++++
 2 files changed, 22 insertions(+)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index ae907a9c2050..9bb16a55a48c 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -636,6 +636,11 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
 
 		entry = mk_huge_pmd(page, vma->vm_page_prot);
 		entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
+		if (userfaultfd_wp(vma)) {
+			entry = pmd_wrprotect(entry);
+			entry = pmd_mkuffd_wp(entry);
+		}
+
 		page_add_new_anon_rmap(page, vma, haddr, true);
 		lru_cache_add_inactive_or_unevictable(page, vma);
 		pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
@@ -643,6 +648,13 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
 		update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
 		add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
 		mm_inc_nr_ptes(vma->vm_mm);
+
+		if (userfaultfd_huge_pmd_wp(vma, *vmf->pmd)) {
+			spin_unlock(vmf->ptl);
+			count_vm_event(THP_FAULT_ALLOC);
+			count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
+			return handle_userfault(vmf, VM_UFFD_WP);
+		}
 		spin_unlock(vmf->ptl);
 		count_vm_event(THP_FAULT_ALLOC);
 		count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
diff --git a/mm/memory.c b/mm/memory.c
index 5efa07fb6cdc..b835746545bf 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3564,6 +3564,11 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
 	if (vma->vm_flags & VM_WRITE)
 		entry = pte_mkwrite(pte_mkdirty(entry));
 
+	if (userfaultfd_wp(vma)) {
+		entry = pte_wrprotect(entry);
+		entry = pte_mkuffd_wp(entry);
+	}
+
 	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
 			&vmf->ptl);
 	if (!pte_none(*vmf->pte)) {
@@ -3590,6 +3595,11 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
 
 	/* No need to invalidate - it was non-present before */
 	update_mmu_cache(vma, vmf->address, vmf->pte);
+
+	if (userfaultfd_pte_wp(vma, *vmf->pte)) {
+		pte_unmap_unlock(vmf->pte, vmf->ptl);
+		return handle_userfault(vmf, VM_UFFD_WP);
+	}
 unlock:
 	pte_unmap_unlock(vmf->pte, vmf->ptl);
 	return ret;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] userfaultfd: Write protect when virtual memory range has no page table entry
  2021-03-19 15:24 [PATCH] userfaultfd: Write protect when virtual memory range has no page table entry Bui Quang Minh
@ 2021-03-20 16:32 ` Andrew Morton
  2021-03-22 10:14 ` Michal Hocko
  1 sibling, 0 replies; 11+ messages in thread
From: Andrew Morton @ 2021-03-20 16:32 UTC (permalink / raw)
  To: Bui Quang Minh
  Cc: linux-mm, linux-kernel, Peter Xu, Andrea Arcangeli, Axel Rasmussen

On Fri, 19 Mar 2021 22:24:28 +0700 Bui Quang Minh <minhquangbui99@gmail.com> wrote:

> userfaultfd_writeprotect() use change_protection() to clear write bit in
> page table entries (pte/pmd). So, later write to this virtual address
> range causes a page fault, which is then handled by userspace program.
> However, change_protection() has no effect when there is no page table
> entries associated with that virtual memory range (a newly mapped memory
> range). As a result, later access to that memory range causes allocating a
> page table entry with write bit still set (due to VM_WRITE flag in
> vma->vm_flags).
> 
> Add checks for VM_UFFD_WP in vma->vm_flags when allocating new page table
> entry in missing page table entry page fault path.

This sounds like a pretty significant bug?

Would it be possible to add a test to
tools/testing/selftests/vm/userfaultfd.c to check for this?  It should
fail without your patch and succeed with it.

Thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] userfaultfd: Write protect when virtual memory range has no page table entry
  2021-03-19 15:24 [PATCH] userfaultfd: Write protect when virtual memory range has no page table entry Bui Quang Minh
  2021-03-20 16:32 ` Andrew Morton
@ 2021-03-22 10:14 ` Michal Hocko
  2021-03-22 10:23   ` Mike Rapoport
  2021-03-22 13:00   ` Mike Rapoport
  1 sibling, 2 replies; 11+ messages in thread
From: Michal Hocko @ 2021-03-22 10:14 UTC (permalink / raw)
  To: Bui Quang Minh
  Cc: akpm, linux-mm, linux-kernel, Andrea Arcangeli, Mike Rapoport

Le'ts Andrea and Mike

On Fri 19-03-21 22:24:28, Bui Quang Minh wrote:
> userfaultfd_writeprotect() use change_protection() to clear write bit in
> page table entries (pte/pmd). So, later write to this virtual address
> range causes a page fault, which is then handled by userspace program.
> However, change_protection() has no effect when there is no page table
> entries associated with that virtual memory range (a newly mapped memory
> range). As a result, later access to that memory range causes allocating a
> page table entry with write bit still set (due to VM_WRITE flag in
> vma->vm_flags).
> 
> Add checks for VM_UFFD_WP in vma->vm_flags when allocating new page table
> entry in missing page table entry page fault path.

From the above it is not really clear whether this is a usability
problem or a bug of the interface.

> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
>  mm/huge_memory.c | 12 ++++++++++++
>  mm/memory.c      | 10 ++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index ae907a9c2050..9bb16a55a48c 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -636,6 +636,11 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
>  
>  		entry = mk_huge_pmd(page, vma->vm_page_prot);
>  		entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
> +		if (userfaultfd_wp(vma)) {
> +			entry = pmd_wrprotect(entry);
> +			entry = pmd_mkuffd_wp(entry);
> +		}
> +
>  		page_add_new_anon_rmap(page, vma, haddr, true);
>  		lru_cache_add_inactive_or_unevictable(page, vma);
>  		pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
> @@ -643,6 +648,13 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
>  		update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
>  		add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
>  		mm_inc_nr_ptes(vma->vm_mm);
> +
> +		if (userfaultfd_huge_pmd_wp(vma, *vmf->pmd)) {
> +			spin_unlock(vmf->ptl);
> +			count_vm_event(THP_FAULT_ALLOC);
> +			count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
> +			return handle_userfault(vmf, VM_UFFD_WP);
> +		}
>  		spin_unlock(vmf->ptl);
>  		count_vm_event(THP_FAULT_ALLOC);
>  		count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
> diff --git a/mm/memory.c b/mm/memory.c
> index 5efa07fb6cdc..b835746545bf 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3564,6 +3564,11 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
>  	if (vma->vm_flags & VM_WRITE)
>  		entry = pte_mkwrite(pte_mkdirty(entry));
>  
> +	if (userfaultfd_wp(vma)) {
> +		entry = pte_wrprotect(entry);
> +		entry = pte_mkuffd_wp(entry);
> +	}
> +
>  	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
>  			&vmf->ptl);
>  	if (!pte_none(*vmf->pte)) {
> @@ -3590,6 +3595,11 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
>  
>  	/* No need to invalidate - it was non-present before */
>  	update_mmu_cache(vma, vmf->address, vmf->pte);
> +
> +	if (userfaultfd_pte_wp(vma, *vmf->pte)) {
> +		pte_unmap_unlock(vmf->pte, vmf->ptl);
> +		return handle_userfault(vmf, VM_UFFD_WP);
> +	}
>  unlock:
>  	pte_unmap_unlock(vmf->pte, vmf->ptl);
>  	return ret;
> -- 
> 2.25.1

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] userfaultfd: Write protect when virtual memory range has no page table entry
  2021-03-22 10:14 ` Michal Hocko
@ 2021-03-22 10:23   ` Mike Rapoport
  2021-03-22 13:00   ` Mike Rapoport
  1 sibling, 0 replies; 11+ messages in thread
From: Mike Rapoport @ 2021-03-22 10:23 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Bui Quang Minh, akpm, linux-mm, linux-kernel, Andrea Arcangeli, Peter Xu

+Peter

On Mon, Mar 22, 2021 at 11:14:37AM +0100, Michal Hocko wrote:
> Le'ts Andrea and Mike
> 
> On Fri 19-03-21 22:24:28, Bui Quang Minh wrote:
> > userfaultfd_writeprotect() use change_protection() to clear write bit in
> > page table entries (pte/pmd). So, later write to this virtual address
> > range causes a page fault, which is then handled by userspace program.
> > However, change_protection() has no effect when there is no page table
> > entries associated with that virtual memory range (a newly mapped memory
> > range). As a result, later access to that memory range causes allocating a
> > page table entry with write bit still set (due to VM_WRITE flag in
> > vma->vm_flags).
> > 
> > Add checks for VM_UFFD_WP in vma->vm_flags when allocating new page table
> > entry in missing page table entry page fault path.
> 
> From the above it is not really clear whether this is a usability
> problem or a bug of the interface.
> 
> > Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> > ---
> >  mm/huge_memory.c | 12 ++++++++++++
> >  mm/memory.c      | 10 ++++++++++
> >  2 files changed, 22 insertions(+)
> > 
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index ae907a9c2050..9bb16a55a48c 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -636,6 +636,11 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
> >  
> >  		entry = mk_huge_pmd(page, vma->vm_page_prot);
> >  		entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
> > +		if (userfaultfd_wp(vma)) {
> > +			entry = pmd_wrprotect(entry);
> > +			entry = pmd_mkuffd_wp(entry);
> > +		}
> > +
> >  		page_add_new_anon_rmap(page, vma, haddr, true);
> >  		lru_cache_add_inactive_or_unevictable(page, vma);
> >  		pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
> > @@ -643,6 +648,13 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
> >  		update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
> >  		add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
> >  		mm_inc_nr_ptes(vma->vm_mm);
> > +
> > +		if (userfaultfd_huge_pmd_wp(vma, *vmf->pmd)) {
> > +			spin_unlock(vmf->ptl);
> > +			count_vm_event(THP_FAULT_ALLOC);
> > +			count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
> > +			return handle_userfault(vmf, VM_UFFD_WP);
> > +		}
> >  		spin_unlock(vmf->ptl);
> >  		count_vm_event(THP_FAULT_ALLOC);
> >  		count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 5efa07fb6cdc..b835746545bf 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -3564,6 +3564,11 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
> >  	if (vma->vm_flags & VM_WRITE)
> >  		entry = pte_mkwrite(pte_mkdirty(entry));
> >  
> > +	if (userfaultfd_wp(vma)) {
> > +		entry = pte_wrprotect(entry);
> > +		entry = pte_mkuffd_wp(entry);
> > +	}
> > +
> >  	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
> >  			&vmf->ptl);
> >  	if (!pte_none(*vmf->pte)) {
> > @@ -3590,6 +3595,11 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
> >  
> >  	/* No need to invalidate - it was non-present before */
> >  	update_mmu_cache(vma, vmf->address, vmf->pte);
> > +
> > +	if (userfaultfd_pte_wp(vma, *vmf->pte)) {
> > +		pte_unmap_unlock(vmf->pte, vmf->ptl);
> > +		return handle_userfault(vmf, VM_UFFD_WP);
> > +	}
> >  unlock:
> >  	pte_unmap_unlock(vmf->pte, vmf->ptl);
> >  	return ret;
> > -- 
> > 2.25.1
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Sincerely yours,
Mike.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] userfaultfd: Write protect when virtual memory range has no page table entry
  2021-03-22 10:14 ` Michal Hocko
  2021-03-22 10:23   ` Mike Rapoport
@ 2021-03-22 13:00   ` Mike Rapoport
  2021-03-22 13:27     ` Peter Xu
                       ` (2 more replies)
  1 sibling, 3 replies; 11+ messages in thread
From: Mike Rapoport @ 2021-03-22 13:00 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Bui Quang Minh, akpm, linux-mm, linux-kernel, Andrea Arcangeli

On Mon, Mar 22, 2021 at 11:14:37AM +0100, Michal Hocko wrote:
> Le'ts Andrea and Mike
> 
> On Fri 19-03-21 22:24:28, Bui Quang Minh wrote:
> > userfaultfd_writeprotect() use change_protection() to clear write bit in
> > page table entries (pte/pmd). So, later write to this virtual address
> > range causes a page fault, which is then handled by userspace program.
> > However, change_protection() has no effect when there is no page table
> > entries associated with that virtual memory range (a newly mapped memory
> > range). As a result, later access to that memory range causes allocating a
> > page table entry with write bit still set (due to VM_WRITE flag in
> > vma->vm_flags).
> > 
> > Add checks for VM_UFFD_WP in vma->vm_flags when allocating new page table
> > entry in missing page table entry page fault path.
> 
> From the above it is not really clear whether this is a usability
> problem or a bug of the interface.

I'd say it's usability/documentation clarity issue. 
Userspace can register an area with

	UFFDIO_REGISTER_MODE_MISSING | UFFDIO_REGISTER_MODE_WP

and then it will be notified either when page table has no entry for a
virtual address or when there is a write to a write protected address.
 
> > Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> > ---
> >  mm/huge_memory.c | 12 ++++++++++++
> >  mm/memory.c      | 10 ++++++++++
> >  2 files changed, 22 insertions(+)
> > 
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index ae907a9c2050..9bb16a55a48c 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -636,6 +636,11 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
> >  
> >  		entry = mk_huge_pmd(page, vma->vm_page_prot);
> >  		entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
> > +		if (userfaultfd_wp(vma)) {
> > +			entry = pmd_wrprotect(entry);
> > +			entry = pmd_mkuffd_wp(entry);
> > +		}
> > +
> >  		page_add_new_anon_rmap(page, vma, haddr, true);
> >  		lru_cache_add_inactive_or_unevictable(page, vma);
> >  		pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
> > @@ -643,6 +648,13 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
> >  		update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
> >  		add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
> >  		mm_inc_nr_ptes(vma->vm_mm);
> > +
> > +		if (userfaultfd_huge_pmd_wp(vma, *vmf->pmd)) {
> > +			spin_unlock(vmf->ptl);
> > +			count_vm_event(THP_FAULT_ALLOC);
> > +			count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
> > +			return handle_userfault(vmf, VM_UFFD_WP);
> > +		}
> >  		spin_unlock(vmf->ptl);
> >  		count_vm_event(THP_FAULT_ALLOC);
> >  		count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 5efa07fb6cdc..b835746545bf 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -3564,6 +3564,11 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
> >  	if (vma->vm_flags & VM_WRITE)
> >  		entry = pte_mkwrite(pte_mkdirty(entry));
> >  
> > +	if (userfaultfd_wp(vma)) {
> > +		entry = pte_wrprotect(entry);
> > +		entry = pte_mkuffd_wp(entry);
> > +	}
> > +
> >  	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
> >  			&vmf->ptl);
> >  	if (!pte_none(*vmf->pte)) {
> > @@ -3590,6 +3595,11 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
> >  
> >  	/* No need to invalidate - it was non-present before */
> >  	update_mmu_cache(vma, vmf->address, vmf->pte);
> > +
> > +	if (userfaultfd_pte_wp(vma, *vmf->pte)) {
> > +		pte_unmap_unlock(vmf->pte, vmf->ptl);
> > +		return handle_userfault(vmf, VM_UFFD_WP);
> > +	}
> >  unlock:
> >  	pte_unmap_unlock(vmf->pte, vmf->ptl);
> >  	return ret;
> > -- 
> > 2.25.1
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Sincerely yours,
Mike.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] userfaultfd: Write protect when virtual memory range has no page table entry
  2021-03-22 13:00   ` Mike Rapoport
@ 2021-03-22 13:27     ` Peter Xu
  2021-03-22 13:49     ` Michal Hocko
  2021-03-23  2:48     ` Bui Quang Minh
  2 siblings, 0 replies; 11+ messages in thread
From: Peter Xu @ 2021-03-22 13:27 UTC (permalink / raw)
  To: Mike Rapoport
  Cc: Michal Hocko, Bui Quang Minh, akpm, linux-mm, linux-kernel,
	Andrea Arcangeli, Axel Rasmussen

On Mon, Mar 22, 2021 at 03:00:37PM +0200, Mike Rapoport wrote:
> On Mon, Mar 22, 2021 at 11:14:37AM +0100, Michal Hocko wrote:
> > Le'ts Andrea and Mike
> > 
> > On Fri 19-03-21 22:24:28, Bui Quang Minh wrote:
> > > userfaultfd_writeprotect() use change_protection() to clear write bit in
> > > page table entries (pte/pmd). So, later write to this virtual address
> > > range causes a page fault, which is then handled by userspace program.
> > > However, change_protection() has no effect when there is no page table
> > > entries associated with that virtual memory range (a newly mapped memory
> > > range). As a result, later access to that memory range causes allocating a
> > > page table entry with write bit still set (due to VM_WRITE flag in
> > > vma->vm_flags).
> > > 
> > > Add checks for VM_UFFD_WP in vma->vm_flags when allocating new page table
> > > entry in missing page table entry page fault path.
> > 
> > From the above it is not really clear whether this is a usability
> > problem or a bug of the interface.
> 
> I'd say it's usability/documentation clarity issue. 
> Userspace can register an area with
> 
> 	UFFDIO_REGISTER_MODE_MISSING | UFFDIO_REGISTER_MODE_WP
> 
> and then it will be notified either when page table has no entry for a
> virtual address or when there is a write to a write protected address.

Right, it's debatable to make it a default behavior since there can be some
application that does not care about zero pages - since currently userfaultfd
wr-protect only works for anonymous page, so any missing entry means a zero
page to be allocated.

Currently if we want to wr-protect all pages including zero pages, we can
either do as what Mike suggested, or one can pre-read the range to fault in the
pages.  The double-mode solution should be even better, since then the user app
would have a chance to know it's zero page without even scanning it.

It'll be a new story for page-cache backed memory regions, and that's indeed
the major work contained in the upcoming shmem+hugetlbfs uffd-wp support series
[1] to allow persisting uffd-wp/write bit even without page table entries,
because then the entry can be null even when there's page cache (so it'll
bypass uffdio missing messages too).

If this behavior is very desired, how about define a new feature bit, say
UFFD_FEATURE_WP_UNALLOCATED?  This could be more efficient than registering
with two modes, since we can do the later UFFDIO_COPY along with the MISSING
page fault as in this patch, meanwhile that'll also contain the same semantic
as UFFDIO_ZEROCOPY so less data copy too (UFFDIO_ZEROCOPY does not support
UFFDIO_COPY_MODE_WP so far).  However we need to be careful on mixture use of
these, e.g., I think UFFD_FEATURE_WP_UNALLOCATED at least shouldn't be allowed
with UFFDIO_REGISTER_MODE_MISSING, otherwise the behavior of missing fault on
uffd-wp area will be undefined.

Thoughts?

[1] https://lore.kernel.org/lkml/20210115170907.24498-1-peterx@redhat.com/

Thanks,

-- 
Peter Xu


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] userfaultfd: Write protect when virtual memory range has no page table entry
  2021-03-22 13:00   ` Mike Rapoport
  2021-03-22 13:27     ` Peter Xu
@ 2021-03-22 13:49     ` Michal Hocko
  2021-03-31 14:49       ` Michal Hocko
  2021-03-23  2:48     ` Bui Quang Minh
  2 siblings, 1 reply; 11+ messages in thread
From: Michal Hocko @ 2021-03-22 13:49 UTC (permalink / raw)
  To: Mike Rapoport
  Cc: Bui Quang Minh, akpm, linux-mm, linux-kernel, Andrea Arcangeli

On Mon 22-03-21 15:00:37, Mike Rapoport wrote:
> On Mon, Mar 22, 2021 at 11:14:37AM +0100, Michal Hocko wrote:
> > Le'ts Andrea and Mike
> > 
> > On Fri 19-03-21 22:24:28, Bui Quang Minh wrote:
> > > userfaultfd_writeprotect() use change_protection() to clear write bit in
> > > page table entries (pte/pmd). So, later write to this virtual address
> > > range causes a page fault, which is then handled by userspace program.
> > > However, change_protection() has no effect when there is no page table
> > > entries associated with that virtual memory range (a newly mapped memory
> > > range). As a result, later access to that memory range causes allocating a
> > > page table entry with write bit still set (due to VM_WRITE flag in
> > > vma->vm_flags).
> > > 
> > > Add checks for VM_UFFD_WP in vma->vm_flags when allocating new page table
> > > entry in missing page table entry page fault path.
> > 
> > From the above it is not really clear whether this is a usability
> > problem or a bug of the interface.
> 
> I'd say it's usability/documentation clarity issue. 
> Userspace can register an area with
> 
> 	UFFDIO_REGISTER_MODE_MISSING | UFFDIO_REGISTER_MODE_WP
> 
> and then it will be notified either when page table has no entry for a
> virtual address or when there is a write to a write protected address.

Thanks for the clarification! I have suspected this to be the case but
I am not really familiar with the interface to have any strong statement
here. Maybe we want to document this explicitly.
-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] userfaultfd: Write protect when virtual memory range has no page table entry
  2021-03-22 13:00   ` Mike Rapoport
  2021-03-22 13:27     ` Peter Xu
  2021-03-22 13:49     ` Michal Hocko
@ 2021-03-23  2:48     ` Bui Quang Minh
  2021-03-23 15:12       ` Peter Xu
  2 siblings, 1 reply; 11+ messages in thread
From: Bui Quang Minh @ 2021-03-23  2:48 UTC (permalink / raw)
  To: Mike Rapoport
  Cc: Michal Hocko, akpm, linux-mm, linux-kernel, Andrea Arcangeli, Peter Xu

On Mon, Mar 22, 2021 at 03:00:37PM +0200, Mike Rapoport wrote:
> On Mon, Mar 22, 2021 at 11:14:37AM +0100, Michal Hocko wrote:
> > Le'ts Andrea and Mike
> > 
> > On Fri 19-03-21 22:24:28, Bui Quang Minh wrote:
> > > userfaultfd_writeprotect() use change_protection() to clear write bit in
> > > page table entries (pte/pmd). So, later write to this virtual address
> > > range causes a page fault, which is then handled by userspace program.
> > > However, change_protection() has no effect when there is no page table
> > > entries associated with that virtual memory range (a newly mapped memory
> > > range). As a result, later access to that memory range causes allocating a
> > > page table entry with write bit still set (due to VM_WRITE flag in
> > > vma->vm_flags).
> > > 
> > > Add checks for VM_UFFD_WP in vma->vm_flags when allocating new page table
> > > entry in missing page table entry page fault path.
> > 
> > From the above it is not really clear whether this is a usability
> > problem or a bug of the interface.
> 
> I'd say it's usability/documentation clarity issue. 
> Userspace can register an area with
> 
> 	UFFDIO_REGISTER_MODE_MISSING | UFFDIO_REGISTER_MODE_WP
> 
> and then it will be notified either when page table has no entry for a
> virtual address or when there is a write to a write protected address.

Yes, you are right. I saw a patch from Peter to linux-man and saw that

	"When there is only UFFDIO_REGISTER_MODE_WP registered, the userspace
	will not receive any message when a missing page is written"

It's my mistake that I didn't look at the documentation carefully when playing
around.

Thanks,
Quang Minh.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] userfaultfd: Write protect when virtual memory range has no page table entry
  2021-03-23  2:48     ` Bui Quang Minh
@ 2021-03-23 15:12       ` Peter Xu
  0 siblings, 0 replies; 11+ messages in thread
From: Peter Xu @ 2021-03-23 15:12 UTC (permalink / raw)
  To: Bui Quang Minh
  Cc: Mike Rapoport, Michal Hocko, akpm, linux-mm, linux-kernel,
	Andrea Arcangeli

On Tue, Mar 23, 2021 at 09:48:03AM +0700, Bui Quang Minh wrote:
> On Mon, Mar 22, 2021 at 03:00:37PM +0200, Mike Rapoport wrote:
> > On Mon, Mar 22, 2021 at 11:14:37AM +0100, Michal Hocko wrote:
> > > Le'ts Andrea and Mike
> > > 
> > > On Fri 19-03-21 22:24:28, Bui Quang Minh wrote:
> > > > userfaultfd_writeprotect() use change_protection() to clear write bit in
> > > > page table entries (pte/pmd). So, later write to this virtual address
> > > > range causes a page fault, which is then handled by userspace program.
> > > > However, change_protection() has no effect when there is no page table
> > > > entries associated with that virtual memory range (a newly mapped memory
> > > > range). As a result, later access to that memory range causes allocating a
> > > > page table entry with write bit still set (due to VM_WRITE flag in
> > > > vma->vm_flags).
> > > > 
> > > > Add checks for VM_UFFD_WP in vma->vm_flags when allocating new page table
> > > > entry in missing page table entry page fault path.
> > > 
> > > From the above it is not really clear whether this is a usability
> > > problem or a bug of the interface.
> > 
> > I'd say it's usability/documentation clarity issue. 
> > Userspace can register an area with
> > 
> > 	UFFDIO_REGISTER_MODE_MISSING | UFFDIO_REGISTER_MODE_WP
> > 
> > and then it will be notified either when page table has no entry for a
> > virtual address or when there is a write to a write protected address.
> 
> Yes, you are right. I saw a patch from Peter to linux-man and saw that
> 
> 	"When there is only UFFDIO_REGISTER_MODE_WP registered, the userspace
> 	will not receive any message when a missing page is written"
> 
> It's my mistake that I didn't look at the documentation carefully when playing
> around.

If there's a mistake, it's me forgetting to write the document when the feature
landed.. :)

But still I think you raised a good point, and I was also serious when
proposing that UFFD_FEATURE_WP_UNALLOCATED idea, since indeed we had similar
issue as QEMU live snapshot e.g. when the guest enabled kernel init_on_free,
meanwhile virtio-balloon could have recycled the same zero page, then that zero
page could got replaced with some garbage page when saving the snapshot, while
we expect it to come back as strictly a zero page, since when the guest OS
reuse this page it'll skip zeroing it assuming it's a zero page.

QEMU plans to fix it using pre-faults as UFFDIO_COPY will complicate the live
snapshot framework, but UFFD_FEATURE_WP_UNALLOCATED should be more efficient.
It's just that we still needs to keep the old behavior.

I'll see whether I can prepare a patch for it shortly, with some test case too.

Thanks,

-- 
Peter Xu


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] userfaultfd: Write protect when virtual memory range has no page table entry
  2021-03-22 13:49     ` Michal Hocko
@ 2021-03-31 14:49       ` Michal Hocko
  2021-04-01  0:24         ` Andrew Morton
  0 siblings, 1 reply; 11+ messages in thread
From: Michal Hocko @ 2021-03-31 14:49 UTC (permalink / raw)
  To: akpm
  Cc: Mike Rapoport, Bui Quang Minh, linux-mm, linux-kernel, Andrea Arcangeli

On Mon 22-03-21 14:49:35, Michal Hocko wrote:
> On Mon 22-03-21 15:00:37, Mike Rapoport wrote:
> > On Mon, Mar 22, 2021 at 11:14:37AM +0100, Michal Hocko wrote:
> > > Le'ts Andrea and Mike
> > > 
> > > On Fri 19-03-21 22:24:28, Bui Quang Minh wrote:
> > > > userfaultfd_writeprotect() use change_protection() to clear write bit in
> > > > page table entries (pte/pmd). So, later write to this virtual address
> > > > range causes a page fault, which is then handled by userspace program.
> > > > However, change_protection() has no effect when there is no page table
> > > > entries associated with that virtual memory range (a newly mapped memory
> > > > range). As a result, later access to that memory range causes allocating a
> > > > page table entry with write bit still set (due to VM_WRITE flag in
> > > > vma->vm_flags).
> > > > 
> > > > Add checks for VM_UFFD_WP in vma->vm_flags when allocating new page table
> > > > entry in missing page table entry page fault path.
> > > 
> > > From the above it is not really clear whether this is a usability
> > > problem or a bug of the interface.
> > 
> > I'd say it's usability/documentation clarity issue. 
> > Userspace can register an area with
> > 
> > 	UFFDIO_REGISTER_MODE_MISSING | UFFDIO_REGISTER_MODE_WP
> > 
> > and then it will be notified either when page table has no entry for a
> > virtual address or when there is a write to a write protected address.
> 
> Thanks for the clarification! I have suspected this to be the case but
> I am not really familiar with the interface to have any strong statement
> here. Maybe we want to document this explicitly.

Btw. Andrew the patch still seems to be in mmotm. Do you plan to keep it
there?

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] userfaultfd: Write protect when virtual memory range has no page table entry
  2021-03-31 14:49       ` Michal Hocko
@ 2021-04-01  0:24         ` Andrew Morton
  0 siblings, 0 replies; 11+ messages in thread
From: Andrew Morton @ 2021-04-01  0:24 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Mike Rapoport, Bui Quang Minh, linux-mm, linux-kernel, Andrea Arcangeli

On Wed, 31 Mar 2021 16:49:27 +0200 Michal Hocko <mhocko@suse.com> wrote:

> > Thanks for the clarification! I have suspected this to be the case but
> > I am not really familiar with the interface to have any strong statement
> > here. Maybe we want to document this explicitly.
> 
> Btw. Andrew the patch still seems to be in mmotm. Do you plan to keep it
> there?

Dropped, thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-04-01  0:25 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-19 15:24 [PATCH] userfaultfd: Write protect when virtual memory range has no page table entry Bui Quang Minh
2021-03-20 16:32 ` Andrew Morton
2021-03-22 10:14 ` Michal Hocko
2021-03-22 10:23   ` Mike Rapoport
2021-03-22 13:00   ` Mike Rapoport
2021-03-22 13:27     ` Peter Xu
2021-03-22 13:49     ` Michal Hocko
2021-03-31 14:49       ` Michal Hocko
2021-04-01  0:24         ` Andrew Morton
2021-03-23  2:48     ` Bui Quang Minh
2021-03-23 15:12       ` Peter Xu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).