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

[Naoya Horiguchi]

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,
Naoya Horiguchi