racing anon_vma_prepares

racing anon_vma_prepares

[Hugh Dickins]

Just noticed that you rely on mmap_sem to protect anon_vma_prepare:
but it doesn't, since concurrent faults can both down_read(&mmap_sem).
I think the anon_vma_alloc should be done where you have anon_vma_prepare,
but some kind of set_anon_vma under page_table_lock to set or free it.

Hugh

Re: racing anon_vma_prepares

[Andrea Arcangeli]

On Tue, Apr 06, 2004 at 05:40:50PM +0100, Hugh Dickins wrote:
> Just noticed that you rely on mmap_sem to protect anon_vma_prepare:
> but it doesn't, since concurrent faults can both down_read(&mmap_sem).
> I think the anon_vma_alloc should be done where you have anon_vma_prepare,
> but some kind of set_anon_vma under page_table_lock to set or free it.

great spotting, I'm grateful you found such a race. It could never be
noticed with testing. it definitely needs the page_table_lock to
serialize (I was biased by the vma merging that happens with the
down_write I guess ;).  Note that in the common case anon_vma_prepare
will do nothing, so I believe this fix should be already close optimal
(I know we could save a few cycles in the unlikely case by doing
something more than just anon_vma_prepare but that would render memory.c
more complicated, so it's doable but it's a lowpriority matter and I'd
be interested in having a smallest possible fix at the moment for
pratical reasons). later on we can optimize it further.

comments?

--- x/mm/objrmap.c.~1~	2004-04-06 18:53:49.987876768 +0200
+++ x/mm/objrmap.c	2004-04-06 19:04:49.190662712 +0200
@@ -725,15 +725,31 @@ int fastcall anon_vma_prepare(struct vm_
 	anon_vma_t * anon_vma = vma->anon_vma;
 
 	might_sleep();
-	if (!anon_vma) {
+	if (unlikely(!anon_vma)) {
+		struct mm_struct * mm;
+
 		anon_vma = anon_vma_alloc();
 		if (!anon_vma)
 			return -ENOMEM;
+
+		mm = vma->vm_mm;
+		spin_lock(&mm->page_table_lock);
+		if (unlikely(vma->anon_vma))
+			goto out_unlock_free;
+
 		vma->anon_vma = anon_vma;
-		/* mmap_sem to protect against threads is enough */
+		/* page_table_lock to protect against threads is enough */
 		list_add(&vma->anon_vma_node, &anon_vma->anon_vma_head);
+
+		spin_unlock(&mm->page_table_lock);
 	}
+ out:
 	return 0;
+
+ out_unlock_free:
+	spin_unlock(&vma->vm_mm->page_table_lock);
+	anon_vma_free(anon_vma);
+	goto out;
 }
 
 void fastcall anon_vma_merge(struct vm_area_struct * vma,

Re: racing anon_vma_prepares

[Hugh Dickins]

On Tue, 6 Apr 2004, Andrea Arcangeli wrote:
> (I know we could save a few cycles in the unlikely case by doing
> something more than just anon_vma_prepare but that would render memory.c
> more complicated, so it's doable but it's a lowpriority matter and I'd
> be interested in having a smallest possible fix at the moment for
> pratical reasons). later on we can optimize it further.

Understood.  Yes, looks okay.  Mainline can improve on it later.

Hugh

Re: racing anon_vma_prepares

[Andrea Arcangeli]

On Tue, Apr 06, 2004 at 06:28:04PM +0100, Hugh Dickins wrote:
> On Tue, 6 Apr 2004, Andrea Arcangeli wrote:
> > (I know we could save a few cycles in the unlikely case by doing
> > something more than just anon_vma_prepare but that would render memory.c
> > more complicated, so it's doable but it's a lowpriority matter and I'd
> > be interested in having a smallest possible fix at the moment for
> > pratical reasons). later on we can optimize it further.
> 
> Understood.  Yes, looks okay.  Mainline can improve on it later.

thanks for the review, I appreciate.