On Wed, Mar 30, 2022 at 3:12 PM Zi Yan <[email protected]> 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
On 30 Mar 2022, at 18:27, Linus Torvalds wrote:
> On Wed, Mar 30, 2022 at 3:12 PM Zi Yan <[email protected]> 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