Messages in this thread Patch in this message | | | Date | Wed, 30 May 2012 23:34:09 +0530 | From | Srikar Dronamraju <> | Subject | Re: [PATCH 1/3] uprobes: install_breakpoint() should fail if is_swbp_insn() == T |
| |
> > So its the -EEXIST from set_swbp() that I was thinking about. I think I > was also wrong on the reason that that can happen. register's vma > iteration is very careful not to have the same vma twice, but it can > race against mmap() because of the uprobe_hash() vs uprobe_mmap_hash() > madness, right?
right.
> > Something like so? > > --- > kernel/events/uprobes.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index 985be4d..b4e749e 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -45,6 +45,19 @@ static DEFINE_SPINLOCK(uprobes_treelock); /* serialize rbtree access */ > > #define UPROBES_HASH_SZ 13 > > +/* > + * We need separate register/unregister and mmap/munmap lock hashes because of > + * mmap_sem nesting. > + * > + * {,un}egister_uprobe() needs to install probes on (potentially) all processes > + * and thus need to acquire multiple mmap_sems (consecutively, not > + * concurrently), whereas uprobe_m{,un}map() is called while holding mmap_sem > + * for the particular process doing the mmap. > + * > + * This all means that register_uprobe() can race with uprobe_mmap() and we > + * can try and install a probe where one is already installed. > + */
Nit: {,un}egister_uprobe should have been uprobe_{,un}register at couple of places.
> + > /* serialize (un)register */ > static struct mutex uprobes_mutex[UPROBES_HASH_SZ]; > > @@ -356,6 +369,9 @@ int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned > { > int result; > > + /* > + * See the comment near uprobes_hash(). > + */ > result = is_swbp_at_addr(mm, vaddr); > if (result == 1) > return -EEXIST; >
| |