[PATCH] mm, compaction: fast_find_migrateblock() should return pfn in the target zone

[PATCH] mm, compaction: fast_find_migrateblock() should return pfn in the target zone

[Rei Yamamoto]

Prevent returning a pfn outside the target zone in case that not
aligned with pageblock boundary.
Otherwise isolate_migratepages_block() would handle pages not in
the target zone.

Signed-off-by: Rei Yamamoto <yamamoto.rei@jp.fujitsu.com>
---
 mm/compaction.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/compaction.c b/mm/compaction.c
index fe915db6149b..de42b8e48758 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1858,6 +1858,8 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
 
 				update_fast_start_pfn(cc, free_pfn);
 				pfn = pageblock_start_pfn(free_pfn);
+				if (pfn < cc->zone->zone_start_pfn)
+					pfn = cc->zone->zone_start_pfn;
 				cc->fast_search_fail = 0;
 				found_block = true;
 				set_pageblock_skip(freepage);
-- 
2.27.0

Re: [PATCH] mm, compaction: fast_find_migrateblock() should return pfn in the target zone

[Miaohe Lin]

On 2022/5/11 12:43, Rei Yamamoto wrote:
> Prevent returning a pfn outside the target zone in case that not
> aligned with pageblock boundary.
> Otherwise isolate_migratepages_block() would handle pages not in
> the target zone.
> 

IIUC, the sole caller isolate_migratepages will ensure the pfn won't outside
the target zone. So the below code change might not be necessary. Or am I miss
something ?

Thanks!

> Signed-off-by: Rei Yamamoto <yamamoto.rei@jp.fujitsu.com>
> ---
>  mm/compaction.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index fe915db6149b..de42b8e48758 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1858,6 +1858,8 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
>  
>  				update_fast_start_pfn(cc, free_pfn);
>  				pfn = pageblock_start_pfn(free_pfn);
> +				if (pfn < cc->zone->zone_start_pfn)
> +					pfn = cc->zone->zone_start_pfn;
>  				cc->fast_search_fail = 0;
>  				found_block = true;
>  				set_pageblock_skip(freepage);
> 

Re: [PATCH] mm, compaction: fast_find_migrateblock() should return pfn in the target zone

[Rei Yamamoto]

On Wed, 11 May 2022 14:25:34 Miaohe Lin wrote:
> On 2022/5/11 12:43, Rei Yamamoto wrote:
>> Prevent returning a pfn outside the target zone in case that not
>> aligned with pageblock boundary.
>> Otherwise isolate_migratepages_block() would handle pages not in
>> the target zone.
>> 
>
> IIUC, the sole caller isolate_migratepages will ensure the pfn won't outside
> the target zone. So the below code change might not be necessary. Or am I miss
> something ?

While block_start_pfn is ensured, this variable is not used as the argument for 
isolate_migratepages_block():
  -----
  static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
  {
  :
          low_pfn = fast_find_migrateblock(cc);
          block_start_pfn = pageblock_start_pfn(low_pfn);
          if (block_start_pfn < cc->zone->zone_start_pfn)
                  block_start_pfn = cc->zone->zone_start_pfn;  <--- block_start_pfn is ensured not outside 
                                                                    the target zone
  :
          block_end_pfn = pageblock_end_pfn(low_pfn);
  :
          for (; block_end_pfn <= cc->free_pfn;
                          fast_find_block = false,
                          cc->migrate_pfn = low_pfn = block_end_pfn,
                          block_start_pfn = block_end_pfn,
                          block_end_pfn += pageblock_nr_pages) {
  :
                  if (isolate_migratepages_block(cc, low_pfn, block_end_pfn,  <--- low_pfn is passed as 
                                                                                   the argument
                                                  isolate_mode))
                          return ISOLATE_ABORT;
  -----

So, the low_pfn passed to isolate_migratepages_block() can be outside the target zone.

Thanks,
Rei

Re: [PATCH] mm, compaction: fast_find_migrateblock() should return pfn in the target zone

[Miaohe Lin]

On 2022/5/11 15:07, Rei Yamamoto wrote:
> On Wed, 11 May 2022 14:25:34 Miaohe Lin wrote:
>> On 2022/5/11 12:43, Rei Yamamoto wrote:
>>> Prevent returning a pfn outside the target zone in case that not
>>> aligned with pageblock boundary.
>>> Otherwise isolate_migratepages_block() would handle pages not in
>>> the target zone.
>>>
>>
>> IIUC, the sole caller isolate_migratepages will ensure the pfn won't outside
>> the target zone. So the below code change might not be necessary. Or am I miss
>> something ?
> 
> While block_start_pfn is ensured, this variable is not used as the argument for 
> isolate_migratepages_block():
>   -----
>   static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
>   {
>   :
>           low_pfn = fast_find_migrateblock(cc);
>           block_start_pfn = pageblock_start_pfn(low_pfn);
>           if (block_start_pfn < cc->zone->zone_start_pfn)
>                   block_start_pfn = cc->zone->zone_start_pfn;  <--- block_start_pfn is ensured not outside 
>                                                                     the target zone
>   :
>           block_end_pfn = pageblock_end_pfn(low_pfn);
>   :
>           for (; block_end_pfn <= cc->free_pfn;
>                           fast_find_block = false,
>                           cc->migrate_pfn = low_pfn = block_end_pfn,
>                           block_start_pfn = block_end_pfn,
>                           block_end_pfn += pageblock_nr_pages) {
>   :
>                   if (isolate_migratepages_block(cc, low_pfn, block_end_pfn,  <--- low_pfn is passed as 
>                                                                                    the argument

Sorry, I think you're right. And could you please add the runtime effect of this issue?

Anyway, this patch looks good to me now. Thanks!

Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>

>                                                   isolate_mode))
>                           return ISOLATE_ABORT;
>   -----
> 
> So, the low_pfn passed to isolate_migratepages_block() can be outside the target zone.
> 
> Thanks,
> Rei
> .
> 

Re: [PATCH] mm, compaction: fast_find_migrateblock() should return pfn in the target zone

[Rei Yamamoto]

On Wed, 11 May 2022 17:26:16 Miaohe Lin wrote:
> On 2022/5/11 15:07, Rei Yamamoto wrote:
>> On Wed, 11 May 2022 14:25:34 Miaohe Lin wrote:
>>> On 2022/5/11 12:43, Rei Yamamoto wrote:
>>>> Prevent returning a pfn outside the target zone in case that not
>>>> aligned with pageblock boundary.
>>>> Otherwise isolate_migratepages_block() would handle pages not in
>>>> the target zone.
>>>>
>>>
>>> IIUC, the sole caller isolate_migratepages will ensure the pfn won't outside
>>> the target zone. So the below code change might not be necessary. Or am I miss
>>> something ?
>> 
>> While block_start_pfn is ensured, this variable is not used as the argument for 
>> isolate_migratepages_block():
>>   -----
>>   static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
>>   {
>>   :
>>           low_pfn = fast_find_migrateblock(cc);
>>           block_start_pfn = pageblock_start_pfn(low_pfn);
>>           if (block_start_pfn < cc->zone->zone_start_pfn)
>>                   block_start_pfn = cc->zone->zone_start_pfn;  <--- block_start_pfn is ensured not outside 
>>                                                                     the target zone
>>   :
>>           block_end_pfn = pageblock_end_pfn(low_pfn);
>>   :
>>           for (; block_end_pfn <= cc->free_pfn;
>>                           fast_find_block = false,
>>                           cc->migrate_pfn = low_pfn = block_end_pfn,
>>                           block_start_pfn = block_end_pfn,
>>                           block_end_pfn += pageblock_nr_pages) {
>>   :
>>                   if (isolate_migratepages_block(cc, low_pfn, block_end_pfn,  <--- low_pfn is passed as 
>>                                                                                    the argument
>
> Sorry, I think you're right. And could you please add the runtime effect of this issue?
>
> Anyway, this patch looks good to me now. Thanks!

Thank you for your review.
The runtime effect is that compaction become unintended behavior.
For example, pages not in the target zone are added to cc->migratepages list in isolate_migratepages_block().
As a result, pages migrate between nodes unintentionally.

Thanks,
Rei

Re: [PATCH] mm, compaction: fast_find_migrateblock() should return pfn in the target zone

[Andrew Morton]

On Thu, 12 May 2022 10:47:36 +0900 Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> wrote:

> ...
>
> > Sorry, I think you're right. And could you please add the runtime effect of this issue?
> >
> > Anyway, this patch looks good to me now. Thanks!
> 
> Thank you for your review.
> The runtime effect is that compaction become unintended behavior.
> For example, pages not in the target zone are added to cc->migratepages list in isolate_migratepages_block().
> As a result, pages migrate between nodes unintentionally.

Thanks.   I updated the changelog thusly:

: At present, pages not in the target zone are added to cc->migratepages
: list in isolate_migratepages_block().  As a result, pages may migrate
: between nodes unintentionally.
: 
: Avoid returning a pfn outside the target zone in the case that it is
: not aligned with a pageblock boundary.  Otherwise
: isolate_migratepages_block() will handle pages not in the target zone.

Re: [PATCH] mm, compaction: fast_find_migrateblock() should return pfn in the target zone

[Miaohe Lin]

On 2022/5/12 9:47, Rei Yamamoto wrote:
> On Wed, 11 May 2022 17:26:16 Miaohe Lin wrote:
>> On 2022/5/11 15:07, Rei Yamamoto wrote:
>>> On Wed, 11 May 2022 14:25:34 Miaohe Lin wrote:
>>>> On 2022/5/11 12:43, Rei Yamamoto wrote:
>>>>> Prevent returning a pfn outside the target zone in case that not
>>>>> aligned with pageblock boundary.
>>>>> Otherwise isolate_migratepages_block() would handle pages not in
>>>>> the target zone.
>>>>>
>>>>
>>>> IIUC, the sole caller isolate_migratepages will ensure the pfn won't outside
>>>> the target zone. So the below code change might not be necessary. Or am I miss
>>>> something ?
>>>
>>> While block_start_pfn is ensured, this variable is not used as the argument for 
>>> isolate_migratepages_block():
>>>   -----
>>>   static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
>>>   {
>>>   :
>>>           low_pfn = fast_find_migrateblock(cc);
>>>           block_start_pfn = pageblock_start_pfn(low_pfn);
>>>           if (block_start_pfn < cc->zone->zone_start_pfn)
>>>                   block_start_pfn = cc->zone->zone_start_pfn;  <--- block_start_pfn is ensured not outside 
>>>                                                                     the target zone
>>>   :
>>>           block_end_pfn = pageblock_end_pfn(low_pfn);
>>>   :
>>>           for (; block_end_pfn <= cc->free_pfn;
>>>                           fast_find_block = false,
>>>                           cc->migrate_pfn = low_pfn = block_end_pfn,
>>>                           block_start_pfn = block_end_pfn,
>>>                           block_end_pfn += pageblock_nr_pages) {
>>>   :
>>>                   if (isolate_migratepages_block(cc, low_pfn, block_end_pfn,  <--- low_pfn is passed as 
>>>                                                                                    the argument
>>
>> Sorry, I think you're right. And could you please add the runtime effect of this issue?
>>
>> Anyway, this patch looks good to me now. Thanks!
> 
> Thank you for your review.
> The runtime effect is that compaction become unintended behavior.
> For example, pages not in the target zone are added to cc->migratepages list in isolate_migratepages_block().
> As a result, pages migrate between nodes unintentionally.

Many thanks for clarifying. :) Is this worth a Fixes tag or even CC stable?

Thanks!

> 
> Thanks,
> Rei
> .
> 

Re: [PATCH] mm, compaction: fast_find_migrateblock() should return pfn in the target zone

[Rei Yamamoto]

On Thu, 12 May 2022 10:27:44 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote:
> On 2022/5/12 9:47, Rei Yamamoto wrote:
>> On Wed, 11 May 2022 17:26:16 Miaohe Lin wrote:
>>> On 2022/5/11 15:07, Rei Yamamoto wrote:
>>>> On Wed, 11 May 2022 14:25:34 Miaohe Lin wrote:
>>>>> On 2022/5/11 12:43, Rei Yamamoto wrote:
>>>>>> Prevent returning a pfn outside the target zone in case that not
>>>>>> aligned with pageblock boundary.
>>>>>> Otherwise isolate_migratepages_block() would handle pages not in
>>>>>> the target zone.
>>>>>>
>>>>>
>>>>> IIUC, the sole caller isolate_migratepages will ensure the pfn won't outside
>>>>> the target zone. So the below code change might not be necessary. Or am I miss
>>>>> something ?
>>>>
>>>> While block_start_pfn is ensured, this variable is not used as the argument for 
>>>> isolate_migratepages_block():
>>>>   -----
>>>>   static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
>>>>   {
>>>>   :
>>>>           low_pfn = fast_find_migrateblock(cc);
>>>>           block_start_pfn = pageblock_start_pfn(low_pfn);
>>>>           if (block_start_pfn < cc->zone->zone_start_pfn)
>>>>                   block_start_pfn = cc->zone->zone_start_pfn;  <--- block_start_pfn is ensured not outside 
>>>>                                                                     the target zone
>>>>   :
>>>>           block_end_pfn = pageblock_end_pfn(low_pfn);
>>>>   :
>>>>           for (; block_end_pfn <= cc->free_pfn;
>>>>                           fast_find_block = false,
>>>>                           cc->migrate_pfn = low_pfn = block_end_pfn,
>>>>                           block_start_pfn = block_end_pfn,
>>>>                           block_end_pfn += pageblock_nr_pages) {
>>>>   :
>>>>                   if (isolate_migratepages_block(cc, low_pfn, block_end_pfn,  <--- low_pfn is passed as 
>>>>                                                                                    the argument
>>>
>>> Sorry, I think you're right. And could you please add the runtime effect of this issue?
>>>
>>> Anyway, this patch looks good to me now. Thanks!
>> 
>> Thank you for your review.
>> The runtime effect is that compaction become unintended behavior.
>> For example, pages not in the target zone are added to cc->migratepages list in isolate_migratepages_block().
>> As a result, pages migrate between nodes unintentionally.
>
> Many thanks for clarifying. :) Is this worth a Fixes tag or even CC stable?
>
> Thanks!

Thank you for your reply.

If add a Fixes tag, I think the following commit:
  Fixes: 70b4459 ("mm, compaction: use free lists to quickly locate a migration source")

Andrew, how do you think about this? 

Thanks,
Rei

Re: [PATCH] mm, compaction: fast_find_migrateblock() should return pfn in the target zone

[Mel Gorman]

On Wed, May 11, 2022 at 01:43:00PM +0900, Rei Yamamoto wrote:
> Prevent returning a pfn outside the target zone in case that not
> aligned with pageblock boundary.
> Otherwise isolate_migratepages_block() would handle pages not in
> the target zone.
> 
> Signed-off-by: Rei Yamamoto <yamamoto.rei@jp.fujitsu.com>

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm, compaction: fast_find_migrateblock() should return pfn in the target zone

[Andrew Morton]

On Thu, 12 May 2022 13:27:33 +0900 Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> wrote:

> >> Thank you for your review.
> >> The runtime effect is that compaction become unintended behavior.
> >> For example, pages not in the target zone are added to cc->migratepages list in isolate_migratepages_block().
> >> As a result, pages migrate between nodes unintentionally.
> >
> > Many thanks for clarifying. :) Is this worth a Fixes tag or even CC stable?
> >
> > Thanks!
> 
> Thank you for your reply.
> 
> If add a Fixes tag, I think the following commit:
>   Fixes: 70b4459 ("mm, compaction: use free lists to quickly locate a migration source")
> 
> Andrew, how do you think about this? 

Thanks, I added that and also a paragraph describing the effect of the bug:

https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/mm-compaction-fast_find_migrateblock-should-return-pfn-in-the-target-zone.patch

I assume this problem isn't sufficiently serious to require a -stable
backport of the fix?

Re: [PATCH] mm, compaction: fast_find_migrateblock() should return pfn in the target zone

[Rei Yamamoto]

On Thu, 12 May 2022 13:49:45 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 12 May 2022 13:27:33 +0900 Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> wrote:
>
>> >> Thank you for your review.
>> >> The runtime effect is that compaction become unintended behavior.
>> >> For example, pages not in the target zone are added to cc->migratepages list in isolate_migratepages_block().
>> >> As a result, pages migrate between nodes unintentionally.
>> >
>> > Many thanks for clarifying. :) Is this worth a Fixes tag or even CC stable?
>> >
>> > Thanks!
>> 
>> Thank you for your reply.
>> 
>> If add a Fixes tag, I think the following commit:
>>   Fixes: 70b4459 ("mm, compaction: use free lists to quickly locate a migration source")
>> 
>> Andrew, how do you think about this? 
>
> Thanks, I added that and also a paragraph describing the effect of the bug:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/mm-compaction-fast_find_migrateblock-should-return-pfn-in-the-target-zone.patch
>
> I assume this problem isn't sufficiently serious to require a -stable
> backport of the fix?

This would be a serious problem for older kernels without commit a984226, 
because it can corrupt the lru list by handling pages in list without holding proper lru_lock.

Thanks,
Rei

Re: [PATCH] mm, compaction: fast_find_migrateblock() should return pfn in the target zone

[Oscar Salvador]

On Wed, May 11, 2022 at 01:43:00PM +0900, Rei Yamamoto wrote:
> Prevent returning a pfn outside the target zone in case that not
> aligned with pageblock boundary.
> Otherwise isolate_migratepages_block() would handle pages not in
> the target zone.
> 
> Signed-off-by: Rei Yamamoto <yamamoto.rei@jp.fujitsu.com>

Reviewed-by: Oscar Salvador <osalvador@suse.de>

> ---
>  mm/compaction.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index fe915db6149b..de42b8e48758 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1858,6 +1858,8 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
>  
>  				update_fast_start_pfn(cc, free_pfn);
>  				pfn = pageblock_start_pfn(free_pfn);
> +				if (pfn < cc->zone->zone_start_pfn)
> +					pfn = cc->zone->zone_start_pfn;
>  				cc->fast_search_fail = 0;
>  				found_block = true;
>  				set_pageblock_skip(freepage);
> -- 
> 2.27.0
> 
> 

-- 
Oscar Salvador
SUSE Labs

Re: [PATCH] mm, compaction: fast_find_migrateblock() should return pfn in the target zone

[Andrew Morton]

On Fri, 13 May 2022 13:11:12 +0900 Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> wrote:

> On Thu, 12 May 2022 13:49:45 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Thu, 12 May 2022 13:27:33 +0900 Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> wrote:
> >
> >> >> Thank you for your review.
> >> >> The runtime effect is that compaction become unintended behavior.
> >> >> For example, pages not in the target zone are added to cc->migratepages list in isolate_migratepages_block().
> >> >> As a result, pages migrate between nodes unintentionally.
> >> >
> >> > Many thanks for clarifying. :) Is this worth a Fixes tag or even CC stable?
> >> >
> >> > Thanks!
> >> 
> >> Thank you for your reply.
> >> 
> >> If add a Fixes tag, I think the following commit:
> >>   Fixes: 70b4459 ("mm, compaction: use free lists to quickly locate a migration source")
> >> 
> >> Andrew, how do you think about this? 
> >
> > Thanks, I added that and also a paragraph describing the effect of the bug:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/mm-compaction-fast_find_migrateblock-should-return-pfn-in-the-target-zone.patch
> >
> > I assume this problem isn't sufficiently serious to require a -stable
> > backport of the fix?
> 
> This would be a serious problem for older kernels without commit a984226, 
> because it can corrupt the lru list by handling pages in list without holding proper lru_lock.

Thanks, I added the above to the changelog.

The patch applies OK to older kernels (I tried v5.10).  So I guess we
put a cc:stable in this, so it gets backported?

Re: [PATCH] mm, compaction: fast_find_migrateblock() should return pfn in the target zone

[Rei Yamamoto]

On Fri, 13 May 2022 14:01:41 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 13 May 2022 13:11:12 +0900 Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> wrote:
>
>> On Thu, 12 May 2022 13:49:45 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
>> > On Thu, 12 May 2022 13:27:33 +0900 Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> wrote:
>> >
>> >> >> Thank you for your review.
>> >> >> The runtime effect is that compaction become unintended behavior.
>> >> >> For example, pages not in the target zone are added to cc->migratepages list in isolate_migratepages_block().
>> >> >> As a result, pages migrate between nodes unintentionally.
>> >> >
>> >> > Many thanks for clarifying. :) Is this worth a Fixes tag or even CC stable?
>> >> >
>> >> > Thanks!
>> >> 
>> >> Thank you for your reply.
>> >> 
>> >> If add a Fixes tag, I think the following commit:
>> >>   Fixes: 70b4459 ("mm, compaction: use free lists to quickly locate a migration source")
>> >> 
>> >> Andrew, how do you think about this? 
>> >
>> > Thanks, I added that and also a paragraph describing the effect of the bug:
>> >
>> > https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/mm-compaction-fast_find_migrateblock-should-return-pfn-in-the-target-zone.patch
>> >
>> > I assume this problem isn't sufficiently serious to require a -stable
>> > backport of the fix?
>> 
>> This would be a serious problem for older kernels without commit a984226, 
>> because it can corrupt the lru list by handling pages in list without holding proper lru_lock.
>
> Thanks, I added the above to the changelog.
>
> The patch applies OK to older kernels (I tried v5.10).  So I guess we
> put a cc:stable in this, so it gets backported?

Sounds great.
I think that's fine.

Thanks,
Rei