Re: [PATCH] codetag: debug: mark codetags for pages which transitioned from being poison to unpoison as empty

[Suren Baghdasaryan <surenb@google.com>]

On Fri, Aug 23, 2024 at 1:10 AM Hao Ge <hao.ge@linux.dev> wrote:
>
> Hi Miaohe
>
>
> On 8/23/24 15:40, Miaohe Lin wrote:
> > On 2024/8/23 11:37, Hao Ge wrote:
> >> Hi Suren and Miaohe
> >>
> >>
> >> On 8/23/24 09:47, Hao Ge wrote:
> >>> Hi Suren and Miaohe
> >>>
> >>>
> >>> Thank you all for taking the time to discuss this issue.
> >>>
> >>>
> >>> On 8/23/24 06:50, Suren Baghdasaryan wrote:
> >>>> On Thu, Aug 22, 2024 at 2:46 AM Hao Ge <hao.ge@linux.dev> wrote:
> >>>>> Hi Miaohe
> >>>>>
> >>>>>
> >>>>> Thank you for taking the time to review this patch.
> >>>>>
> >>>>>
> >>>>> On 8/22/24 16:04, Miaohe Lin wrote:
> >>>>>> On 2024/8/22 10:58, Hao Ge wrote:
> >>>>>>> From: Hao Ge <gehao@kylinos.cn>
> >>>>>>>
> >>>>>> Thanks for your patch.
> >>>>>>
> >>>>>>> The PG_hwpoison page will be caught and isolated on the entrance to
> >>>>>>> the free buddy page pool. so,when we clear this flag and return it
> >>>>>>> to the buddy system,mark codetags for pages as empty.
> >>>>>>>
> >>>>>> Is below scene cause the problem?
> >>>>>>
> >>>>>> 1. Pages are allocated. pgalloc_tag_add() will be called when prep_new_page().
> >>>>>>
> >>>>>> 2. Pages are hwpoisoned. memory_failure() will set PG_hwpoison flag and pgalloc_tag_sub()
> >>>>>> will be called when pages are caught and isolated on the entrance to buddy.
> >>>> Hi Folks,
> >>>> Thanks for reporting this! Could you please describe in more details
> >>>> how memory_failure() ends up calling pgalloc_tag_sub()? It's not
> >>>> obvious to me which path leads to pgalloc_tag_sub(), so I must be
> >>>> missing something.
> >>>
> >>> OK,Let me describe the scenario I encountered.
> >>>
> >>> In the Link [1] I mentioned,here is the logic behind it:
> >>>
> >>> It performed the following operations:
> >>>
> >>> madvise(ptrs[num_alloc], pagesize, MADV_SOFT_OFFLINE)
> >>>
> >>> and then the kernel's call stack looks like this:
> >>>
> >>> do_madvise
> >>>
> >>> soft_offline_page
> >>>
> >>> page_handle_poison
> >>>
> >>> __folio_put
> >>>
> >>> free_unref_page
> >>>
> >> I just reviewed it and I think I missed a stack.
> >>
> >> Actually, it's like this
> >>
> >> do_madvise
> >>
> >> soft_offline_page
> >>
> >> soft_offline_in_use_page
> >>
> >> page_handle_poison
> >>
> >> __folio_put
> >>
> >> free_unref_page
> >>
> >>
> >> And I've come up with a minimal solution. If everyone agrees, I'll send the patch.look this
> >>
> >> https://elixir.bootlin.com/linux/v6.11-rc4/source/mm/page_alloc.c#L1056
> >>
> >> Let's directly call clear_page_tag_ref after pgalloc_tag_sub.
> > I tend to agree with you. It should be a good practice to call clear_page_tag_ref()
> > whenever page_tag finished its work. Do you think below code is also needed?
>
> Actually, this is not necessary,It follows the normal logic of
> allocation and release.

Yes, we don't need to clear_page_tag_ref() after every
pgalloc_tag_sub(), only in this special situation. alloc_tag_sub()
resets ref->ct to NULL at the end, so not clearing the tag allows us
to catch if an extra alloc_tag_sub() is called on this page later.

>
> The actual intention of the clear_page_tag_reffunction is to indicate to
> thealloc_tag  that the page is not being returned to the
>
> buddy system through normal allocation.
>
> Just like when the page first enters the buddy system,
>
> https://elixir.bootlin.com/linux/v6.11-rc4/source/mm/mm_init.c#L2464
>
> So, can you help review this patch?

Yeah, that looks good, just spelling in the comment needs fixing. I'll
comment on that patch.
Thanks,
Suren.

>
> https://lore.kernel.org/all/20240823062002.21165-1-hao.ge@linux.dev/
>
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index de54c3567539..707710f03cf5 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -1104,6 +1104,7 @@ __always_inline bool free_pages_prepare(struct page *page,
> >          reset_page_owner(page, order);
> >          page_table_check_free(page, order);
> >          pgalloc_tag_sub(page, 1 << order);
> > +       clear_page_tag_ref(page);
> >
> >          if (!PageHighMem(page)) {
> >                  debug_check_no_locks_freed(page_address(page),
> >
> > Thanks.
> > .