Re: [PATCH v4 3.0-rc2-tip 7/22] 7: uprobes: mmap and fork hooks.

[Peter Zijlstra]

On Thu, 2011-06-16 at 08:56 +0530, Srikar Dronamraju wrote: 
> * Peter Zijlstra <peterz@xxxxxxxxxxxxx> [2011-06-15 20:11:26]:
> 
> > On Tue, 2011-06-07 at 18:29 +0530, Srikar Dronamraju wrote:
> > > +       up_write(&mm->mmap_sem);
> > > +       mutex_lock(&uprobes_mutex);
> > > +       down_read(&mm->mmap_sem); 
> > 
> > egads, and all that without a comment explaining why you think that is
> > even remotely sane.
> > 
> > I'm not at all convinced, it would expose the mmap() even though you
> > could still decide to tear it down if this function were to fail, I bet
> > there's some funnies there.
> 
> The problem is with lock ordering.  register/unregister operations
> acquire uprobes_mutex (which serializes register unregister and the
> mmap_hook) and then holds mmap_sem for read before they insert a
> breakpoint.
> 
> But the mmap hook would be called with mmap_sem held for write. So
> acquiring uprobes_mutex can result in deadlock. Hence we release the
> mmap_sem, take the uprobes_mutex and then again hold the mmap_sem.

Sure, I saw why you wanted to do it, I'm just not quite convinced its
safe to do and something like this definitely wants a comment explaining
why its safe to drop mmap_sem.  

> After we re-acquire the mmap_sem, we do check if the vma is valid.

But you don't on the return path, and if !ret
mmap_region():unmap_and_free_vma will be touching vma again to remove
it.

> Do we have better solutions?

/me kicks the brain into gear and walks off to get a fresh cup of tea.

So the reason we take uprobes_mutex there is to avoid probes from going
away while you're installing them, right?

So we start by doing this add_to_temp_list() thing (horrid name), which
iterates the probes on this inode under uprobes_treelock and adds them
to a list.

Then we iterate the list, installing the probles.

How about we make the initial pass under uprobes_treelock take a
references on the probe, and then after install_breakpoint() succeeds we
again take uprobes_treelock and validate the uprobe still exists in the
tree and drop the extra reference, if not we simply remove the
breakpoint again and continue like it never existed.

That should avoid the need to take uprobes_mutex and not require
dropping mmap_sem, right?
--
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/