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

[Rik van Riel <riel@surriel.com>]

[-- Attachment #1: Type: text/plain, Size: 1602 bytes --]

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?


> > --- 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.

-- 
All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]