linux-kernel - Re: [PATCH] mm: Reuse only-pte-mapped KSM page in do_wp

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: <cfb2180d-8da8-6d99-44b8-ffc052869182@virtuozzo.com>
Date:   Sun, 30 Dec 2018 09:47:51 +0000
From:   Kirill Tkhai <ktkhai@...tuozzo.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
CC:     "kirill@...temov.name" <kirill@...temov.name>,
        "hughd@...gle.com" <hughd@...gle.com>,
        "aarcange@...hat.com" <aarcange@...hat.com>,
        "christian.koenig@....com" <christian.koenig@....com>,
        "imbrenda@...ux.vnet.ibm.com" <imbrenda@...ux.vnet.ibm.com>,
        "yang.shi@...ux.alibaba.com" <yang.shi@...ux.alibaba.com>,
        "riel@...riel.com" <riel@...riel.com>,
        "ying.huang@...el.com" <ying.huang@...el.com>,
        "minchan@...nel.org" <minchan@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>
Subject: Re: [PATCH] mm: Reuse only-pte-mapped KSM page in do_wp_page()

On 30.12.2018 00:40, Andrew Morton wrote:
> On Thu, 13 Dec 2018 18:29:08 +0300 Kirill Tkhai <ktkhai@...tuozzo.com> wrote:
> 
>> This patch adds an optimization for KSM pages almost
>> in the same way, that we have for ordinary anonymous
>> pages. If there is a write fault in a page, which is
>> mapped to an only pte, and it is not related to swap
>> cache; the page may be reused without copying its
>> content.
>>
>> [Note, that we do not consider PageSwapCache() pages
>>  at least for now, since we don't want to complicate
>>  __get_ksm_page(), which has nice optimization based
>>  on this (for the migration case). Currenly it is
>>  spinning on PageSwapCache() pages, waiting for when
>>  they have unfreezed counters (i.e., for the migration
>>  finish). But we don't want to make it also spinning
>>  on swap cache pages, which we try to reuse, since
>>  there is not a very high probability to reuse them.
>>  So, for now we do not consider PageSwapCache() pages
>>  at all.]
>>
>> So, in reuse_ksm_page() we check for 1)PageSwapCache()
>> and 2)page_stable_node(), to skip a page, which KSM
>> is currently trying to link to stable tree. Then we
>> do page_ref_freeze() to prohibit KSM to merge one more
>> page into the page, we are reusing. After that, nobody
>> can refer to the reusing page: KSM skips !PageSwapCache()
>> pages with zero refcount; and the protection against
>> of all other participants is the same as for reused
>> ordinary anon pages pte lock, page lock and mmap_sem.
>>
>> ...
>>
>> +bool reuse_ksm_page(struct page *page,
>> +		    struct vm_area_struct *vma,
>> +		    unsigned long address)
>> +{
>> +	VM_BUG_ON_PAGE(is_zero_pfn(page_to_pfn(page)), page);
>> +	VM_BUG_ON_PAGE(!page_mapped(page), page);
>> +	VM_BUG_ON_PAGE(!PageLocked(page), page);
>> +
>> +	if (PageSwapCache(page) || !page_stable_node(page))
>> +		return false;
>> +	/* Prohibit parallel get_ksm_page() */
>> +	if (!page_ref_freeze(page, 1))
>> +		return false;
>> +
>> +	page_move_anon_rmap(page, vma);
>> +	page->index = linear_page_index(vma, address);
>> +	page_ref_unfreeze(page, 1);
>> +
>> +	return true;
>> +}
> 
> Can we avoid those BUG_ON()s?
> 
> Something like this:
> 
> --- a/mm/ksm.c~mm-reuse-only-pte-mapped-ksm-page-in-do_wp_page-fix
> +++ a/mm/ksm.c
> @@ -2649,9 +2649,14 @@ bool reuse_ksm_page(struct page *page,
>  		    struct vm_area_struct *vma,
>  		    unsigned long address)
>  {
> -	VM_BUG_ON_PAGE(is_zero_pfn(page_to_pfn(page)), page);
> -	VM_BUG_ON_PAGE(!page_mapped(page), page);
> -	VM_BUG_ON_PAGE(!PageLocked(page), page);
> +#ifdef CONFIG_DEBUG_VM
> +	if (WARN_ON(is_zero_pfn(page_to_pfn(page))) ||
> +			WARN_ON(!page_mapped(page)) ||
> +			WARN_ON(!PageLocked(page))) {
> +		dump_page(page, "reuse_ksm_page");
> +		return false;
> +	}
> +#endif

Looks good!
  
>  	if (PageSwapCache(page) || !page_stable_node(page))
>  		return false;
> 
> We don't have a VM_WARN_ON_PAGE() and we can't provide one because the
> VM_foo() macros don't return a value.  It's irritating and I keep
> forgetting why we ended up doing them this way.
Thanks!