linux-kernel - Re: [PATCH v3 2/4] mm/hwpoison: check if a subpage of a hugetlb folio is raw HWPOISON

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 12 Jul 2023 10:25:41 +0800
From:   Miaohe Lin <linmiaohe@...wei.com>
To:     Mike Kravetz <mike.kravetz@...cle.com>,
        Jiaqi Yan <jiaqiyan@...gle.com>
CC:     <akpm@...ux-foundation.org>, <naoya.horiguchi@....com>,
        <songmuchun@...edance.com>, <shy828301@...il.com>,
        <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
        <duenwen@...gle.com>, <axelrasmussen@...gle.com>,
        <jthoughton@...gle.com>
Subject: Re: [PATCH v3 2/4] mm/hwpoison: check if a subpage of a hugetlb folio
 is raw HWPOISON

On 2023/7/12 2:01, Mike Kravetz wrote:
> On 07/11/23 10:05, Jiaqi Yan wrote:
>> On Mon, Jul 10, 2023 at 8:16 AM Jiaqi Yan <jiaqiyan@...gle.com> wrote:
>>> On Fri, Jul 7, 2023 at 7:57 PM Miaohe Lin <linmiaohe@...wei.com> wrote:
>>>> On 2023/7/8 4:19, Jiaqi Yan wrote:
>>>>
>>>>> +             if (subpage == p->page) {
>>>>> +                     ret = true;
>>>>> +                     break;
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>> +     return ret;
>>>>>  }
>>>>
>>>> It seems there's a race between __is_raw_hwp_subpage and unpoison_memory:
>>>>   unpoison_memory               __is_raw_hwp_subpage
>>>>                                   if (!folio_test_hwpoison(folio)) -- hwpoison is set
>>>>     folio_free_raw_hwp            llist_for_each_entry_safe raw_hwp_list
>>>>       llist_del_all                 ..
>>>>     folio_test_clear_hwpoison
>>>>
>>>
>>> Thanks Miaohe for raising this concern.
>>>
>>>> But __is_raw_hwp_subpage is used in hugetlbfs, unpoison_memory couldn't reach here because there's a
>>>> folio_mapping == NULL check before folio_free_raw_hwp.
>>>
>>> I agree. But in near future I do want to make __is_raw_hwp_subpage
>>> work for shared-mapping hugetlb, so it would be nice to work with
>>> unpoison_memory. It doesn't seem to me that holding mf_mutex in
>>> __is_raw_hwp_subpage is nice or even absolutely correct. Let me think
>>> if I can come up with something in v4.
>>
>> At my 2nd thought, if __is_raw_hwp_subpage simply takes mf_mutex
>> before llist_for_each_entry, it will introduce a deadlock:
>>
>> unpoison_memory                       __is_raw_hwp_subpage
>>   held mf_mutex                         held hugetlb_lock
>>   get_hwpoison_hugetlb_folio            attempts mf_mutex
>>     attempts hugetlb lock
>>
>> Not for this patch series, but for future, is it a good idea to make
>> mf_mutex available to hugetlb code? Then enforce the order of locking
>> to be mf_mutex first, hugetlb_lock second? I believe this is the
>> current locking pattern / order for try_memory_failure_hugetlb.
> 
> I think only holding mf_mutex in __is_raw_hwp_subpage would be sufficient
> to prevent races with unpoison_memory.  memory failure code needs to take

Since soft_offline_page, memory_failure and unpoison_memory both holds mf_mutex,
I think this should be enough to prevent races between them too.

Thanks.