[PATCH] mm: migrate: fix missing update page_private to hugetlb_page_subpool

[PATCH] mm: migrate: fix missing update page_private to hugetlb_page_subpool

[Muchun Song]

Since commit d6995da31122 ("hugetlb: use page.private for hugetlb specific
page flags") converts page.private for hugetlb specific page flags. We
should use hugetlb_page_subpool() to get the subpool pointer instead of
page_private(). The commit forgot to update it in the page migration
routine. So fix it.

Fixes: d6995da31122 ("hugetlb: use page.private for hugetlb specific page flags")
Reported-by: Anshuman Khandual <anshuman.khandual@arm.com>
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/migrate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index e7a173da74ec..43419c4bb097 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1290,7 +1290,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
 	 * page_mapping() set, hugetlbfs specific move page routine will not
 	 * be called and we could leak usage counts for subpools.
 	 */
-	if (page_private(hpage) && !page_mapping(hpage)) {
+	if (hugetlb_page_subpool(hpage) && !page_mapping(hpage)) {
 		rc = -EBUSY;
 		goto out_unlock;
 	}
-- 
2.11.0

Re: [PATCH] mm: migrate: fix missing update page_private to hugetlb_page_subpool

[Mike Kravetz]

On 5/19/21 7:59 PM, Muchun Song wrote:
> Since commit d6995da31122 ("hugetlb: use page.private for hugetlb specific
> page flags") converts page.private for hugetlb specific page flags. We
> should use hugetlb_page_subpool() to get the subpool pointer instead of
> page_private(). The commit forgot to update it in the page migration
> routine. So fix it.
> 
> Fixes: d6995da31122 ("hugetlb: use page.private for hugetlb specific page flags")
> Reported-by: Anshuman Khandual <anshuman.khandual@arm.com>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/migrate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thank you Muchun!

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
-- 
Mike Kravetz

Re: [PATCH] mm: migrate: fix missing update page_private to hugetlb_page_subpool

[Andrew Morton]

On Thu, 20 May 2021 10:59:49 +0800 Muchun Song <songmuchun@bytedance.com> wrote:

> Since commit d6995da31122 ("hugetlb: use page.private for hugetlb specific
> page flags") converts page.private for hugetlb specific page flags. We
> should use hugetlb_page_subpool() to get the subpool pointer instead of
> page_private(). The commit forgot to update it in the page migration
> routine. So fix it.
> 
> ...
>
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1290,7 +1290,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
>  	 * page_mapping() set, hugetlbfs specific move page routine will not
>  	 * be called and we could leak usage counts for subpools.
>  	 */
> -	if (page_private(hpage) && !page_mapping(hpage)) {
> +	if (hugetlb_page_subpool(hpage) && !page_mapping(hpage)) {
>  		rc = -EBUSY;
>  		goto out_unlock;
>  	}

So it uses the wrong page*, so this isn't just a cosmetic fix.  One
cannot tell from this changelog.

Please describe the runtime effects of this bug.  Please always include
this information when fixing bugs.  And when adding them.

Re: [External] Re: [PATCH] mm: migrate: fix missing update page_private to hugetlb_page_subpool

[Muchun Song]

On Thu, May 20, 2021 at 12:24 PM Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Thu, 20 May 2021 10:59:49 +0800 Muchun Song <songmuchun@bytedance.com> wrote:
>
> > Since commit d6995da31122 ("hugetlb: use page.private for hugetlb specific
> > page flags") converts page.private for hugetlb specific page flags. We
> > should use hugetlb_page_subpool() to get the subpool pointer instead of
> > page_private(). The commit forgot to update it in the page migration
> > routine. So fix it.
> >
> > ...
> >
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -1290,7 +1290,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
> >        * page_mapping() set, hugetlbfs specific move page routine will not
> >        * be called and we could leak usage counts for subpools.
> >        */
> > -     if (page_private(hpage) && !page_mapping(hpage)) {
> > +     if (hugetlb_page_subpool(hpage) && !page_mapping(hpage)) {
> >               rc = -EBUSY;
> >               goto out_unlock;
> >       }
>
> So it uses the wrong page*, so this isn't just a cosmetic fix.  One
> cannot tell from this changelog.
>
> Please describe the runtime effects of this bug.  Please always include
> this information when fixing bugs.  And when adding them.

OK. I should update the commit log in the next version.

Thanks.

Re: [External] Re: [PATCH] mm: migrate: fix missing update page_private to hugetlb_page_subpool

[Muchun Song]

On Thu, May 20, 2021 at 12:24 PM Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Thu, 20 May 2021 10:59:49 +0800 Muchun Song <songmuchun@bytedance.com> wrote:
>
> > Since commit d6995da31122 ("hugetlb: use page.private for hugetlb specific
> > page flags") converts page.private for hugetlb specific page flags. We
> > should use hugetlb_page_subpool() to get the subpool pointer instead of
> > page_private(). The commit forgot to update it in the page migration
> > routine. So fix it.
> >
> > ...
> >
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -1290,7 +1290,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
> >        * page_mapping() set, hugetlbfs specific move page routine will not
> >        * be called and we could leak usage counts for subpools.
> >        */
> > -     if (page_private(hpage) && !page_mapping(hpage)) {
> > +     if (hugetlb_page_subpool(hpage) && !page_mapping(hpage)) {
> >               rc = -EBUSY;
> >               goto out_unlock;
> >       }
>
> So it uses the wrong page*, so this isn't just a cosmetic fix.  One
> cannot tell from this changelog.
>
> Please describe the runtime effects of this bug.  Please always include
> this information when fixing bugs.  And when adding them.

OK. I should update the commit log in the next version.

Thanks.

Re: [External] Re: [PATCH] mm: migrate: fix missing update page_private to hugetlb_page_subpool

[Mike Kravetz]

On 5/19/21 10:13 PM, Muchun Song wrote:
> On Thu, May 20, 2021 at 12:24 PM Andrew Morton
> <akpm@linux-foundation.org> wrote:
>>
>> On Thu, 20 May 2021 10:59:49 +0800 Muchun Song <songmuchun@bytedance.com> wrote:
>>
>>> Since commit d6995da31122 ("hugetlb: use page.private for hugetlb specific
>>> page flags") converts page.private for hugetlb specific page flags. We
>>> should use hugetlb_page_subpool() to get the subpool pointer instead of
>>> page_private(). The commit forgot to update it in the page migration
>>> routine. So fix it.
>>>
>>> ...
>>>
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -1290,7 +1290,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
>>>        * page_mapping() set, hugetlbfs specific move page routine will not
>>>        * be called and we could leak usage counts for subpools.
>>>        */
>>> -     if (page_private(hpage) && !page_mapping(hpage)) {
>>> +     if (hugetlb_page_subpool(hpage) && !page_mapping(hpage)) {
>>>               rc = -EBUSY;
>>>               goto out_unlock;
>>>       }
>>
>> So it uses the wrong page*, so this isn't just a cosmetic fix.  One
>> cannot tell from this changelog.
>>
>> Please describe the runtime effects of this bug.  Please always include
>> this information when fixing bugs.  And when adding them.
> 
> OK. I should update the commit log in the next version.
> 

Unfortunately, it is more than a cosmetic fix.

This 'could' prevent the migration of hugetlb pages.  page_private(hpage)
is now used for hugetlb page specific flags.  At migration time, the
only flag which could be set is HPageVmemmapOptimized.  This flag will
only be set if the new vmemmap reduction feature is enabled.  In
addition, !page_mapping() implies an anonymous mapping.  So, this will
prevent migration of hugetb pages in anonymous mappings if the vmemmap
reduction feature is enabled.

In addition, that if statement checked for the rare race condition of a
page being migrated while in the process of being freed.  Since that
check is now wrong, we could leak hugetlb subpool usage counts.

Muchun, feel free to use as much of that as you would like in the
updated commit message.
-- 
Mike Kravetz

Re: [External] Re: [PATCH] mm: migrate: fix missing update page_private to hugetlb_page_subpool

[Muchun Song]

On Fri, May 21, 2021 at 2:17 AM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> On 5/19/21 10:13 PM, Muchun Song wrote:
> > On Thu, May 20, 2021 at 12:24 PM Andrew Morton
> > <akpm@linux-foundation.org> wrote:
> >>
> >> On Thu, 20 May 2021 10:59:49 +0800 Muchun Song <songmuchun@bytedance.com> wrote:
> >>
> >>> Since commit d6995da31122 ("hugetlb: use page.private for hugetlb specific
> >>> page flags") converts page.private for hugetlb specific page flags. We
> >>> should use hugetlb_page_subpool() to get the subpool pointer instead of
> >>> page_private(). The commit forgot to update it in the page migration
> >>> routine. So fix it.
> >>>
> >>> ...
> >>>
> >>> --- a/mm/migrate.c
> >>> +++ b/mm/migrate.c
> >>> @@ -1290,7 +1290,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
> >>>        * page_mapping() set, hugetlbfs specific move page routine will not
> >>>        * be called and we could leak usage counts for subpools.
> >>>        */
> >>> -     if (page_private(hpage) && !page_mapping(hpage)) {
> >>> +     if (hugetlb_page_subpool(hpage) && !page_mapping(hpage)) {
> >>>               rc = -EBUSY;
> >>>               goto out_unlock;
> >>>       }
> >>
> >> So it uses the wrong page*, so this isn't just a cosmetic fix.  One
> >> cannot tell from this changelog.
> >>
> >> Please describe the runtime effects of this bug.  Please always include
> >> this information when fixing bugs.  And when adding them.
> >
> > OK. I should update the commit log in the next version.
> >
>
> Unfortunately, it is more than a cosmetic fix.
>
> This 'could' prevent the migration of hugetlb pages.  page_private(hpage)
> is now used for hugetlb page specific flags.  At migration time, the
> only flag which could be set is HPageVmemmapOptimized.  This flag will
> only be set if the new vmemmap reduction feature is enabled.  In
> addition, !page_mapping() implies an anonymous mapping.  So, this will
> prevent migration of hugetb pages in anonymous mappings if the vmemmap
> reduction feature is enabled.
>
> In addition, that if statement checked for the rare race condition of a
> page being migrated while in the process of being freed.  Since that
> check is now wrong, we could leak hugetlb subpool usage counts.
>
> Muchun, feel free to use as much of that as you would like in the
> updated commit message.

Thanks Mike. I'll use this.

> --
> Mike Kravetz

Re: [External] Re: [PATCH] mm: migrate: fix missing update page_private to hugetlb_page_subpool

[Muchun Song]

On Fri, May 21, 2021 at 2:17 AM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> On 5/19/21 10:13 PM, Muchun Song wrote:
> > On Thu, May 20, 2021 at 12:24 PM Andrew Morton
> > <akpm@linux-foundation.org> wrote:
> >>
> >> On Thu, 20 May 2021 10:59:49 +0800 Muchun Song <songmuchun@bytedance.com> wrote:
> >>
> >>> Since commit d6995da31122 ("hugetlb: use page.private for hugetlb specific
> >>> page flags") converts page.private for hugetlb specific page flags. We
> >>> should use hugetlb_page_subpool() to get the subpool pointer instead of
> >>> page_private(). The commit forgot to update it in the page migration
> >>> routine. So fix it.
> >>>
> >>> ...
> >>>
> >>> --- a/mm/migrate.c
> >>> +++ b/mm/migrate.c
> >>> @@ -1290,7 +1290,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
> >>>        * page_mapping() set, hugetlbfs specific move page routine will not
> >>>        * be called and we could leak usage counts for subpools.
> >>>        */
> >>> -     if (page_private(hpage) && !page_mapping(hpage)) {
> >>> +     if (hugetlb_page_subpool(hpage) && !page_mapping(hpage)) {
> >>>               rc = -EBUSY;
> >>>               goto out_unlock;
> >>>       }
> >>
> >> So it uses the wrong page*, so this isn't just a cosmetic fix.  One
> >> cannot tell from this changelog.
> >>
> >> Please describe the runtime effects of this bug.  Please always include
> >> this information when fixing bugs.  And when adding them.
> >
> > OK. I should update the commit log in the next version.
> >
>
> Unfortunately, it is more than a cosmetic fix.
>
> This 'could' prevent the migration of hugetlb pages.  page_private(hpage)
> is now used for hugetlb page specific flags.  At migration time, the
> only flag which could be set is HPageVmemmapOptimized.  This flag will
> only be set if the new vmemmap reduction feature is enabled.  In
> addition, !page_mapping() implies an anonymous mapping.  So, this will
> prevent migration of hugetb pages in anonymous mappings if the vmemmap
> reduction feature is enabled.
>
> In addition, that if statement checked for the rare race condition of a
> page being migrated while in the process of being freed.  Since that
> check is now wrong, we could leak hugetlb subpool usage counts.
>
> Muchun, feel free to use as much of that as you would like in the
> updated commit message.

Thanks Mike. I'll use this.

> --
> Mike Kravetz