linux-kernel - Re: [PATCH v20 08/20] mm: page_idle_get_page() does not need lru

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: <20201105045702.GI17076@casper.infradead.org>
Date:   Thu, 5 Nov 2020 04:57:02 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     Alex Shi <alex.shi@...ux.alibaba.com>
Cc:     Johannes Weiner <hannes@...xchg.org>, akpm@...ux-foundation.org,
        mgorman@...hsingularity.net, tj@...nel.org, hughd@...gle.com,
        khlebnikov@...dex-team.ru, daniel.m.jordan@...cle.com,
        lkp@...el.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        cgroups@...r.kernel.org, shakeelb@...gle.com,
        iamjoonsoo.kim@....com, richard.weiyang@...il.com,
        kirill@...temov.name, alexander.duyck@...il.com,
        rong.a.chen@...el.com, mhocko@...e.com, vdavydov.dev@...il.com,
        shy828301@...il.com, Vlastimil Babka <vbabka@...e.cz>,
        Minchan Kim <minchan@...nel.org>
Subject: Re: [PATCH v20 08/20] mm: page_idle_get_page() does not need lru_lock

On Thu, Nov 05, 2020 at 12:52:05PM +0800, Alex Shi wrote:
> @@ -1054,8 +1054,27 @@ static void __page_set_anon_rmap(struct page *page,
>  	if (!exclusive)
>  		anon_vma = anon_vma->root;
>  
> +	/*
> +	 * w/o the WRITE_ONCE here the following scenario may happens due to
> +	 * store reordering.
> +	 *
> +	 *      CPU 0                                          CPU 1
> +	 *
> +	 * do_anonymous_page				page_idle_clear_pte_refs
> +	 *   __page_set_anon_rmap
> +	 *     page->mapping = anon_vma + PAGE_MAPPING_ANON
> +	 *   lru_cache_add_inactive_or_unevictable()
> +	 *     SetPageLRU(page)
> +	 *                                               rmap_walk
> +	 *                                                if PageAnon(page)
> +	 *
> +	 *  The 'SetPageLRU' may reordered before page->mapping setting, and
> +	 *  page->mapping may set with anon_vma, w/o anon bit, then rmap_walk
> +	 *  may goes to rmap_walk_file() for a anon page.
> +	 */
> +
>  	anon_vma = (void *) anon_vma + PAGE_MAPPING_ANON;
> -	page->mapping = (struct address_space *) anon_vma;
> +	WRITE_ONCE(page->mapping, (struct address_space *) anon_vma);
>  	page->index = linear_page_index(vma, address);
>  }

I don't like these verbose comments with detailed descriptions in
the source code.  They're fine in changelogs, but they clutter the
code, and they get outdated really quickly.  My preference is for
something more brief:

	/*
	 * Prevent page->mapping from pointing to an anon_vma without
	 * the PAGE_MAPPING_ANON bit set.  This could happen if the
	 * compiler stores anon_vma and then adds PAGE_MAPPING_ANON to it.
	 */