On 2022/3/15 13:49, HORIGUCHI NAOYA(堀口 直也) wrote:
> On Mon, Mar 14, 2022 at 03:10:25PM +0800, Miaohe Lin wrote:
>> On 2022/3/14 10:13, Naoya Horiguchi 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_failure() 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,
>>
>> It seems hold page lock could not help solve this race condition as hugetlb
>> page demotion is not required to hold the page lock. Could you please explain
>> this a bit more?
>
> Sorry, the last line in the paragraph need change. What prevents the current
> race is hugetlb_lock, not page lock. The page lock is here to prevent the
> race with hugepage allocation (not directly related to the current issue,
> but it's still necessary).
Many thanks for clarifying this.
>
>>
>> BTW:Is there some words missing or here should be 'page lock.' instead of 'page lock,' ?
>
> I should use a period here, I'll fix it.
>
> [...]
>
>>> @@ -1503,24 +1502,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);
>>> if (!res) {
>>
>> In this (res == 0) case, hugetlb page could be dissolved via __page_handle_poison.
>> But since PageHWPoison is not set yet, we can't set the PageHWPoison to the correct
>> page. Think about the below code in dissolve_free_huge_page:
>> /*
>> * Move PageHWPoison flag from head page to the raw
>> * error page, which makes any subpages rather than
>> * the error page reusable.
>> */
>> if (PageHWPoison(head) && page != head) {
>> SetPageHWPoison(page);
>> ClearPageHWPoison(head);
>> }
>>
>> SetPageHWPoison won't be called for the error page. Or am I miss something?
>
> No, you're right. We need call page_handle_poison() instead of
> __page_handle_poison().
>
> @@ -1512,7 +1512,7 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
> }
> unlock_page(head);
> res = MF_FAILED;
> - if (__page_handle_poison(p)) {
> + if (page_handle_poison(p, true, false)) {
> page_ref_inc(p);
> res = MF_RECOVERED;
> }
>
This one looks good to me.
>
>
>>
>>> lock_page(head);
>>> if (hwpoison_filter(p)) {
>>> - if (TestClearPageHWPoison(head))
>>> - num_poisoned_pages_dec();
>>> unlock_page(head);
>>> return -EOPNOTSUPP;
>>> }
>>> @@ -1553,13 +1539,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))
>>> + 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
>>> @@ -1585,6 +1574,14 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
>>> out:
>>> unlock_page(head);
>>> return res;
>>> +already_hwpoisoned:
>>> + put_page(p);
>>> + unlock_page(head);
>>
>> Generally speaking, we should do unlock_page before put_page or page might be disappeared
>> before we unlock the page. This should be ok when memory_failure succeeds to handle the
>> page previously as it holds one extra page refcnt. But it might be problematic when
>> memory_failure failed to handle the page last time. We might be the last user here.
>
> OK, so another code path in "if (hwpoison_filter)@ block seems to need
> the same change in the order.
You're right.
>
> Thanks,
> Naoya Horiguchi
>
Many thanks for your patch.
