| Message ID | 0-v1-976effcd4468+d4-gup_cma_fix_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [rc] mm/gup: use unpin_user_pages() in check_and_migrate_cma_pages() | expand |
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 > --- a/mm/gup.c > +++ 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. 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 } We should change that site as well. And since we now have 2 places should this be a helper? Ira > - for (i = 0; i < nr_pages; i++) > - put_page(pages[i]); > + if (gup_flags & FOLL_PIN) > + unpin_user_pages(pages, nr_pages); > + else > + for (i = 0; i < nr_pages; i++) > + put_page(pages[i]); > > if (migrate_pages(&cma_page_list, alloc_migration_target, NULL, > (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE)) { > -- > 2.28.0 >
On 10/30/20 1:50 PM, 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 >> --- a/mm/gup.c >> +++ 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. Modify, maybe. But it's still relevant: the pages came from either gup or pup. > > 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 } > > We should change that site as well. > > And since we now have 2 places should this be a helper? > +1 thanks,
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 Thus for this stable patch let's just leave it simple? Thanks, Jason
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
diff --git a/mm/gup.c b/mm/gup.c index 1bb349e5ed212a..2e26757f3c9276 100644 --- a/mm/gup.c +++ 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. */ - for (i = 0; i < nr_pages; i++) - put_page(pages[i]); + if (gup_flags & FOLL_PIN) + unpin_user_pages(pages, nr_pages); + else + for (i = 0; i < nr_pages; i++) + put_page(pages[i]); if (migrate_pages(&cma_page_list, alloc_migration_target, NULL, (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE)) {
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.