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 <v...@zeniv.linux.org.uk>
---
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
