rmap: fix theoretical race between do_wp_page and shrink_active_list

PATCH Minchan Kim 2 participants 2 messages May 13, 2015 Memory management

lore

Share & tools

lore

Minchan Kim

author

2015-05-13 01:44 UTC #0 lore raw

Hi, Rik

I'd like to bring up the issue in this thread although I already gave my Acked-by.

Below issue causes by no PG_locked page in page_referenced while page_move_anon_rmap depends on PG_locked to prevent race with rmap code.

So, although this patch fixes below one example, we still have a problem in rmap.

If page_referenced holds PG_locked for all of pages unconditionally, we don't need this patch and might remove READ_ONCE introduced by 80e148 and more than.

What do you think about?

Quoted 72 lines · Vladimir Davydov

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.
Signed-off-by Vladimir Davydov <[email protected]>
Cc "Paul E. McKenney" <[email protected]>
Cc "Kirill A. Shutemov" <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Hugh Dickins <[email protected]>
---
Changes in v2:
- do not add READ_ONCE to PageAnon and WRITE_ONCE to __page_set_anon_rmap and __hugepage_set_anon_rmap (Kirill)
 mm/rmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/rmap.c +1 -1
index 24dd3f9fee27..8b18fd4227d1 100644
@@ -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);
 }
 
 /**
Signature · 7 lines
1.7.10.4

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

Signature · 8 lines

Kind regards,
Minchan Kim

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

Rik van Riel

Acked-by

2015-05-13 02:04 UTC #1 lore raw

Quoted 8 lines · Minchan Kim

Hi, Rik
I'd like to bring up the issue in this thread although I already gave my Acked-by.
Below issue causes by no PG_locked page in page_referenced while page_move_anon_rmap depends on PG_locked to prevent race with rmap code.

Quoted 8 lines

So, although this patch fixes below one example, we still have a problem in rmap.
If page_referenced holds PG_locked for all of pages unconditionally, we don't need this patch and might remove READ_ONCE introduced by 80e148 and more than.
What do you think about?

Maybe the reclaim code and page_referenced are fine.

However, I have seen one real world bug report of a page->mapping pointing to an anon_vma without the PAGE_MAPPING_ANON bit being set.

This is a pretty hard to hit race, so I have only ever heard of it happening once, and I do not remember the details of exactly what code blew up trying to follow the page->mapping pointer in the wrong way.

I wish I remember what needs this patch, but I have a rather strong suspicion there is something that needs it...

Acked-by Rik van Riel <[email protected]>

Quoted 74 lines · Vladimir Davydov

Quoted 72 lines
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.
Signed-off-by Vladimir Davydov <[email protected]>
Cc "Paul E. McKenney" <[email protected]>
Cc "Kirill A. Shutemov" <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Hugh Dickins <[email protected]>
---
Changes in v2:
- do not add READ_ONCE to PageAnon and WRITE_ONCE to __page_set_anon_rmap and __hugepage_set_anon_rmap (Kirill)
 mm/rmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/rmap.c +1 -1
index 24dd3f9fee27..8b18fd4227d1 100644
@@ -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);
 }
 
 /**
Signature · 7 lines
1.7.10.4

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

Signature · 7 lines

All rights reversed

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