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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 18 May 2020 19:04:40 -0700 From: Yang Shi <yang.shi@...ux.alibaba.com> To: "Kirill A. Shutemov" <kirill@...temov.name> Cc: kirill.shutemov@...ux.intel.com, akpm@...ux-foundation.org, linux-mm@...ck.org, linux-kernel@...r.kernel.org Subject: Re: [RFC linux-next PATCH] mm: khugepaged: remove error message when checking external pins On 5/18/20 3:19 AM, Kirill A. Shutemov wrote: > On Wed, May 13, 2020 at 05:03:03AM +0800, Yang Shi wrote: >> When running khugepaged with higher frequency (for example, set >> scan_sleep_millisecs to 0), the below error message was reported: >> >> khugepaged: expected_refcount (1024) > refcount (512) >> page:ffffd75784258000 count:511 mapcount:1 mapping:ffff968de06c7421 index:0x7fa288600 >> compound_mapcount: 0 >> flags: 0x17fffc00009003c(uptodate|dirty|lru|active|head|swapbacked) >> raw: 017fffc00009003c ffffd7578ba70788 ffffd7578bdb5148 ffff968de06c7421 >> raw: 00000007fa288600 0000000000000000 000001ff00000000 ffff968e5e7d6000 >> page dumped because: Unexpected refcount >> page->mem_cgroup:ffff968e5e7d6000 >> >> This is introduced by allowing collapsing fork shared and PTE-mapped >> THPs. The check may run into the below race: >> >> Assuming parent process forked child process, then they do >> >> CPU A CPU B CPU C >> ----- ----- ----- >> Parent Child khugepaged >> >> MADV_DONTNEED >> split huge pmd >> Double mapped >> MADV_DONTNEED >> zap_huge_pmd >> remove_page_rmap >> Clear double map >> khugepaged_scan_pmd(parent) >> check mapcount and refcount >> --> total_mapcount > refcount >> dec mapcount >> >> The issue can be reproduced by the below test program. > Good catch! Thanks. And the fix looks reasnable. > > We might want to have a similar debug check in near !is_refcount_suitable() > case in __collapse_huge_page_isolate(). The function is called with > anon_vma lock taken on write and it should prevent the false-positive. However it seems MADV_DONTNEED path doesn't take anon_vma lock. > > Anyway: > > Acked-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com> Thanks. > >> ---8<--- >> void main() >> { >> void *addr; >> int ret; >> pid_t pid; >> >> addr = memalign(ALIGN, 2 * 1024 * 1024); >> if (!addr) { >> printf("malloc failed\n"); >> return; >> } >> >> ret = madvise(addr, 2 * 1024 * 1024, MADV_HUGEPAGE); >> if (ret < 0) { >> printf("madvise failed\n"); >> return; >> } >> >> memset(addr, 0xdeadbeef, 2 * 1024 * 1024); >> >> pid = fork(); >> >> if (pid == 0) { >> /* Child process */ >> ret = madvise(addr + (2 * 1024 * 1024) - 4096, 4096, MADV_DONTNEED); >> if (ret < 0) { >> printf("madvise failed in child\n"); >> return; >> } >> sleep(120); >> } else if (pid > 0) { >> sleep(5); >> /* Parent process */ >> ret = madvise(addr, 2 * 1024 * 1024, MADV_DONTNEED); >> if (ret < 0) { >> printf("madvise failed in parent\n"); >> return; >> } >> } else { >> printf("fork failed\n"); >> return; >> } >> >> sleep(120); >> } >> ---8<--- >> >> So, total_mapcount > refcount seems not unexpected due to the inherent >> race. Removed the error message even though it is protected by >> CONFIG_VM_DEBUG since we have to live with the race and AFAIK some >> distros may have CONFIG_VM_DEBUG enabled dy default. >> >> Since such case is ephemeral we could always try collapse the area again >> later, so it sounds not harmful. But, it might report false positive if >> the page has excessive GUP pins (i.e. 512), however it might be not that >> bad since the same check will be done later. I didn't figure out a >> simple way to prevent the false positive. >> >> Added some notes to elaborate the race and the consequence as well. >> >> Cc: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com> >> Signed-off-by: Yang Shi <yang.shi@...ux.alibaba.com> >> --- >> mm/khugepaged.c | 24 +++++++++++++++++------- >> 1 file changed, 17 insertions(+), 7 deletions(-) >> >> diff --git a/mm/khugepaged.c b/mm/khugepaged.c >> index 1fdd677..048f5d4 100644 >> --- a/mm/khugepaged.c >> +++ b/mm/khugepaged.c >> @@ -602,12 +602,6 @@ static bool is_refcount_suitable(struct page *page) >> if (PageSwapCache(page)) >> expected_refcount += compound_nr(page); >> >> - if (IS_ENABLED(CONFIG_DEBUG_VM) && expected_refcount > refcount) { >> - pr_err("expected_refcount (%d) > refcount (%d)\n", >> - expected_refcount, refcount); >> - dump_page(page, "Unexpected refcount"); >> - } >> - >> return page_count(page) == expected_refcount; >> } >> >> @@ -1341,7 +1335,23 @@ static int khugepaged_scan_pmd(struct mm_struct *mm, >> goto out_unmap; >> } >> >> - /* Check if the page has any GUP (or other external) pins */ >> + /* >> + * Check if the page has any GUP (or other external) pins. >> + * >> + * Here the check is racy it may see totmal_mapcount > refcount >> + * in some cases. >> + * For example, one process with one forked child process. >> + * The parent has the PMD split due to MADV_DONTNEED, then >> + * the child is trying unmap the whole PMD, but khugepaged >> + * may be scanning the parent between the child has >> + * PageDoubleMap flag cleared and dec the mapcount. So >> + * khugepaged may see total_mapcount > refcount. >> + * >> + * But such case is ephemeral we could always retry collapse >> + * later. However it may report false positive if the page >> + * has excessive GUP pins (i.e. 512). Anyway the same check >> + * will be done again later the risk seems low. >> + */ >> if (!is_refcount_suitable(page)) { >> result = SCAN_PAGE_COUNT; >> goto out_unmap; >> -- >> 1.8.3.1 >> >>
Powered by blists - more mailing lists