Re: [PATCH] mm: fix page leak with multiple threads mapping the same page

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 06 Jul 2022 20:42:56 -0400 Rik van Riel <riel@surriel.com> wrote:

> On Thu, 2022-07-07 at 01:46 +0300, Kirill A. Shutemov wrote:
> > On Tue, Jul 05, 2022 at 04:00:36PM -0400, Josef Bacik wrote:
> > > 
> > > Fix this by returning VM_FAULT_NOPAGE in the
> > > pmd_devmap_trans_unstable()
> > > case, this makes us drop the ref on the page properly, and now my
> > > reproducer no longer leaks the huge pages.
> > > 
> > > Fixes: f9ce0be71d1f ("mm: Cleanup faultaround and finish_fault()
> > > codepaths")
> > > Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
> > > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > > Signed-off-by: Rik van Riel <riel@surriel.com>
> > > Signed-off-by: Chris Mason <clm@fb.com>
> > 
> > Cc: stable@ 
> 
> Yes, it should.  How do we send a patch to stable@
> after the start of the thread?

cc'ing the stable list doesn't have much affect, afaik.

What Kirill means is that we should add cc:stable to this patch's
changelog.  I did that yesterday.

> 
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -4371,7 +4371,7 @@ vm_fault_t finish_fault(struct vm_fault *vmf)
> > >  
> > >         /* See comment in handle_pte_fault() */
> > >         if (pmd_devmap_trans_unstable(vmf->pmd))
> > > -               return 0;
> > > +               return VM_FAULT_NOPAGE;
> > 
> > Comment update would be nice.
> > 
> > Other instances of pmd_devmap_trans_unstable() return 0 in the fault
> > path.
> > Explanation would be helpful.
> > 
> The explanation is that by the time we get to
> finish_fault, we already have a reference on a
> page, and we need to ensure that reference
> gets released by the caller.
> 
> VM_FAULT_NOPAGE is one of the ways to indicate
> that the page should be freed.

I think Kirill meant this should be added to the code comment!