linux-kernel - Re: [PATCH v2] mm/page_isolation: fix potential missing call to unset_migratetype

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6cb372ff-0b68-2143-913e-04ab3e6b4160@huawei.com>
Date:   Mon, 13 Sep 2021 20:43:35 +0800
From:   Miaohe Lin <linmiaohe@...wei.com>
To:     David Hildenbrand <david@...hat.com>,
        Michal Hocko <mhocko@...e.com>
CC:     <akpm@...ux-foundation.org>, <vbabka@...e.cz>,
        <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] mm/page_isolation: fix potential missing call to
 unset_migratetype_isolate()

On 2021/9/13 20:20, David Hildenbrand wrote:
> On 13.09.21 14:12, Michal Hocko wrote:
>> On Mon 13-09-21 19:51:25, Miaohe Lin wrote:
>>> In start_isolate_page_range() undo path, pfn_to_online_page() just checks
>>> the first pfn in a pageblock while __first_valid_page() will traverse the
>>> pageblock until the first online pfn is found. So we may miss the call to
>>> unset_migratetype_isolate() in undo path and pages will remain isolated
>>> unexpectedly. Fix this by calling undo_isolate_page_range() and this will
>>> also help to simplify the code further.
>>
>> I like the clean up part but is this a real problem that requires CC
>> stable? Have you ever seen this to be a real problem? It looks like
>> something based on reading the code.

I'm sorry but I haven't seen this to be a real problem. It's a theoretical bug.

> 
> We discussed that it isn't an issue anymore (we never call it on memory holes), but might have been an issue on older kernels, back when we didn't have the "memory holes" check in the memory offlining path in place.

So is the Cc:stable needed in this case?

Many thanks for both of you.

> 
> Agreed, these details belong into this description.
> 
>>
>>> Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages")
>>> Signed-off-by: Miaohe Lin <linmiaohe@...wei.com>
>>> Cc: <stable@...r.kernel.org>
>>> ---
>>> v1->v2:
>>>    Simplify the code further per David Hildenbrand.
>>> ---
>>>   mm/page_isolation.c | 20 +++-----------------
>>>   1 file changed, 3 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>>> index a95c2c6562d0..f93cc63d8fa1 100644
>>> --- a/mm/page_isolation.c
>>> +++ b/mm/page_isolation.c
>>> @@ -183,7 +183,6 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
>>>                    unsigned migratetype, int flags)
>>>   {
>>>       unsigned long pfn;
>>> -    unsigned long undo_pfn;
>>>       struct page *page;
>>>         BUG_ON(!IS_ALIGNED(start_pfn, pageblock_nr_pages));
>>> @@ -193,25 +192,12 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
>>>            pfn < end_pfn;
>>>            pfn += pageblock_nr_pages) {
>>>           page = __first_valid_page(pfn, pageblock_nr_pages);
>>> -        if (page) {
>>> -            if (set_migratetype_isolate(page, migratetype, flags)) {
>>> -                undo_pfn = pfn;
>>> -                goto undo;
>>> -            }
>>> +        if (page && set_migratetype_isolate(page, migratetype, flags)) {
>>> +            undo_isolate_page_range(start_pfn, pfn, migratetype);
>>> +            return -EBUSY;
>>>           }
>>>       }
>>>       return 0;
>>> -undo:
>>> -    for (pfn = start_pfn;
>>> -         pfn < undo_pfn;
>>> -         pfn += pageblock_nr_pages) {
>>> -        struct page *page = pfn_to_online_page(pfn);
>>> -        if (!page)
>>> -            continue;
>>> -        unset_migratetype_isolate(page, migratetype);
>>> -    }
>>> -
>>> -    return -EBUSY;
>>>   }
>>>     /*
>>> -- 
>>> 2.23.0
>>
> 
>