[PATCH] mm: page_alloc: validate buddy before check its migratetype.

[PATCH] mm: page_alloc: validate buddy before check its migratetype.

[Zi Yan]

From: Zi Yan <ziy@nvidia.com>

Whenever a buddy page is found, page_is_buddy() should be called to
check its validity. Add the missing check during pageblock merge check.

Fixes: 1dd214b8f21c ("mm: page_alloc: avoid merging non-fallbackable pageblocks with others")

Reported-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Zi Yan <ziy@nvidia.com>
---
 mm/page_alloc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index bdc8f60ae462..6c6af8658775 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1108,6 +1108,9 @@ static inline void __free_one_page(struct page *page,
 
 		buddy_pfn = __find_buddy_pfn(pfn, order);
 		buddy = page + (buddy_pfn - pfn);
+
+		if (!page_is_buddy(page, buddy, order))
+			goto done_merging;
 		buddy_mt = get_pageblock_migratetype(buddy);
 
 		if (migratetype != buddy_mt
-- 
2.35.1

Re: [PATCH] mm: page_alloc: validate buddy before check its migratetype.

[Linus Torvalds]

On Wed, Mar 30, 2022 at 3:12 PM Zi Yan <zi.yan@sent.com> wrote:
>
> Whenever a buddy page is found, page_is_buddy() should be called to
> check its validity. Add the missing check during pageblock merge check.

Applied.

>                 buddy_pfn = __find_buddy_pfn(pfn, order);
>                 buddy = page + (buddy_pfn - pfn);
> +
> +               if (!page_is_buddy(page, buddy, order))
> +                       goto done_merging;

I wonder if that sequence shouldn't be made some helper function.

Also, looking around, I will note that unset_migratetype_isolate() in
mm/page_isolation.c is missing that "page_is_buddy()" check.

I _think_ it's probably ok because we checked

        if (PageBuddy(page)) {

on the (original, non-puddy) page, and then we only use the buddy page
pointer for that

                   if (!is_migrate_isolate_page(buddy)) {

and it's been like that for a _loong_ time.

But honestly, it feels like we would be better off with always having
the page_is_buddy() check anyway.

Or is there some reason why we don't want it here?

             Linus

Re: [PATCH] mm: page_alloc: validate buddy before check its migratetype.

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]

On 30 Mar 2022, at 18:27, Linus Torvalds wrote:

> On Wed, Mar 30, 2022 at 3:12 PM Zi Yan <zi.yan@sent.com> wrote:
>>
>> Whenever a buddy page is found, page_is_buddy() should be called to
>> check its validity. Add the missing check during pageblock merge check.
>
> Applied.
>
>>                 buddy_pfn = __find_buddy_pfn(pfn, order);
>>                 buddy = page + (buddy_pfn - pfn);
>> +
>> +               if (!page_is_buddy(page, buddy, order))
>> +                       goto done_merging;
>
> I wonder if that sequence shouldn't be made some helper function.
>
> Also, looking around, I will note that unset_migratetype_isolate() in
> mm/page_isolation.c is missing that "page_is_buddy()" check.
>
> I _think_ it's probably ok because we checked
>
>         if (PageBuddy(page)) {
>
> on the (original, non-puddy) page, and then we only use the buddy page
> pointer for that
>
>                    if (!is_migrate_isolate_page(buddy)) {
>
> and it's been like that for a _loong_ time.
>
> But honestly, it feels like we would be better off with always having
> the page_is_buddy() check anyway.
>
> Or is there some reason why we don't want it here?
>
>              Linus

Like I said in the other email, memory isolation depends on sparsemem,
which would preclude the same NULL dereferencing from happening. But
I agree a helper function would be better. I will send a patch and see
how people think about it.


--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH] mm: page_alloc: validate buddy before check its migratetype.

[Steven Rostedt]

On Wed, 30 Mar 2022 18:12:38 -0400
Zi Yan <zi.yan@sent.com> wrote:

> From: Zi Yan <ziy@nvidia.com>
> 
> Whenever a buddy page is found, page_is_buddy() should be called to
> check its validity. Add the missing check during pageblock merge check.
> 
> Fixes: 1dd214b8f21c ("mm: page_alloc: avoid merging non-fallbackable pageblocks with others")

Link: https://lore.kernel.org/all/20220330154208.71aca532@gandalf.local.home/

> 
> Reported-by: Steven Rostedt <rostedt@goodmis.org>

Tested-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve

> Signed-off-by: Zi Yan <ziy@nvidia.com>
> ---
>  mm/page_alloc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index bdc8f60ae462..6c6af8658775 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1108,6 +1108,9 @@ static inline void __free_one_page(struct page *page,
>  
>  		buddy_pfn = __find_buddy_pfn(pfn, order);
>  		buddy = page + (buddy_pfn - pfn);
> +
> +		if (!page_is_buddy(page, buddy, order))
> +			goto done_merging;
>  		buddy_mt = get_pageblock_migratetype(buddy);
>  
>  		if (migratetype != buddy_mt

Re: [PATCH] mm: page_alloc: validate buddy before check its migratetype.

[Linus Torvalds]

On Wed, Mar 30, 2022 at 3:12 PM Zi Yan <zi.yan@sent.com> wrote:
>
> Fixes: 1dd214b8f21c ("mm: page_alloc: avoid merging non-fallbackable pageblocks with others")

Oh, btw - should this perhaps be backported further back than that
alleged "fixes" commit?

It does look like maybe the problem potentially existed before too,
and was just much harder to trigger.

That said, google doesn't find any other reports that look like
Steven's oops, so maybe it really never happened and backporting isn't
called for.

Or possibly my google-fu is just bad.

                  Linus

Re: [PATCH] mm: page_alloc: validate buddy before check its migratetype.

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 1921 bytes --]

On 30 Mar 2022, at 19:03, Linus Torvalds wrote:

> On Wed, Mar 30, 2022 at 3:12 PM Zi Yan <zi.yan@sent.com> wrote:
>>
>> Fixes: 1dd214b8f21c ("mm: page_alloc: avoid merging non-fallbackable pageblocks with others")
>
> Oh, btw - should this perhaps be backported further back than that
> alleged "fixes" commit?
>
> It does look like maybe the problem potentially existed before too,
> and was just much harder to trigger.
>
> That said, google doesn't find any other reports that look like
> Steven's oops, so maybe it really never happened and backporting isn't
> called for.
>
> Or possibly my google-fu is just bad.
>

There might not be any issue with the original code because this bug
could only be triggered when CONFIG_FLATMEM and CONFIG_MEMORY_ISOLATION
are both set, which never happens, since CONFIG_MEMORY_ISOLATION
depends on CONFIG_SPARSEMEM.

By checking Steven's boot log, it should be PFN 0x21ee00 that triggers
the bug, since the physical memory range ends at PFN 0x21edff.
PFN 0x21ee00 is 2MB aligned instead of MAX_ORDER-1 (4MB) aligned.
The original code assumes all physical memory ranges are at least
MAX_ORDER-1 aligned, which is true when CONFIG_SPARSEMEM is set
(CONFIG_MEMORY_ISOLATION depends on it), since CONFIG_SPARSEMEM
allocates pageblock_flags array (the NULL-deferenced bitmap points
to) at section size granularity (128MB > 4MB). However, CONFIG_FLATMEM
does not do this. It allocates pageblock_flags array at the exact size
of the physical memory. So checking 0x21ee00 will not cause NULL
dereferencing when CONFIG_MEMORY_ISOLATION is set and the original
if statement can be true.

Now I am wondering if the page_is_buddy() check is correct for
CONFIG_FLATMEM. Is mem_map allocation aligned to MAX_ORDER-1
or just the present physical memory range? Is PageBuddy(0x21ee00)
accessing some random memory location?

--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH] mm: page_alloc: validate buddy before check its migratetype.

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 2128 bytes --]

On 30 Mar 2022, at 19:48, Zi Yan wrote:

> On 30 Mar 2022, at 19:03, Linus Torvalds wrote:
>
>> On Wed, Mar 30, 2022 at 3:12 PM Zi Yan <zi.yan@sent.com> wrote:
>>>
>>> Fixes: 1dd214b8f21c ("mm: page_alloc: avoid merging non-fallbackable pageblocks with others")
>>
>> Oh, btw - should this perhaps be backported further back than that
>> alleged "fixes" commit?
>>
>> It does look like maybe the problem potentially existed before too,
>> and was just much harder to trigger.
>>
>> That said, google doesn't find any other reports that look like
>> Steven's oops, so maybe it really never happened and backporting isn't
>> called for.
>>
>> Or possibly my google-fu is just bad.
>>
>
> There might not be any issue with the original code because this bug
> could only be triggered when CONFIG_FLATMEM and CONFIG_MEMORY_ISOLATION
> are both set, which never happens, since CONFIG_MEMORY_ISOLATION
> depends on CONFIG_SPARSEMEM.
>
> By checking Steven's boot log, it should be PFN 0x21ee00 that triggers
> the bug, since the physical memory range ends at PFN 0x21edff.
> PFN 0x21ee00 is 2MB aligned instead of MAX_ORDER-1 (4MB) aligned.
> The original code assumes all physical memory ranges are at least
> MAX_ORDER-1 aligned, which is true when CONFIG_SPARSEMEM is set
> (CONFIG_MEMORY_ISOLATION depends on it), since CONFIG_SPARSEMEM
> allocates pageblock_flags array (the NULL-deferenced bitmap points
> to) at section size granularity (128MB > 4MB). However, CONFIG_FLATMEM
> does not do this. It allocates pageblock_flags array at the exact size
> of the physical memory. So checking 0x21ee00 will not cause NULL
> dereferencing when CONFIG_MEMORY_ISOLATION is set and the original
> if statement can be true.
>
> Now I am wondering if the page_is_buddy() check is correct for
> CONFIG_FLATMEM. Is mem_map allocation aligned to MAX_ORDER-1
> or just the present physical memory range? Is PageBuddy(0x21ee00)
> accessing some random memory location?

OK. mem_map seems to be MAX_ORDER-1 aligned, so there is no
problem with PageBuddy(0x21ee00).



--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH] mm: page_alloc: validate buddy before check its migratetype.

[Vlastimil Babka]

On 3/31/22 02:10, Zi Yan wrote:
> On 30 Mar 2022, at 19:48, Zi Yan wrote:
> 
>> On 30 Mar 2022, at 19:03, Linus Torvalds wrote:
>>
>>> On Wed, Mar 30, 2022 at 3:12 PM Zi Yan <zi.yan@sent.com> wrote:
>>>>
>>>> Fixes: 1dd214b8f21c ("mm: page_alloc: avoid merging non-fallbackable pageblocks with others")
>>>
>>> Oh, btw - should this perhaps be backported further back than that
>>> alleged "fixes" commit?
>>>
>>> It does look like maybe the problem potentially existed before too,
>>> and was just much harder to trigger.
>>>
>>> That said, google doesn't find any other reports that look like
>>> Steven's oops, so maybe it really never happened and backporting isn't
>>> called for.
>>>
>>> Or possibly my google-fu is just bad.
>>>
>>
>> There might not be any issue with the original code because this bug
>> could only be triggered when CONFIG_FLATMEM and CONFIG_MEMORY_ISOLATION
>> are both set, which never happens, since CONFIG_MEMORY_ISOLATION
>> depends on CONFIG_SPARSEMEM.

Good point. Which means unset_migratetype_isolate() that Linus pointed
out, is currently also safe as it's a CONFIG_MEMORY_ISOLATION code.
We could still implement the suggested page_find_buddy() wrapper using
page_is_buddy() internally, as well as the cleanup of __free_one_page(),
but it's not urgent.

>> By checking Steven's boot log, it should be PFN 0x21ee00 that triggers
>> the bug, since the physical memory range ends at PFN 0x21edff.
>> PFN 0x21ee00 is 2MB aligned instead of MAX_ORDER-1 (4MB) aligned.
>> The original code assumes all physical memory ranges are at least
>> MAX_ORDER-1 aligned, which is true when CONFIG_SPARSEMEM is set
>> (CONFIG_MEMORY_ISOLATION depends on it), since CONFIG_SPARSEMEM
>> allocates pageblock_flags array (the NULL-deferenced bitmap points
>> to) at section size granularity (128MB > 4MB). However, CONFIG_FLATMEM
>> does not do this. It allocates pageblock_flags array at the exact size
>> of the physical memory. So checking 0x21ee00 will not cause NULL
>> dereferencing when CONFIG_MEMORY_ISOLATION is set and the original
>> if statement can be true.
>>
>> Now I am wondering if the page_is_buddy() check is correct for
>> CONFIG_FLATMEM. Is mem_map allocation aligned to MAX_ORDER-1
>> or just the present physical memory range? Is PageBuddy(0x21ee00)
>> accessing some random memory location?
> 
> OK. mem_map seems to be MAX_ORDER-1 aligned, so there is no
> problem with PageBuddy(0x21ee00).

Yeah mem_map has to be in all config variants, otherwise buddy merging
would have been blowing up in page_is_buddy() even prior to all the
"sometimes avoid merging pageblock" changes.
> 
> --
> Best Regards,
> Yan, Zi

Re: [PATCH] mm: page_alloc: validate buddy before check its migratetype.

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 3345 bytes --]

On 31 Mar 2022, at 4:57, Vlastimil Babka wrote:

> On 3/31/22 02:10, Zi Yan wrote:
>> On 30 Mar 2022, at 19:48, Zi Yan wrote:
>>
>>> On 30 Mar 2022, at 19:03, Linus Torvalds wrote:
>>>
>>>> On Wed, Mar 30, 2022 at 3:12 PM Zi Yan <zi.yan@sent.com> wrote:
>>>>>
>>>>> Fixes: 1dd214b8f21c ("mm: page_alloc: avoid merging non-fallbackable pageblocks with others")
>>>>
>>>> Oh, btw - should this perhaps be backported further back than that
>>>> alleged "fixes" commit?
>>>>
>>>> It does look like maybe the problem potentially existed before too,
>>>> and was just much harder to trigger.
>>>>
>>>> That said, google doesn't find any other reports that look like
>>>> Steven's oops, so maybe it really never happened and backporting isn't
>>>> called for.
>>>>
>>>> Or possibly my google-fu is just bad.
>>>>
>>>
>>> There might not be any issue with the original code because this bug
>>> could only be triggered when CONFIG_FLATMEM and CONFIG_MEMORY_ISOLATION
>>> are both set, which never happens, since CONFIG_MEMORY_ISOLATION
>>> depends on CONFIG_SPARSEMEM.
>
> Good point. Which means unset_migratetype_isolate() that Linus pointed
> out, is currently also safe as it's a CONFIG_MEMORY_ISOLATION code.
> We could still implement the suggested page_find_buddy() wrapper using
> page_is_buddy() internally, as well as the cleanup of __free_one_page(),
> but it's not urgent.
>
Sure. Will do that.

>>> By checking Steven's boot log, it should be PFN 0x21ee00 that triggers
>>> the bug, since the physical memory range ends at PFN 0x21edff.
>>> PFN 0x21ee00 is 2MB aligned instead of MAX_ORDER-1 (4MB) aligned.
>>> The original code assumes all physical memory ranges are at least
>>> MAX_ORDER-1 aligned, which is true when CONFIG_SPARSEMEM is set
>>> (CONFIG_MEMORY_ISOLATION depends on it), since CONFIG_SPARSEMEM
>>> allocates pageblock_flags array (the NULL-deferenced bitmap points
>>> to) at section size granularity (128MB > 4MB). However, CONFIG_FLATMEM
>>> does not do this. It allocates pageblock_flags array at the exact size
>>> of the physical memory. So checking 0x21ee00 will not cause NULL
>>> dereferencing when CONFIG_MEMORY_ISOLATION is set and the original
>>> if statement can be true.


For the record, the actual situation is that struct page of PFN 0x21ee00
is allocated but uninitialized. In CONFIG_FLATMEM, pageblock flags are
stored in zone->pageblock_flags. When reading PFN 0x21ee00's pageblock
flags, the page's zone is determined to be ZONE_MOVABLE, since page->flags
is -1UL and page_zonenum() is 3. The system does not have ZONE_MOVABLE,
so zone->pageblock_flags is NULL and reading it caused NULL pointer
dereferencing.

>>>
>>> Now I am wondering if the page_is_buddy() check is correct for
>>> CONFIG_FLATMEM. Is mem_map allocation aligned to MAX_ORDER-1
>>> or just the present physical memory range? Is PageBuddy(0x21ee00)
>>> accessing some random memory location?
>>
>> OK. mem_map seems to be MAX_ORDER-1 aligned, so there is no
>> problem with PageBuddy(0x21ee00).
>
> Yeah mem_map has to be in all config variants, otherwise buddy merging
> would have been blowing up in page_is_buddy() even prior to all the
> "sometimes avoid merging pageblock" changes.
>>
>> --
>> Best Regards,
>> Yan, Zi


--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]