Re: [PATCH] mm: Downgrade mmap_sem before locking or populating onmmap

[Al Viro]

On Sun, Dec 16, 2012 at 05:04:03PM +0000, Al Viro wrote:

> FWIW, I've done some checking of ->mmap_sem uses yesterday.  Got further than
> the last time; catch so far, just from find_vma() audit:
> * arm swp_emulate.c - missing ->mmap_sem around find_vma().  Fix sent to
> rmk.
> * blackfin ptrace - find_vma() without any protection, definitely broken
> * m68k sys_cacheflush() - ditto
> * mips process_fpemu_return() - ditto
> * mips octeon_flush_cache_sigtramp() - ditto
> * omap_vout_uservirt_to_phys() - ditto, patch sent
> * vb2_get_contig_userptr() - probaly a bug, unless I've misread the (very
> twisty maze of) v4l2 code leading to it
> * vb2_get_contig_userptr() - ditto
> * gntdev_ioctl_get_offset_for_vaddr() - definitely broken
> and there's a couple of dubious places in arch/* I hadn't finished with,
> plus a lot in mm/* proper.
> 
> That's just from a couple of days of RTFS.  The locking in there is far too
> convoluted as it is; worse, it's not localized code-wise, so rechecking
> correctness is going to remain a big time-sink ;-/
> 
> Making it *more* complex doesn't look like a good idea, TBH...

While we are at it: fs/proc/task_nommu.c:m_stop() is fucked.  It assumes
that the process in question hadn't done execve() since m_start().  And
it doesn't hold anywhere near enough locks to guarantee that.  task_mmu.c
counterpart avoids that fun by using ->vm_mm to get to mm_struct in question.
Completely untested patch follows; if it works, that's -stable fodder.

Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
---
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 1ccfa53..dc26605 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -211,6 +211,14 @@ static int show_tid_map(struct seq_file *m, void *_p)
 	return show_map(m, _p, 0);
 }
 
+static void stop_it(void *_p)
+{
+	struct vm_area_struct *vma = rb_entry(_p, struct vm_area_struct, vm_rb);
+	struct mm_struct *mm = vma->vm_mm;
+	up_read(&mm->mmap_sem);
+	mmput(mm);
+}
+
 static void *m_start(struct seq_file *m, loff_t *pos)
 {
 	struct proc_maps_private *priv = m->private;
@@ -235,6 +243,8 @@ static void *m_start(struct seq_file *m, loff_t *pos)
 	for (p = rb_first(&mm->mm_rb); p; p = rb_next(p))
 		if (n-- == 0)
 			return p;
+	up_read(&mm->mmap_sem);
+	mmput(mm);
 	return NULL;
 }
 
@@ -243,9 +253,8 @@ static void m_stop(struct seq_file *m, void *_vml)
 	struct proc_maps_private *priv = m->private;
 
 	if (priv->task) {
-		struct mm_struct *mm = priv->task->mm;
-		up_read(&mm->mmap_sem);
-		mmput(mm);
+		if (_vml)
+			stop_it(_vml);
 		put_task_struct(priv->task);
 	}
 }
@@ -255,7 +264,12 @@ static void *m_next(struct seq_file *m, void *_p, loff_t *pos)
 	struct rb_node *p = _p;
 
 	(*pos)++;
-	return p ? rb_next(p) : NULL;
+	if (!p)
+		return NULL;
+	p = rb_next(p);
+	if (!p)
+		stop_it(_p);
+	return p;
 }
 
 static const struct seq_operations proc_pid_maps_ops = {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/