Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220330221238.396357-1-zi.yan@sent.com>
In-Reply-To: <20220330221238.396357-1-zi.yan@sent.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed, 30 Mar 2022 15:27:04 -0700
Message-ID: <CAHk-=wji_AmYygZMTsPMdJ7XksMt7kOur8oDfDdniBRMjm4VkQ@mail.gmail.com>
Subject: Re: [PATCH] mm: page_alloc: validate buddy before check its migratetype.
To:     Zi Yan <ziy@nvidia.com>
Cc:     Steven Rostedt <rostedt@goodmis.org>,
        David Hildenbrand <david@redhat.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Mel Gorman <mgorman@techsingularity.net>,
        Mike Rapoport <rppt@kernel.org>,
        Oscar Salvador <osalvador@suse.de>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux-MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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