linux-kernel - Re: [PATCH v2 3/3] madvise:madvise_free_pte_range(): don't use mapcount() against large folio for sharing check

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]

Message-ID: <g6t5ldoy64lrurvuttfhw3kxc6tdtphbskxezyaocaadrhgkfb@kws4v6gewbh3>
Date:   Thu, 17 Aug 2023 07:57:52 +0000
From:   Daniel Gomez <da.gomez@...sung.com>
To:     David Hildenbrand <david@...hat.com>,
        "Yin, Fengwei" <fengwei.yin@...el.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "willy@...radead.org" <willy@...radead.org>,
        "vishal.moola@...il.com" <vishal.moola@...il.com>,
        "wangkefeng.wang@...wei.com" <wangkefeng.wang@...wei.com>,
        "minchan@...nel.org" <minchan@...nel.org>,
        "yuzhao@...gle.com" <yuzhao@...gle.com>,
        "ryan.roberts@....com" <ryan.roberts@....com>,
        "shy828301@...il.com" <shy828301@...il.com>
Subject: Re: [PATCH v2 3/3] madvise:madvise_free_pte_range(): don't use
 mapcount() against large folio for sharing check

On Wed, Aug 16, 2023 at 05:11:54PM +0200, David Hildenbrand wrote:
> On 16.08.23 16:13, Daniel Gomez wrote:
> > On Wed, Aug 16, 2023 at 08:04:11PM +0800, Yin, Fengwei wrote:
> > >
> > >
> > > On 8/16/2023 7:44 PM, Daniel Gomez wrote:
> > > > On Wed, Aug 16, 2023 at 07:30:35AM +0800, Yin Fengwei wrote:
> > > > >
> > > > >
> > > > > On 8/15/23 21:25, Daniel Gomez wrote:
> > > > > > Hi Yin,
> > > > > > On Tue, Aug 08, 2023 at 10:09:17AM +0800, Yin Fengwei wrote:
> > > > > > > Commit 98b211d6415f ("madvise: convert madvise_free_pte_range() to use a
> > > > > > > folio") replaced the page_mapcount() with folio_mapcount() to check
> > > > > > > whether the folio is shared by other mapping.
> > > > > > >
> > > > > > > It's not correct for large folios. folio_mapcount() returns the total
> > > > > > > mapcount of large folio which is not suitable to detect whether the folio
> > > > > > > is shared.
> > > > > > >
> > > > > > > Use folio_estimated_sharers() which returns a estimated number of shares.
> > > > > > > That means it's not 100% correct. It should be OK for madvise case here.
> > > > > >
> > > > > > I'm trying to understand why it should be ok for madvise this change, so
> > > > > > I hope it's okay to ask you few questions.
> > > > > >
> > > > > > folio_mapcount() calculates the total maps for all the subpages of a
> > > > > > folio. However, the folio_estimated_sharers does it only for the first
> > > > > > subpage making it not true for large folios. Then, wouldn't this change
> > > > > > drop support for large folios?
> > > > > I saw David explained this very well in another mail.
> > > > >
> > > > > >
> > > > > > Seems like folio_entire_mapcount() is not accurate either because of it
> > > > > > does not inclue PTE-mapped sub-pages which I think we need here. Hence,
> > > > > > the folio_mapcount(). Could this be something missing in the test side?
> > >  > >
> > > > > > I tried to replicate the setup with CONFIG_TRANSPARENT_HUGEPAGE but
> > > > > > seems like I'm not able to do it:
> > > > > >
> > > > > > ./cow
> > > > > > # [INFO] detected THP size: 2048 KiB
> > > > > > # [INFO] detected hugetlb size: 2048 KiB
> > > > > > # [INFO] detected hugetlb size: 1048576 KiB
> > > > > > # [INFO] huge zeropage is enabled
> > > > > > TAP version 13
> > > > > > 1..166
> > > > > > # [INFO] Anonymous memory tests in private mappings
> > > > > > # [RUN] Basic COW after fork() ... with base page
> > > > > > not ok 1 MADV_NOHUGEPAGE failed
> > > > > > # [RUN] Basic COW after fork() ... with swapped out base page
> > > > > > not ok 2 MADV_NOHUGEPAGE failed
> > > > > > # [RUN] Basic COW after fork() ... with THP
> > > > > > not ok 3 MADV_HUGEPAGE failed
> > > > > > # [RUN] Basic COW after fork() ... with swapped-out THP
> > > > > > not ok 4 MADV_HUGEPAGE failed
> > > > > > # [RUN] Basic COW after fork() ... with PTE-mapped THP
> > > > > > not ok 5 MADV_HUGEPAGE failed
> > > > > > # [RUN] Basic COW after fork() ... with swapped-out, PTE-mapped THP
> > > > > > not ok 6 MADV_HUGEPAGE failed
> > > > > > ...
> > > > > Can you post the MADV_PAGEOUT and PTE-mapped THP related testing result?
> > > > > And I suppose swap need be enabled also for the testing.
> > > >
> > > > You may find a dump of the logs in the link below with system information. Let me
> > > > know if you find something wrong in my setup or if you need something else.
> > > > Besides CONFIG_TRANSPARENT_HUGEPAGE, CONFIG_SWAP is also enabled in the kernel.
> > > >
> > > > https://gitlab.com/-/snippets/2584135
> > > >
> > > > Also, strace reports ENOSYS for MADV_*:
> > > > madvise(0x7f2912465000, 4096, MADV_NOHUGEPAGE) = -1 ENOSYS (Function not implemented)
> > > > madvise(0x7f2912000000, 2097152, MADV_HUGEPAGE) = -1 ENOSYS (Function not implemented)
> > > O. The problem here is MADV_HUGEPAGE/MADV_NOHUGEPAGE doesn't work.
> > > Do you have CONFIG_ADVISE_SYSCALLS enabled?
> > It worked after I enabled the conf. Some tests failed and some were
> > skipped. But I managed to reproduce the issue now, thanks Yin!
> >
> > Bail out! 4 out of 166 tests failed
> > # Totals: pass:146 fail:4 xfail:0 xpass:0 skip:16 error:0
> >
>
> These hugetlb that are failing are known failures.

Hi David, thanks for letting me know. Also, thanks for the description
of the folio mapping in the other mail.
>
> --
> Cheers,
>
> David / dhildenb
>