Re: [PATCH v1] mm/hwpoison: set PageHWPoison after taking page lock in memory_failure_hugetlb()

[Miaohe Lin]

On 2022/3/10 8:30, Yang Shi wrote:
> On Wed, Mar 9, 2022 at 4:01 PM HORIGUCHI NAOYA(堀口　直也)
> <naoya.horiguchi@xxxxxxx> wrote:
>>
>> On Wed, Mar 09, 2022 at 01:55:30PM -0800, Yang Shi wrote:
>>> On Wed, Mar 9, 2022 at 1:15 AM Naoya Horiguchi
>>> <naoya.horiguchi@xxxxxxxxx> wrote:
>>>>
>>>> From: Naoya Horiguchi <naoya.horiguchi@xxxxxxx>
>>>>
>>>> There is a race condition between memory_failure_hugetlb() and hugetlb
>>>> free/demotion, which causes setting PageHWPoison flag on the wrong page
>>>> (which was a hugetlb when memory_failrue() was called, but was removed
>>>> or demoted when memory_failure_hugetlb() is called).  This results in
>>>> killing wrong processes.  So set PageHWPoison flag with holding page lock,
>>>>
>>>> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@xxxxxxx>
>>>> ---
>>>>  mm/memory-failure.c | 27 ++++++++++++---------------
>>>>  1 file changed, 12 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>>> index ac6492e36978..fe25eee8f9d6 100644
>>>> --- a/mm/memory-failure.c
>>>> +++ b/mm/memory-failure.c
>>>> @@ -1494,24 +1494,11 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
>>>>         int res;
>>>>         unsigned long page_flags;
>>>>
>>>> -       if (TestSetPageHWPoison(head)) {
>>>> -               pr_err("Memory failure: %#lx: already hardware poisoned\n",
>>>> -                      pfn);
>>>> -               res = -EHWPOISON;
>>>> -               if (flags & MF_ACTION_REQUIRED)
>>>> -                       res = kill_accessing_process(current, page_to_pfn(head), flags);
>>>> -               return res;
>>>> -       }
>>>> -
>>>> -       num_poisoned_pages_inc();
>>>> -
>>>>         if (!(flags & MF_COUNT_INCREASED)) {
>>>>                 res = get_hwpoison_page(p, flags);
>>>
>>> I'm not an expert of hugetlb, I may be wrong. I'm wondering how this
>>> could solve the race? Is the below race still possible?
>>>
>>> __get_hwpoison_page()
>>>   head = compound_head(page)
>>>
>>> hugetlb demotion (1G --> 2M)
>>>   get_hwpoison_huge_page(head, &hugetlb);
>>
>> Thanks for the comment.
>> I assume Miaohe's patch below introduces additional check to detect the
>> race.  The patch calls compound_head() for the raw error page again, so
>> the demotion case should be detected.  I'll make the dependency clear in
>> the commit log.
>>
>> https://lore.kernel.org/linux-mm/20220228140245.24552-2-linmiaohe@xxxxxxxxxx/
>>
>>>
>>>
>>> Then the head may point to a 2M page, but the hwpoisoned subpage is
>>> not in that 2M range?
>>>
>>>
>>>>                 if (!res) {
>>>>                         lock_page(head);
>>>>                         if (hwpoison_filter(p)) {
>>>> -                               if (TestClearPageHWPoison(head))
>>>> -                                       num_poisoned_pages_dec();
>>>>                                 unlock_page(head);
>>>>                                 return -EOPNOTSUPP;
>>>>                         }
>>>> @@ -1544,13 +1531,16 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
>>>>         page_flags = head->flags;
>>>>
>>>>         if (hwpoison_filter(p)) {
>>>> -               if (TestClearPageHWPoison(head))
>>>> -                       num_poisoned_pages_dec();
>>>>                 put_page(p);
>>>>                 res = -EOPNOTSUPP;
>>>>                 goto out;
>>>>         }
>>>>
>>>> +       if (TestSetPageHWPoison(head))
>>>
>>> And I don't think "head" is still the head you expected if the race
>>> happened. I think we need to re-retrieve the head once the page
>>> refcount is bumped and locked.
>>
>> I think the above justification works for this.
>> When the kernel reaches this line, the hugepage is properly pinned without being
>> freed or demoted, so "head" is still pointing to the same head page as expected.
> 
> I think Mike's comment in the earlier email works for this too. The
> huge page may get demoted before the page is pinned and locked, so the
> actual hwpoisoned subpage may belong to another smaller huge page now.
> 

I thinks Naoya assumes that there is a check before we use "head":

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 5444a8ef4867..0d7c58340a98 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1534,6 +1534,17 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
 	}

 	lock_page(head);
+
+	/**
+	 * The page could have changed compound pages due to race window.
+	 * If this happens just bail out.
+	 */
+	if (!PageHuge(p) || compound_head(p) != head) {
+		action_result(pfn, MF_MSG_DIFFERENT_COMPOUND, MF_IGNORED);
+		res = -EBUSY;
+		goto out;
+	}
+
 	page_flags = head->flags;

 	if (hwpoison_filter(p)) {
-- 
from: https://lore.kernel.org/linux-mm/20220228140245.24552-2-linmiaohe@xxxxxxxxxx/

Thanks.

> 
>>
>> Thanks,
>> Naoya Horiguchi
>>
>>>
>>>> +               goto already_hwpoisoned;
>>>> +
>>>> +       num_poisoned_pages_inc();
>>>> +
>>>>         /*
>>>>          * TODO: hwpoison for pud-sized hugetlb doesn't work right now, so
>>>>          * simply disable it. In order to make it work properly, we need
>>>> @@ -1576,6 +1566,13 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
>>>>  out:
>>>>         unlock_page(head);
>>>>         return res;
>>>> +already_hwpoisoned:
>>>> +       unlock_page(head);
>>>> +       pr_err("Memory failure: %#lx: already hardware poisoned\n", pfn);
>>>> +       res = -EHWPOISON;
>>>> +       if (flags & MF_ACTION_REQUIRED)
>>>> +               res = kill_accessing_process(current, page_to_pfn(head), flags);
>>>> +       return res;
>>>>  }
>>>>
>>>>  static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
>>>> --
>>>> 2.25.1
>>>>
> .
>