Re: [PATCH rc] mm/gup: use unpin_user_pages() in check_and_migrate_cma_pages()

[Ira Weiny <ira.weiny@intel.com>]

On Fri, Oct 30, 2020 at 08:58:25PM -0300, Jason Gunthorpe wrote:
> On Fri, Oct 30, 2020 at 01:50:26PM -0700, Ira Weiny wrote:
> > On Fri, Oct 30, 2020 at 01:32:28PM -0300, Jason Gunthorpe wrote:
> > > When FOLL_PIN is passed to __get_user_pages() the page list must be put
> > > back using unpin_user_pages() otherwise the page pin reference persists in
> > > a corrupted state.
> > > 
> > > Cc: <stable@kernel.org>
> > > Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> > > Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages")
> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > >  mm/gup.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > I don't have any way to test CMA stuff, this was noticed by inspection.
> > > 
> > > diff --git a/mm/gup.c b/mm/gup.c
> > > index 1bb349e5ed212a..2e26757f3c9276 100644
> > > +++ b/mm/gup.c
> > > @@ -1630,8 +1630,11 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm,
> > >  		/*
> > >  		 * drop the above get_user_pages reference.
> > >  		 */
> > 
> > I wonder if that comment should be deleted/modified?  It does not seem to apply
> > any longer.
> 
> It is still basically right, the 'above' is just a bit vauge.. Don't
> want to touch extra stuff for a stable patch.
> 
> > Also, looks like there is another place this occurs right before the call to
> > check_and_migrate_cma_pages() in __gup_longterm_locked()
> > 
> > 1730                 if (check_dax_vmas(vmas_tmp, rc)) {
> > 1731                         for (i = 0; i < rc; i++)
> > 1732                                 put_page(pages[i]);
> > 1733                         rc = -EOPNOTSUPP;
> > 1734                         goto out;
> > 1735                 }
> 
> Oh, yes! I will update this in v2 - good eyes
>  
> > And since we now have 2 places should this be a helper?
> 
> I have another patch that deletes check_dax_vmas() and this code
> 
> https://github.com/jgunthorpe/linux/commit/48ee608271e124e4a89353b9694502372c1b2df0
> 
> Does it look OK to you? I was going to check it again and post next
> week

I think it is ok...  This code has gotten twisted a bit.  I wonder if
__get_user_pages_remote() could also be cleaned up after this patch?

Also, __gup_longterm_locked() seems a bit of an odd name after that change.
However, I don't have a better name so I think it is fine to leave it for now.
It seems like the CMA 'longterm' checks are more about pinning but I don't
recall the details ATM.

> 
> Thus for this stable patch let's just leave it simple?

Yea good for stable!

Ira

> 
> Thanks,
> Jason