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] [thread-next>] [day] [month] [year] [list]

Message-ID: <4jvrmdpyteny5vaqmcrctzrovap2oy2zuukybbhfqyqbbb5xmy@ufgxufss2ngw>
Date:   Tue, 15 Aug 2023 13:25:08 +0000
From:   Daniel Gomez <da.gomez@...sung.com>
To:     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>,
        "david@...hat.com" <david@...hat.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

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?

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
...

Daniel
>
> User-visible effects is that the THP is skipped when user call madvise.
> But the correct behavior is THP should be split and processed then.
>
> NOTE: this change is a temporary fix to reduce the user-visible effects
> before the long term fix from David is ready.
>
> Fixes: 98b211d6415f ("madvise: convert madvise_free_pte_range() to use a folio")
> Cc: stable@...r.kernel.org
> Signed-off-by: Yin Fengwei <fengwei.yin@...el.com>
> Reviewed-by: Yu Zhao <yuzhao@...gle.com>
> Reviewed-by: Ryan Roberts <ryan.roberts@....com>
> ---
>  mm/madvise.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 49af35e2d99a..4dded5d27e7e 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -683,7 +683,7 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>  		if (folio_test_large(folio)) {
>  			int err;
>
> -			if (folio_mapcount(folio) != 1)
> +			if (folio_estimated_sharers(folio) != 1)
>  				break;
>  			if (!folio_trylock(folio))
>  				break;
> --
> 2.39.2
>