lkml.org 
[lkml]   [2015]   [May]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] rmap: fix theoretical race between do_wp_page and shrink_active_list
Hello Andrew,

On Tue, May 12, 2015 at 03:28:40PM -0700, Andrew Morton wrote:
> 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

IMHO, rather than shrink_active_list, PageAnon in page_referenced is better to me.

--
Kind regards,
Minchan Kim


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