Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Subject: Re: [PATCH v2 1/3] mm/page_alloc: correct start page when guard page
 debug is enabled
To:     Naoya Horiguchi <naoya.horiguchi@linux.dev>
Cc:     akpm@linux-foundation.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, willy@infradead.org,
        naoya.horiguchi@nec.com, osalvador@suse.de
References: <20230826154745.4019371-1-shikemeng@huaweicloud.com>
 <20230826154745.4019371-2-shikemeng@huaweicloud.com>
 <20230828152113.GA886794@ik1-406-35019.vs.sakura.ne.jp>
 <b42625d1-4c50-5c61-ef92-5008383f8682@huaweicloud.com>
 <20230926113338.GA1539169@ik1-406-35019.vs.sakura.ne.jp>
From:   Kemeng Shi <shikemeng@huaweicloud.com>
Message-ID: <4c050b13-8aca-7b19-333e-907e483804ea@huaweicloud.com>
Date:   Wed, 27 Sep 2023 09:13:46 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.0
MIME-Version: 1.0
In-Reply-To: <20230926113338.GA1539169@ik1-406-35019.vs.sakura.ne.jp>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


on 9/26/2023 7:33 PM, Naoya Horiguchi wrote:
> On Wed, Aug 30, 2023 at 02:27:33PM +0800, Kemeng Shi wrote:
>>
>>
>> on 8/28/2023 11:21 PM, Naoya Horiguchi wrote:
>>> On Sat, Aug 26, 2023 at 11:47:43PM +0800, Kemeng Shi wrote:
>>>> When guard page debug is enabled and set_page_guard returns success, we
>>>> miss to forward page to point to start of next split range and we will do
>>>> split unexpectedly in page range without target page. Move start page
>>>> update before set_page_guard to fix this.
>>>>
>>>> As we split to wrong target page, then splited pages are not able to merge
>>>> back to original order when target page is put back and splited pages
>>>> except target page is not usable. To be specific:
>>>>
>>>> Consider target page is the third page in buddy page with order 2.
>>>> | buddy-2 | Page | Target | Page |
>>>>
>>>> After break down to target page, we will only set first page to Guard
>>>> because of bug.
>>>> | Guard   | Page | Target | Page |
>>>>
>>>> When we try put_page_back_buddy with target page, the buddy page of target
>>>> if neither guard nor buddy, Then it's not able to construct original page
>>>> with order 2
>>>> | Guard | Page | buddy-0 | Page |
>>>>
>>>> All pages except target page is not in free list and is not usable.
>>>>
>>>> Fixes: 06be6ff3d2ec ("mm,hwpoison: rework soft offline for free pages")
>>>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>>>
>>> Thank you for finding the problem and writing patches.  I think the patch
>>> fixes the reported problem, But I wonder that we really need guard page
>>> mechanism in break_down_buddy_pages() which is only called from memory_failure.
>>> As stated in Documentation/admin-guide/kernel-parameters.txt, this is a
>>> debugging feature to detect memory corruption due to buggy kernel or drivers
>>> code.  So if HW memory failrue seems to be out of the scope, and I feel that
>>> we could simply remove it from break_down_buddy_pages().
>>>
>>>         debug_guardpage_minorder=
>>>                         [KNL] When CONFIG_DEBUG_PAGEALLOC is set, this
>>>                         parameter allows control of the order of pages that will
>>>                         be intentionally kept free (and hence protected) by the
>>>                         buddy allocator. Bigger value increase the probability
>>>                         of catching random memory corruption, but reduce the
>>>                         amount of memory for normal system use. The maximum
>>>                         possible value is MAX_ORDER/2.  Setting this parameter
>>>                         to 1 or 2 should be enough to identify most random
>>>                         memory corruption problems caused by bugs in kernel or
>>>                         driver code when a CPU writes to (or reads from) a
>>>                         random memory location. Note that there exists a class
>>>                         of memory corruptions problems caused by buggy H/W or
>>>                         F/W or by drivers badly programming DMA (basically when
>>>                         memory is written at bus level and the CPU MMU is
>>>                         bypassed) which are not detectable by
>>>                         CONFIG_DEBUG_PAGEALLOC, hence this option will not help
>>>                         tracking down these problems.
>>>
>>> If you have any idea about how guard page mechanism helps memory_failrue,
>>> could you share it?
>>>
>> Hi Naoya, thanks for feedback. Commit c0a32fc5a2e47 ("mm: more intensive
>> memory corruption debugging") menthioned we konw that with
>> CONFIG_DEBUG_PAGEALLOC configured, the CPU will generate an exception on
>> access (read,write) to an unallocated page, which permits us to catch code
>> which corrupts memory; Guard page aims to keep more free/protected pages
>> and to interlace free/protected and allocated pages to increase the
>> probability of catching corruption. Keep guard page around failrue looks
>> helpful to catch random access. Wish this can help.
> 
> Sorry for my late response.
> I'm OK with keeping guardpage stuff in this code path as long as it properly works.
> And the patch looks good to me.
> 
> Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> 
> Do you think of sending this patch (only patch 1/3) to -stable?
> If so, please add "Cc: stable@vger.kernel.org" tag.
> 
Thanks for reply. Will cc stable in next version. Thanks!
> Thanks,
> Naoya Horiguchi
>