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 <[email protected]>
---
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
On Thu, 12 May 2022 10:27:44 +0800 Miaohe Lin <[email protected]> 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
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
On Thu, 12 May 2022 10:47:36 +0900 Rei Yamamoto <[email protected]> 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.
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 <[email protected]>
Acked-by: Mel Gorman <[email protected]>
--
Mel Gorman
SUSE Labs
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 <[email protected]>
Reviewed-by: Oscar Salvador <[email protected]>
> ---
> 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
On Fri, 13 May 2022 13:11:12 +0900 Rei Yamamoto <[email protected]> wrote:
> On Thu, 12 May 2022 13:49:45 -0700 Andrew Morton <[email protected]> wrote:
> > On Thu, 12 May 2022 13:27:33 +0900 Rei Yamamoto <[email protected]> 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?
On Thu, 12 May 2022 13:27:33 +0900 Rei Yamamoto <[email protected]> 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?
On Fri, 13 May 2022 14:01:41 -0700 Andrew Morton <[email protected]> wrote:
> On Fri, 13 May 2022 13:11:12 +0900 Rei Yamamoto <[email protected]> wrote:
>
>> On Thu, 12 May 2022 13:49:45 -0700 Andrew Morton <[email protected]> wrote:
>> > On Thu, 12 May 2022 13:27:33 +0900 Rei Yamamoto <[email protected]> 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