Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754055AbbELW2o (ORCPT ); Tue, 12 May 2015 18:28:44 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:33600 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751379AbbELW2l (ORCPT ); Tue, 12 May 2015 18:28:41 -0400 Date: Tue, 12 May 2015 15:28:40 -0700 From: Andrew Morton To: Vladimir Davydov Cc: , , "Paul E. McKenney" , "Kirill A. Shutemov" , Rik van Riel , Hugh Dickins Subject: Re: [PATCH v2] rmap: fix theoretical race between do_wp_page and shrink_active_list Message-Id: <20150512152840.20805775ae82c69b9a8f3028@linux-foundation.org> In-Reply-To: <1431425919-28057-1-git-send-email-vdavydov@parallels.com> References: <1431425919-28057-1-git-send-email-vdavydov@parallels.com> X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3004 Lines: 78 On Tue, 12 May 2015 13:18:39 +0300 Vladimir Davydov wrote: > As noted by Paul the compiler is free to store a temporary result in a > variable on stack, heap or global unless it is explicitly marked as > volatile, see: > > http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4455.html#sample-optimizations > > This can result in a race between do_wp_page() and shrink_active_list() > as follows. > > In do_wp_page() we can call page_move_anon_rmap(), which sets > page->mapping as follows: > > anon_vma = (void *) anon_vma + PAGE_MAPPING_ANON; > page->mapping = (struct address_space *) anon_vma; > > The page in question may be on an LRU list, because nowhere in > do_wp_page() we remove it from the list, neither do we take any LRU > related locks. Although the page is locked, shrink_active_list() can > still call page_referenced() on it concurrently, because the latter does > not require an anonymous page to be locked: > > CPU0 CPU1 > ---- ---- > do_wp_page shrink_active_list > lock_page page_referenced > PageAnon->yes, so skip trylock_page > page_move_anon_rmap > page->mapping = anon_vma > rmap_walk > PageAnon->no > rmap_walk_file > BUG > page->mapping += PAGE_MAPPING_ANON > > This patch fixes this race by explicitly forbidding the compiler to > split page->mapping store in page_move_anon_rmap() with the aid of > WRITE_ONCE. > > ... > > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -950,7 +950,7 @@ void page_move_anon_rmap(struct page *page, > VM_BUG_ON_PAGE(page->index != linear_page_index(vma, address), 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); Please let's not put things like WRITE_ONCE() in there without documenting them - otherwise it's terribly hard for readers to work out why it was added. How's this look? --- a/mm/rmap.c~rmap-fix-theoretical-race-between-do_wp_page-and-shrink_active_list-fix +++ a/mm/rmap.c @@ -950,6 +950,11 @@ void page_move_anon_rmap(struct page *pa VM_BUG_ON_PAGE(page->index != linear_page_index(vma, address), page); anon_vma = (void *) anon_vma + PAGE_MAPPING_ANON; + /* + * Ensure that anon_vma and the PAGE_MAPPING_ANON bit are written + * simultaneously, so a concurrent reader (eg shrink_active_list) will + * not see one without the other. + */ WRITE_ONCE(page->mapping, (struct address_space *) anon_vma); } _ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/