Re: [PATCH v2 1/3] mm/page_alloc: correct start page when guard page debug is enabled

[Kemeng Shi]

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@xxxxxxxxxxxxxxx>
>>>
>>> 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@xxxxxxx>
> 
> Do you think of sending this patch (only patch 1/3) to -stable?
> If so, please add "Cc: stable@xxxxxxxxxxxxxxx" tag.
> 
Thanks for reply. Will cc stable in next version. Thanks!
> Thanks,
> Naoya Horiguchi
>