Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1591182 > unrolled thread

[PATCH 0/4] thp: fix few MADV_DONTNEED races

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] thp: fix few MADV_DONTNEED races "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-03-02 16:20 +0100
    [PATCH 4/4] thp: fix MADV_DONTNEED vs clear soft dirty race "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-03-02 17:00 +0100
      Re: [PATCH 4/4] thp: fix MADV_DONTNEED vs clear soft dirty race Andrew Morton <akpm@linux-foundation.org> - 2017-03-03 23:50 +0100
    [PATCH 3/4] thp: fix MADV_DONTNEED vs. MADV_FREE race "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-03-02 17:00 +0100
      Re: [PATCH 3/4] thp: fix MADV_DONTNEED vs. MADV_FREE race "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2017-03-03 06:40 +0100
        Re: [PATCH 3/4] thp: fix MADV_DONTNEED vs. MADV_FREE race "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-03-03 11:30 +0100
          Re: [PATCH 3/4] thp: fix MADV_DONTNEED vs. MADV_FREE race Minchan Kim <minchan@kernel.org> - 2017-03-06 03:00 +0100
            Re: [PATCH 3/4] thp: fix MADV_DONTNEED vs. MADV_FREE race "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-03-07 15:40 +0100
      Re: [PATCH 3/4] thp: fix MADV_DONTNEED vs. MADV_FREE race "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-03-06 03:50 +0100
        Re: [PATCH 3/4] thp: fix MADV_DONTNEED vs. MADV_FREE race "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-03-07 15:50 +0100
    [PATCH 1/4] thp: reduce indentation level in change_huge_pmd() "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-03-02 17:00 +0100
    [PATCH 2/4] thp: fix MADV_DONTNEED vs. numa balancing race "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-03-03 03:20 +0100
      Re: [PATCH 2/4] thp: fix MADV_DONTNEED vs. numa balancing race Dave Hansen <dave.hansen@intel.com> - 2017-03-03 18:20 +0100

#1591182 — [PATCH 0/4] thp: fix few MADV_DONTNEED races

For MADV_DONTNEED to work properly with huge pages, it's critical to not clear
pmd intermittently unless you hold down_write(mmap_sem). Otherwise
MADV_DONTNEED can miss the THP which can lead to userspace breakage.

See example of such race in commit message of patch 2/4.

All these races are found by code inspection. I haven't seen them triggered. 
I don't think it's worth to apply them to stable@.

Kirill A. Shutemov (4):
  thp: reduce indentation level in change_huge_pmd()
  thp: fix MADV_DONTNEED vs. numa balancing race
  thp: fix MADV_DONTNEED vs. MADV_FREE race
  thp: fix MADV_DONTNEED vs clear soft dirty race

 fs/proc/task_mmu.c |  9 +++++-
 mm/huge_memory.c   | 86 ++++++++++++++++++++++++++++++++++++------------------
 2 files changed, 66 insertions(+), 29 deletions(-)

-- 
2.11.0

[toc] | [next] | [standalone]

#1591221 — [PATCH 4/4] thp: fix MADV_DONTNEED vs clear soft dirty race

Yet another instance of the same race.

Fix is identical to change_huge_pmd().

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 fs/proc/task_mmu.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index ee3efb229ef6..0ce5294abc2c 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -899,7 +899,14 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma,
 static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
 		unsigned long addr, pmd_t *pmdp)
 {
-	pmd_t pmd = pmdp_huge_get_and_clear(vma->vm_mm, addr, pmdp);
+	pmd_t pmd = *pmdp;
+
+	/* See comment in change_huge_pmd() */
+	pmdp_invalidate(vma, addr, pmdp);
+	if (pmd_dirty(*pmdp))
+		pmd = pmd_mkdirty(pmd);
+	if (pmd_young(*pmdp))
+		pmd = pmd_mkyoung(pmd);
 
 	pmd = pmd_wrprotect(pmd);
 	pmd = pmd_clear_soft_dirty(pmd);
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1592325 — Re: [PATCH 4/4] thp: fix MADV_DONTNEED vs clear soft dirty race

On Thu,  2 Mar 2017 18:10:34 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:

> Yet another instance of the same race.
> 
> Fix is identical to change_huge_pmd().

Nit: someone who is reading this changelog a year from now will be
quite confused - how do they work out what the race was?

I'll add

: See "thp: fix MADV_DONTNEED vs. numa balancing race" for more details.

to the changelogs to help them a bit.

Also, it wasn't a great idea to start this series with a "Restructure
code in preparation for a fix".  If people later start hitting this
race, the fixes will be difficult to backport.  I'm OK with taking that
risk, but please do bear this in mind in the future.

[toc] | [prev] | [next] | [standalone]

#1591223 — [PATCH 3/4] thp: fix MADV_DONTNEED vs. MADV_FREE race

Basically the same race as with numa balancing in change_huge_pmd(), but
a bit simpler to mitigate: we don't need to preserve dirty/young flags
here due to MADV_FREE functionality.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Minchan Kim <minchan@kernel.org>
---
 mm/huge_memory.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index bb2b3646bd78..324217c31ec9 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1566,8 +1566,6 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
 		deactivate_page(page);
 
 	if (pmd_young(orig_pmd) || pmd_dirty(orig_pmd)) {
-		orig_pmd = pmdp_huge_get_and_clear_full(tlb->mm, addr, pmd,
-			tlb->fullmm);
 		orig_pmd = pmd_mkold(orig_pmd);
 		orig_pmd = pmd_mkclean(orig_pmd);
 
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1591668 — Re: [PATCH 3/4] thp: fix MADV_DONTNEED vs. MADV_FREE race

On March 02, 2017 11:11 PM Kirill A. Shutemov wrote: 
> 
> Basically the same race as with numa balancing in change_huge_pmd(), but
> a bit simpler to mitigate: we don't need to preserve dirty/young flags
> here due to MADV_FREE functionality.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Minchan Kim <minchan@kernel.org>
> ---
>  mm/huge_memory.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index bb2b3646bd78..324217c31ec9 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1566,8 +1566,6 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
>  		deactivate_page(page);
> 
>  	if (pmd_young(orig_pmd) || pmd_dirty(orig_pmd)) {
> -		orig_pmd = pmdp_huge_get_and_clear_full(tlb->mm, addr, pmd,
> -			tlb->fullmm);
>  		orig_pmd = pmd_mkold(orig_pmd);
>  		orig_pmd = pmd_mkclean(orig_pmd);
> 
$ grep -n set_pmd_at  linux-4.10/arch/powerpc/mm/pgtable-book3s64.c

/*
 * set a new huge pmd. We should not be called for updating
 * an existing pmd entry. That should go via pmd_hugepage_update.
 */
void set_pmd_at(struct mm_struct *mm, unsigned long addr,

[toc] | [prev] | [next] | [standalone]

#1591839 — Re: [PATCH 3/4] thp: fix MADV_DONTNEED vs. MADV_FREE race

On Fri, Mar 03, 2017 at 01:35:11PM +0800, Hillf Danton wrote:
> 
> On March 02, 2017 11:11 PM Kirill A. Shutemov wrote: 
> > 
> > Basically the same race as with numa balancing in change_huge_pmd(), but
> > a bit simpler to mitigate: we don't need to preserve dirty/young flags
> > here due to MADV_FREE functionality.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Cc: Minchan Kim <minchan@kernel.org>
> > ---
> >  mm/huge_memory.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index bb2b3646bd78..324217c31ec9 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -1566,8 +1566,6 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
> >  		deactivate_page(page);
> > 
> >  	if (pmd_young(orig_pmd) || pmd_dirty(orig_pmd)) {
> > -		orig_pmd = pmdp_huge_get_and_clear_full(tlb->mm, addr, pmd,
> > -			tlb->fullmm);
> >  		orig_pmd = pmd_mkold(orig_pmd);
> >  		orig_pmd = pmd_mkclean(orig_pmd);
> > 
> $ grep -n set_pmd_at  linux-4.10/arch/powerpc/mm/pgtable-book3s64.c
> 
> /*
>  * set a new huge pmd. We should not be called for updating
>  * an existing pmd entry. That should go via pmd_hugepage_update.
>  */
> void set_pmd_at(struct mm_struct *mm, unsigned long addr,

+Aneesh.

Urgh... Power is special again.

I think this should work fine.

From 056914fa025992c0a2212aee057c26307ce60238 Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Date: Thu, 2 Mar 2017 16:47:45 +0300
Subject: [PATCH] thp: fix MADV_DONTNEED vs. MADV_FREE race

Basically the same race as with numa balancing in change_huge_pmd(), but
a bit simpler to mitigate: we don't need to preserve dirty/young flags
here due to MADV_FREE functionality.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Minchan Kim <minchan@kernel.org>
---
 mm/huge_memory.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index bb2b3646bd78..23c1b3d58cf4 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1566,8 +1566,7 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
 		deactivate_page(page);
 
 	if (pmd_young(orig_pmd) || pmd_dirty(orig_pmd)) {
-		orig_pmd = pmdp_huge_get_and_clear_full(tlb->mm, addr, pmd,
-			tlb->fullmm);
+		pmdp_invalidate(vma, addr, pmd);
 		orig_pmd = pmd_mkold(orig_pmd);
 		orig_pmd = pmd_mkclean(orig_pmd);
 
-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1592886 — Re: [PATCH 3/4] thp: fix MADV_DONTNEED vs. MADV_FREE race

Hello, Kirill,

On Fri, Mar 03, 2017 at 01:26:36PM +0300, Kirill A. Shutemov wrote:
> On Fri, Mar 03, 2017 at 01:35:11PM +0800, Hillf Danton wrote:
> > 
> > On March 02, 2017 11:11 PM Kirill A. Shutemov wrote: 
> > > 
> > > Basically the same race as with numa balancing in change_huge_pmd(), but
> > > a bit simpler to mitigate: we don't need to preserve dirty/young flags
> > > here due to MADV_FREE functionality.
> > > 
> > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > Cc: Minchan Kim <minchan@kernel.org>
> > > ---
> > >  mm/huge_memory.c | 2 --
> > >  1 file changed, 2 deletions(-)
> > > 
> > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > > index bb2b3646bd78..324217c31ec9 100644
> > > --- a/mm/huge_memory.c
> > > +++ b/mm/huge_memory.c
> > > @@ -1566,8 +1566,6 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
> > >  		deactivate_page(page);
> > > 
> > >  	if (pmd_young(orig_pmd) || pmd_dirty(orig_pmd)) {
> > > -		orig_pmd = pmdp_huge_get_and_clear_full(tlb->mm, addr, pmd,
> > > -			tlb->fullmm);
> > >  		orig_pmd = pmd_mkold(orig_pmd);
> > >  		orig_pmd = pmd_mkclean(orig_pmd);
> > > 
> > $ grep -n set_pmd_at  linux-4.10/arch/powerpc/mm/pgtable-book3s64.c
> > 
> > /*
> >  * set a new huge pmd. We should not be called for updating
> >  * an existing pmd entry. That should go via pmd_hugepage_update.
> >  */
> > void set_pmd_at(struct mm_struct *mm, unsigned long addr,
> 
> +Aneesh.
> 
> Urgh... Power is special again.
> 
> I think this should work fine.
> 
> From 056914fa025992c0a2212aee057c26307ce60238 Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Date: Thu, 2 Mar 2017 16:47:45 +0300
> Subject: [PATCH] thp: fix MADV_DONTNEED vs. MADV_FREE race
> 
> Basically the same race as with numa balancing in change_huge_pmd(), but
> a bit simpler to mitigate: we don't need to preserve dirty/young flags
> here due to MADV_FREE functionality.

Could you elaborate a bit more here rather than relying on other
patch's description?

And could you say what happens to the userspace if that race
happens? When I guess from title "MADV_DONTNEED vs MADV_FREE",
a page cannot be zapped but marked lazyfree or vise versa? Right?

Thanks.

> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Minchan Kim <minchan@kernel.org>
> ---
>  mm/huge_memory.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index bb2b3646bd78..23c1b3d58cf4 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1566,8 +1566,7 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
>  		deactivate_page(page);
>  
>  	if (pmd_young(orig_pmd) || pmd_dirty(orig_pmd)) {
> -		orig_pmd = pmdp_huge_get_and_clear_full(tlb->mm, addr, pmd,
> -			tlb->fullmm);
> +		pmdp_invalidate(vma, addr, pmd);
>  		orig_pmd = pmd_mkold(orig_pmd);
>  		orig_pmd = pmd_mkclean(orig_pmd);
>  
> -- 
>  Kirill A. Shutemov
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[toc] | [prev] | [next] | [standalone]

#1594286 — Re: [PATCH 3/4] thp: fix MADV_DONTNEED vs. MADV_FREE race

On Mon, Mar 06, 2017 at 10:44:46AM +0900, Minchan Kim wrote:
> Hello, Kirill,
> 
> On Fri, Mar 03, 2017 at 01:26:36PM +0300, Kirill A. Shutemov wrote:
> > On Fri, Mar 03, 2017 at 01:35:11PM +0800, Hillf Danton wrote:
> > > 
> > > On March 02, 2017 11:11 PM Kirill A. Shutemov wrote: 
> > > > 
> > > > Basically the same race as with numa balancing in change_huge_pmd(), but
> > > > a bit simpler to mitigate: we don't need to preserve dirty/young flags
> > > > here due to MADV_FREE functionality.
> > > > 
> > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > > Cc: Minchan Kim <minchan@kernel.org>
> > > > ---
> > > >  mm/huge_memory.c | 2 --
> > > >  1 file changed, 2 deletions(-)
> > > > 
> > > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > > > index bb2b3646bd78..324217c31ec9 100644
> > > > --- a/mm/huge_memory.c
> > > > +++ b/mm/huge_memory.c
> > > > @@ -1566,8 +1566,6 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
> > > >  		deactivate_page(page);
> > > > 
> > > >  	if (pmd_young(orig_pmd) || pmd_dirty(orig_pmd)) {
> > > > -		orig_pmd = pmdp_huge_get_and_clear_full(tlb->mm, addr, pmd,
> > > > -			tlb->fullmm);
> > > >  		orig_pmd = pmd_mkold(orig_pmd);
> > > >  		orig_pmd = pmd_mkclean(orig_pmd);
> > > > 
> > > $ grep -n set_pmd_at  linux-4.10/arch/powerpc/mm/pgtable-book3s64.c
> > > 
> > > /*
> > >  * set a new huge pmd. We should not be called for updating
> > >  * an existing pmd entry. That should go via pmd_hugepage_update.
> > >  */
> > > void set_pmd_at(struct mm_struct *mm, unsigned long addr,
> > 
> > +Aneesh.
> > 
> > Urgh... Power is special again.
> > 
> > I think this should work fine.
> > 
> > From 056914fa025992c0a2212aee057c26307ce60238 Mon Sep 17 00:00:00 2001
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > Date: Thu, 2 Mar 2017 16:47:45 +0300
> > Subject: [PATCH] thp: fix MADV_DONTNEED vs. MADV_FREE race
> > 
> > Basically the same race as with numa balancing in change_huge_pmd(), but
> > a bit simpler to mitigate: we don't need to preserve dirty/young flags
> > here due to MADV_FREE functionality.
> 
> Could you elaborate a bit more here rather than relying on other
> patch's description?

Okay, updated patch is below.

> And could you say what happens to the userspace if that race
> happens? When I guess from title "MADV_DONTNEED vs MADV_FREE",
> a page cannot be zapped but marked lazyfree or vise versa? Right?

"Vise versa" part should be fine. The case I'm worry about is that
MADV_DONTNEED would skip the pmd and it will not be cleared.
Userspace expects the area of memory to be clean after MADV_DONTNEED, but
it's not. It can lead to userspace misbehaviour.

From a0967b0293a6f8053d85785c4d6340e550e849ea Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Date: Thu, 2 Mar 2017 16:47:45 +0300
Subject: [PATCH] thp: fix MADV_DONTNEED vs. MADV_FREE race

Both MADV_DONTNEED and MADV_FREE handled with down_read(mmap_sem).
It's critical to not clear pmd intermittently while handling MADV_FREE to
avoid race with MADV_DONTNEED:

	CPU0:				CPU1:
				madvise_free_huge_pmd()
				 pmdp_huge_get_and_clear_full()
madvise_dontneed()
 zap_pmd_range()
  pmd_trans_huge(*pmd) == 0 (without ptl)
  // skip the pmd
				 set_pmd_at();
				 // pmd is re-established

It results in MADV_DONTNEED skipping the pmd, leaving it not cleared. It
violates MADV_DONTNEED interface and can result is userspace misbehaviour.

Basically it's the same race as with numa balancing in change_huge_pmd(),
but a bit simpler to mitigate: we don't need to preserve dirty/young flags
here due to MADV_FREE functionality.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Minchan Kim <minchan@kernel.org>
---
 mm/huge_memory.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 51a8c376d020..3c9ef1104d85 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1568,8 +1568,7 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
 		deactivate_page(page);
 
 	if (pmd_young(orig_pmd) || pmd_dirty(orig_pmd)) {
-		orig_pmd = pmdp_huge_get_and_clear_full(tlb->mm, addr, pmd,
-			tlb->fullmm);
+		pmdp_invalidate(vma, addr, pmd);
 		orig_pmd = pmd_mkold(orig_pmd);
 		orig_pmd = pmd_mkclean(orig_pmd);
 
-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1592900 — Re: [PATCH 3/4] thp: fix MADV_DONTNEED vs. MADV_FREE race

"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:

> Basically the same race as with numa balancing in change_huge_pmd(), but
> a bit simpler to mitigate: we don't need to preserve dirty/young flags
> here due to MADV_FREE functionality.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Minchan Kim <minchan@kernel.org>
> ---
>  mm/huge_memory.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index bb2b3646bd78..324217c31ec9 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1566,8 +1566,6 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
>  		deactivate_page(page);
>  
>  	if (pmd_young(orig_pmd) || pmd_dirty(orig_pmd)) {
> -		orig_pmd = pmdp_huge_get_and_clear_full(tlb->mm, addr, pmd,
> -			tlb->fullmm);
>  		orig_pmd = pmd_mkold(orig_pmd);
>  		orig_pmd = pmd_mkclean(orig_pmd);
>  

Instead can we do a new interface that does something like

pmdp_huge_update(tlb->mm, addr, pmd, new_pmd);

We do have a variant already in ptep_set_access_flags. What we need is
something that can be used to update THP pmd, without converting it to
pmd_none and one which doens't loose reference and change bit ?

-aneesh

[toc] | [prev] | [next] | [standalone]

#1594303 — Re: [PATCH 3/4] thp: fix MADV_DONTNEED vs. MADV_FREE race

On Mon, Mar 06, 2017 at 08:19:03AM +0530, Aneesh Kumar K.V wrote:
> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:
> 
> > Basically the same race as with numa balancing in change_huge_pmd(), but
> > a bit simpler to mitigate: we don't need to preserve dirty/young flags
> > here due to MADV_FREE functionality.
> >
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Cc: Minchan Kim <minchan@kernel.org>
> > ---
> >  mm/huge_memory.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index bb2b3646bd78..324217c31ec9 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -1566,8 +1566,6 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
> >  		deactivate_page(page);
> >  
> >  	if (pmd_young(orig_pmd) || pmd_dirty(orig_pmd)) {
> > -		orig_pmd = pmdp_huge_get_and_clear_full(tlb->mm, addr, pmd,
> > -			tlb->fullmm);
> >  		orig_pmd = pmd_mkold(orig_pmd);
> >  		orig_pmd = pmd_mkclean(orig_pmd);
> >  
> 
> Instead can we do a new interface that does something like
> 
> pmdp_huge_update(tlb->mm, addr, pmd, new_pmd);
> 
> We do have a variant already in ptep_set_access_flags. What we need is
> something that can be used to update THP pmd, without converting it to
> pmd_none and one which doens't loose reference and change bit ?

Sounds like a good idea. Would you volunteer to implement it?
I don't have time for this right now.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1591227 — [PATCH 1/4] thp: reduce indentation level in change_huge_pmd()

Restructure code in preparation for a fix.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 mm/huge_memory.c | 52 ++++++++++++++++++++++++++--------------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 71e3dede95b4..e7ce73b2b208 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1722,37 +1722,37 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
 {
 	struct mm_struct *mm = vma->vm_mm;
 	spinlock_t *ptl;
-	int ret = 0;
+	pmd_t entry;
+	bool preserve_write;
+	int ret;
 
 	ptl = __pmd_trans_huge_lock(pmd, vma);
-	if (ptl) {
-		pmd_t entry;
-		bool preserve_write = prot_numa && pmd_write(*pmd);
-		ret = 1;
+	if (!ptl)
+		return 0;
 
-		/*
-		 * Avoid trapping faults against the zero page. The read-only
-		 * data is likely to be read-cached on the local CPU and
-		 * local/remote hits to the zero page are not interesting.
-		 */
-		if (prot_numa && is_huge_zero_pmd(*pmd)) {
-			spin_unlock(ptl);
-			return ret;
-		}
+	preserve_write = prot_numa && pmd_write(*pmd);
+	ret = 1;
 
-		if (!prot_numa || !pmd_protnone(*pmd)) {
-			entry = pmdp_huge_get_and_clear_notify(mm, addr, pmd);
-			entry = pmd_modify(entry, newprot);
-			if (preserve_write)
-				entry = pmd_mk_savedwrite(entry);
-			ret = HPAGE_PMD_NR;
-			set_pmd_at(mm, addr, pmd, entry);
-			BUG_ON(vma_is_anonymous(vma) && !preserve_write &&
-					pmd_write(entry));
-		}
-		spin_unlock(ptl);
-	}
+	/*
+	 * Avoid trapping faults against the zero page. The read-only
+	 * data is likely to be read-cached on the local CPU and
+	 * local/remote hits to the zero page are not interesting.
+	 */
+	if (prot_numa && is_huge_zero_pmd(*pmd))
+		goto unlock;
 
+	if (prot_numa && pmd_protnone(*pmd))
+		goto unlock;
+
+	entry = pmdp_huge_get_and_clear_notify(mm, addr, pmd);
+	entry = pmd_modify(entry, newprot);
+	if (preserve_write)
+		entry = pmd_mk_savedwrite(entry);
+	ret = HPAGE_PMD_NR;
+	set_pmd_at(mm, addr, pmd, entry);
+	BUG_ON(vma_is_anonymous(vma) && !preserve_write && pmd_write(entry));
+unlock:
+	spin_unlock(ptl);
 	return ret;
 }
 
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1591627 — [PATCH 2/4] thp: fix MADV_DONTNEED vs. numa balancing race

In case prot_numa, we are under down_read(mmap_sem). It's critical
to not clear pmd intermittently to avoid race with MADV_DONTNEED
which is also under down_read(mmap_sem):

	CPU0:				CPU1:
				change_huge_pmd(prot_numa=1)
				 pmdp_huge_get_and_clear_notify()
madvise_dontneed()
 zap_pmd_range()
  pmd_trans_huge(*pmd) == 0 (without ptl)
  // skip the pmd
				 set_pmd_at();
				 // pmd is re-established

The race makes MADV_DONTNEED miss the huge pmd and don't clear it
which may break userspace.

Found by code analysis, never saw triggered.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 mm/huge_memory.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index e7ce73b2b208..bb2b3646bd78 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1744,7 +1744,39 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
 	if (prot_numa && pmd_protnone(*pmd))
 		goto unlock;
 
-	entry = pmdp_huge_get_and_clear_notify(mm, addr, pmd);
+	/*
+	 * In case prot_numa, we are under down_read(mmap_sem). It's critical
+	 * to not clear pmd intermittently to avoid race with MADV_DONTNEED
+	 * which is also under down_read(mmap_sem):
+	 *
+	 *	CPU0:				CPU1:
+	 *				change_huge_pmd(prot_numa=1)
+	 *				 pmdp_huge_get_and_clear_notify()
+	 * madvise_dontneed()
+	 *  zap_pmd_range()
+	 *   pmd_trans_huge(*pmd) == 0 (without ptl)
+	 *   // skip the pmd
+	 *				 set_pmd_at();
+	 *				 // pmd is re-established
+	 *
+	 * The race makes MADV_DONTNEED miss the huge pmd and don't clear it
+	 * which may break userspace.
+	 *
+	 * pmdp_invalidate() is required to make sure we don't miss
+	 * dirty/young flags set by hardware.
+	 */
+	entry = *pmd;
+	pmdp_invalidate(vma, addr, pmd);
+
+	/*
+	 * Recover dirty/young flags.  It relies on pmdp_invalidate to not
+	 * corrupt them.
+	 */
+	if (pmd_dirty(*pmd))
+		entry = pmd_mkdirty(entry);
+	if (pmd_young(*pmd))
+		entry = pmd_mkyoung(entry);
+
 	entry = pmd_modify(entry, newprot);
 	if (preserve_write)
 		entry = pmd_mk_savedwrite(entry);
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1592153 — Re: [PATCH 2/4] thp: fix MADV_DONTNEED vs. numa balancing race

On 03/02/2017 07:10 AM, Kirill A. Shutemov wrote:
> @@ -1744,7 +1744,39 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>  	if (prot_numa && pmd_protnone(*pmd))
>  		goto unlock;
>  
> -	entry = pmdp_huge_get_and_clear_notify(mm, addr, pmd);

Are there any remaining call sites for pmdp_huge_get_and_clear_notify()
after this?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web