Re: [PATCH v2] rmap: fix theoretical race between do_wp_page and shrink_active_list

[Minchan Kim <minchan@kernel.org>]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>