Re: [PATCH v5 3.1.0-rc4-tip 4/26] uprobes: Define hooks formmap/munmap.

[Srikar Dronamraju]

> > >
> > > So. We are adding the new mapping, we should find all breakpoints this
> > > file has in the start/end range.
> > >
> > > We are holding ->mmap_sem... this seems enough to protect against the
> > > races with register/unregister. Except, what if __register_uprobe()
> > > fails? In this case __unregister_uprobe() does delete_uprobe() at the
> > > very end. What if mmap mmap_uprobe() is called right before delete_?
> > >
> >
> > Because consumers would be NULL before _unregister_uprobe kicks in, we
> > shouldnt have a problem here.
> 
> Hmm. But it is not NULL.
> 
> Once again, I didn't mean unregister_uprobe(). I meant register_uprobe().
> In this case, if __register_uprobe() fails, we are doing __unregister
> but uprobe->consumer != NULL.

Oh Okay, I missed setting uprobe->consumer = NULL once __register_uprobe
fails.
I shall go ahead and set uprobe->consumer = NULL; (the other option is
calling del_consumer() but I dont see a need for calling this.) just
before calling __unregister_uprobe() if and only if __register_uprobe
fails.

> 
> Just suppose that the caller of register_uprobe() gets a (long) preemption
> right before __unregister_uprobe()->delete_uprobe(). What if mmap() is
> called at this time?
> 
> > Am I missing something?
> 
> May be you, may be me. Please recheck ;)

Rechecked and found the issue. Thanks.

> 
> > I think this would be taken care of if we move the munmap_uprobe() hook
> > from unmap_vmas to unlink_file_vma().
> 
> Probably yes, we should rely on prio_tree locking/changes.
> 
> > The other thing that I need to investigate a bit more is if I have
> > handle all cases of mremap correctly.
> 
> Yes. May be mmap_uprobe() should be "closer" to vma_prio_tree_add/insert
> too, but I am not sure.

Okay, that seems like a good idea.

> 
> Oleg.
> 

-- 
Thanks and Regards
Srikar
--
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/