Re: A small window for a race condition in mm/rmap.c:page_lock_anon_vma_read

[Peter Zijlstra]

On Wed, Dec 21, 2016 at 03:43:43PM +0100, Michal Hocko wrote:
> anon_vma locking is clever^Wsubtle as hell. CC Peter...
> 
> On Tue 20-12-16 09:32:27, Dashi DS1 Cao wrote:
> > I've collected four crash dumps with similar backtrace. 
> > 
> > PID: 247    TASK: ffff881fcfad8000  CPU: 14  COMMAND: "kswapd1"
> >  #0 [ffff881fcfad7978] machine_kexec at ffffffff81051e9b
> >  #1 [ffff881fcfad79d8] crash_kexec at ffffffff810f27e2
> >  #2 [ffff881fcfad7aa8] oops_end at ffffffff8163f448
> >  #3 [ffff881fcfad7ad0] die at ffffffff8101859b
> >  #4 [ffff881fcfad7b00] do_general_protection at ffffffff8163ed3e
> >  #5 [ffff881fcfad7b30] general_protection at ffffffff8163e5e8
> >     [exception RIP: down_read_trylock+9]
> >     RIP: ffffffff810aa9f9  RSP: ffff881fcfad7be0  RFLAGS: 00010286
> >     RAX: 0000000000000000  RBX: ffff882b47ddadc0  RCX: 0000000000000000
> >     RDX: 0000000000000000  RSI: 0000000000000000  RDI: 91550b2b32f5a3e8
> 
> rdi is obviously a mess - smells like a string. So either sombody has
> overwritten root_anon_vma or this is really a use after free...

e8 - ï
a3 - ï
f5 - ï
32 - 2
2b - +
 b - 

55 - U
91 - ï

Not a string..

> >     RBP: ffff881fcfad7be0   R8: ffffea00ecc28860   R9: ffff883fcffeae28
> >     R10: ffffffff81a691a0  R11: 0000000000000001  R12: ffff882b47ddadc1
> >     R13: ffffea00ecc28840  R14: 91550b2b32f5a3e8  R15: ffffea00ecc28840
> >     ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0000
> >  #6 [ffff881fcfad7be8] page_lock_anon_vma_read at ffffffff811a3365
> >  #7 [ffff881fcfad7c18] page_referenced at ffffffff811a35e7
> >  #8 [ffff881fcfad7c90] shrink_active_list at ffffffff8117e8cc
> >  #9 [ffff881fcfad7d48] balance_pgdat at ffffffff81180288
> > #10 [ffff881fcfad7e20] kswapd at ffffffff81180813
> > #11 [ffff881fcfad7ec8] kthread at ffffffff810a5b8f
> > #12 [ffff881fcfad7f50] ret_from_fork at ffffffff81646a98
> > 
> > I suspect my customer hits into a small window of a race condition in mm/rmap.c: page_lock_anon_vma_read.
> > struct anon_vma *page_lock_anon_vma_read(struct page *page)
> > {
> >         struct anon_vma *anon_vma = NULL;
> >         struct anon_vma *root_anon_vma;
> >         unsigned long anon_mapping;
> > 
> >         rcu_read_lock();
> >         anon_mapping = (unsigned long)READ_ONCE(page->mapping);
> >         if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON)
> >                 goto out;
> >         if (!page_mapped(page))
> >                 goto out;
> > 
> >         anon_vma = (struct anon_vma *) (anon_mapping - PAGE_MAPPING_ANON);
> >         root_anon_vma = READ_ONCE(anon_vma->root);
> 
> Could you dump the anon_vma and struct page as well?
> 
> >         if (down_read_trylock(&root_anon_vma->rwsem)) {
> >                 /*
> >                  * If the page is still mapped, then this anon_vma is still
> >                  * its anon_vma, and holding the mutex ensures that it will
> >                  * not go away, see anon_vma_free().
> >                  */
> >                 if (!page_mapped(page)) {
> >                         up_read(&root_anon_vma->rwsem);
> >                         anon_vma = NULL;
> >                 }
> >                 goto out;
> >         }
> > ...
> > }
> > 
> > Between the time the two "page_mapped(page)" are checked, the address
> > (anon_mapping - PAGE_MAPPING_ANON) is unmapped! However it seems
> > that anon_vma->root could still be read in but the value is wild. So
> > the kernel crashes in down_read_trylock. But it's weird that all the
> > "struct page" has its member "_mapcount" still with value 0, not -1,
> > in the four crashes.

So the point is that while we hold rcu_read_lock() the actual memory
backing the anon_vmas cannot be freed. It can be reused, but only for
another anon_vma.

Now, anon_vma_alloc() sets ->root to self, while anon_vma_free() leaves
->root set to whatever. And any other ->root assignment is to a valid
anon_vma.

Therefore, the same rules that ensure anon_vma stays valid, should also
ensure anon_vma->root stays valid.

Now, one thing that might go wobbly is that ->root assignments are not
done using WRITE_ONCE(), this means a naughty compiler can miscompile
those stores and introduce store-tearing, if our READ_ONCE() would
observe such a tear, we'd be up some creek without a paddle.


Now, its been a long time since I looked at any of this code, and I see
that Hugh has fixed at least two wobblies in my original code.