lkml.org 
[lkml]   [2015]   [May]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v2] rmap: fix theoretical race between do_wp_page and shrink_active_list
On Tue, 12 May 2015 13:18:39 +0300 Vladimir Davydov <vdavydov@parallels.com> 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);
}

_


\
 
 \ /
  Last update: 2015-05-13 00:41    [W:0.048 / U:0.900 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site