Messages in this thread Patch in this message | | | Date | Fri, 9 Apr 2010 13:56:57 +0900 | From | KAMEZAWA Hiroyuki <> | Subject | Re: [PATCH 02/13] mm: Revalidate anon_vma in page_lock_anon_vma() |
| |
On Fri, 9 Apr 2010 13:16:41 +1000 Nick Piggin <npiggin@suse.de> wrote:
> On Thu, Apr 08, 2010 at 09:17:39PM +0200, Peter Zijlstra wrote: > > There is nothing preventing the anon_vma from being detached while we > > are spinning to acquire the lock. Most (all?) current users end up > > calling something like vma_address(page, vma) on it, which has a > > fairly good chance of weeding out wonky vmas. > > > > However suppose the anon_vma got freed and re-used while we were > > waiting to acquire the lock, and the new anon_vma fits with the > > page->index (because that is the only thing vma_address() uses to > > determine if the page fits in a particular vma, we could end up > > traversing faulty anon_vma chains. > > > > Close this hole for good by re-validating that page->mapping still > > holds the very same anon_vma pointer after we acquire the lock, if not > > be utterly paranoid and retry the whole operation (which will very > > likely bail, because it's unlikely the page got attached to a different > > anon_vma in the meantime). > > Hm, looks like a bugfix? How was this supposed to be safe? > IIUC.
Before Rik's change to anon_vma, once page->mapping is set as anon_vma | 0x1, it's not modified until the page is freed. After the patch, do_wp_page() overwrite page->mapping when it reuse existing page.
== static int do_wp_page(struct mm_struct *mm, struct vm_area_struct *vma, unsigned long address, pte_t *page_table, pmd_t *pmd, spinlock_t *ptl, pte_t orig_pte) { .... if (PageAnon(old_page) && !PageKsm(old_page)) { if (!trylock_page(old_page)) { page_cache_get(old_page); .... reuse = reuse_swap_page(old_page); if (reuse) /* * The page is all ours. Move it to our anon_vma so * the rmap code will not search our parent or siblings. * Protected against the rmap code by the page lock. */ page_move_anon_rmap(old_page, vma, address); ----(*) } === (*) is new.
Then, this new check makes sense in the current kernel.
> > > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> > > Cc: Hugh Dickins <hugh.dickins@tiscali.co.uk> > > Cc: Linus Torvalds <torvalds@linux-foundation.org> > > --- > > mm/rmap.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > Index: linux-2.6/mm/rmap.c > > =================================================================== > > --- linux-2.6.orig/mm/rmap.c > > +++ linux-2.6/mm/rmap.c > > @@ -294,6 +294,7 @@ struct anon_vma *page_lock_anon_vma(stru > > unsigned long anon_mapping; > > > > rcu_read_lock(); > > +again: > > anon_mapping = (unsigned long) ACCESS_ONCE(page->mapping); > > if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON) > > goto out; > > @@ -302,6 +303,12 @@ struct anon_vma *page_lock_anon_vma(stru > > > > anon_vma = (struct anon_vma *) (anon_mapping - PAGE_MAPPING_ANON); > > spin_lock(&anon_vma->lock); > > + > > + if (page_rmapping(page) != anon_vma) { > > very unlikely()? > I think so.
Thanks, -Kame
| |