Date:   Wed, 13 Apr 2022 12:28:15 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Vlastimil Babka <vbabka@...e.cz>, linux-kernel@...r.kernel.org
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Hugh Dickins <hughd@...gle.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        David Rientjes <rientjes@...gle.com>,
        Shakeel Butt <shakeelb@...gle.com>,
        John Hubbard <jhubbard@...dia.com>,
        Jason Gunthorpe <jgg@...dia.com>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Yang Shi <shy828301@...il.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Matthew Wilcox <willy@...radead.org>,
        Jann Horn <jannh@...gle.com>, Michal Hocko <mhocko@...nel.org>,
        Nadav Amit <namit@...are.com>, Rik van Riel <riel@...riel.com>,
        Roman Gushchin <guro@...com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Peter Xu <peterx@...hat.com>,
        Donald Dutile <ddutile@...hat.com>,
        Christoph Hellwig <hch@....de>,
        Oleg Nesterov <oleg@...hat.com>, Jan Kara <jack@...e.cz>,
        Liang Zhang <zhangliang5@...wei.com>,
        Pedro Gomes <pedrodemargomes@...il.com>,
        Oded Gabbay <oded.gabbay@...il.com>, linux-mm@...ck.org
Subject: Re: [PATCH v3 11/16] mm/page-flags: reuse PG_mappedtodisk as
 PG_anon_exclusive for PageAnon() pages

On 13.04.22 10:25, Vlastimil Babka wrote:
> On 3/29/22 18:04, David Hildenbrand wrote:
>> The basic question we would like to have a reliable and efficient answer
>> to is: is this anonymous page exclusive to a single process or might it
>> be shared? We need that information for ordinary/single pages, hugetlb
>> pages, and possibly each subpage of a THP.
>>
>> Introduce a way to mark an anonymous page as exclusive, with the
>> ultimate goal of teaching our COW logic to not do "wrong COWs", whereby
>> GUP pins lose consistency with the pages mapped into the page table,
>> resulting in reported memory corruptions.
>>
>> Most pageflags already have semantics for anonymous pages, however,
>> PG_mappedtodisk should never apply to pages in the swapcache, so let's
>> reuse that flag.
>>
>> As PG_has_hwpoisoned also uses that flag on the second tail page of a
>> compound page, convert it to PG_error instead, which is marked as
>> PF_NO_TAIL, so never used for tail pages.
>>
>> Use custom page flag modification functions such that we can do
>> additional sanity checks. The semantics we'll put into some kernel doc
>> in the future are:
>>
>> "
>>   PG_anon_exclusive is *usually* only expressive in combination with a
>>   page table entry. Depending on the page table entry type it might
>>   store the following information:
>>
>>        Is what's mapped via this page table entry exclusive to the
>>        single process and can be mapped writable without further
>>        checks? If not, it might be shared and we might have to COW.
>>
>>   For now, we only expect PTE-mapped THPs to make use of
>>   PG_anon_exclusive in subpages. For other anonymous compound
>>   folios (i.e., hugetlb), only the head page is logically mapped and
>>   holds this information.
>>
>>   For example, an exclusive, PMD-mapped THP only has PG_anon_exclusive
>>   set on the head page. When replacing the PMD by a page table full
>>   of PTEs, PG_anon_exclusive, if set on the head page, will be set on
>>   all tail pages accordingly. Note that converting from a PTE-mapping
>>   to a PMD mapping using the same compound page is currently not
>>   possible and consequently doesn't require care.
>>
>>   If GUP wants to take a reliable pin (FOLL_PIN) on an anonymous page,
>>   it should only pin if the relevant PG_anon_bit is set. In that case,
> 
> 					^ PG_anon_exclusive bit ?
> 
>>   the pin will be fully reliable and stay consistent with the pages
>>   mapped into the page table, as the bit cannot get cleared (e.g., by
>>   fork(), KSM) while the page is pinned. For anonymous pages that
>>   are mapped R/W, PG_anon_exclusive can be assumed to always be set
>>   because such pages cannot possibly be shared.
>>
>>   The page table lock protecting the page table entry is the primary
>>   synchronization mechanism for PG_anon_exclusive; GUP-fast that does
>>   not take the PT lock needs special care when trying to clear the
>>   flag.
>>
>>   Page table entry types and PG_anon_exclusive:
>>   * Present: PG_anon_exclusive applies.
>>   * Swap: the information is lost. PG_anon_exclusive was cleared.
>>   * Migration: the entry holds this information instead.
>>                PG_anon_exclusive was cleared.
>>   * Device private: PG_anon_exclusive applies.
>>   * Device exclusive: PG_anon_exclusive applies.
>>   * HW Poison: PG_anon_exclusive is stale and not changed.
>>
>>   If the page may be pinned (FOLL_PIN), clearing PG_anon_exclusive is
>>   not allowed and the flag will stick around until the page is freed
>>   and folio->mapping is cleared.
> 
> Or also if it's unpinned?

I'm afraid I didn't get your question. Once the page is no longer
pinned, we can succeed in clearing PG_anon_exclusive (just like pinning
never happened). Does that answer your question?

> 
>> "
>>
>> We won't be clearing PG_anon_exclusive on destructive unmapping (i.e.,
>> zapping) of page table entries, page freeing code will handle that when
>> also invalidate page->mapping to not indicate PageAnon() anymore.
>> Letting information about exclusivity stick around will be an important
>> property when adding sanity checks to unpinning code.
>>
>> Note that we properly clear the flag in free_pages_prepare() via
>> PAGE_FLAGS_CHECK_AT_PREP for each individual subpage of a compound page,
>> so there is no need to manually clear the flag.
>>
>> Signed-off-by: David Hildenbrand <david@...hat.com>
> 
> Acked-by: Vlastimil Babka <vbabka@...e.cz>

Thanks!

> 
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -3663,6 +3663,17 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>>  		goto out_nomap;
>>  	}
>>  
>> +	/*
>> +	 * PG_anon_exclusive reuses PG_mappedtodisk for anon pages. A swap pte
>> +	 * must never point at an anonymous page in the swapcache that is
>> +	 * PG_anon_exclusive. Sanity check that this holds and especially, that
>> +	 * no filesystem set PG_mappedtodisk on a page in the swapcache. Sanity
>> +	 * check after taking the PT lock and making sure that nobody
>> +	 * concurrently faulted in this page and set PG_anon_exclusive.
>> +	 */
>> +	BUG_ON(!PageAnon(page) && PageMappedToDisk(page));
>> +	BUG_ON(PageAnon(page) && PageAnonExclusive(page));
>> +
> 
> Hmm, dunno why not VM_BUG_ON?

Getting PageAnonExclusive accidentally set by a file system would result
in an extremely unpleasant security issue. I most surely want to catch
something like that in any case, especially in the foreseeable future.

-- 
Thanks,

David / dhildenb

Powered by blists - more mailing lists