* Oleg Nesterov <[email protected]> [2012-09-30 21:42:11]: > Strictly speaking this race was added by me in 56bb4cf6. However > I think that this bug is just another indication that we should > move copy_insn/uprobe_analyze_insn code from install_breakpoint() > to uprobe_register(), there are a lot of other reasons for that. > Until then, add a hack to close the race. > > A task can hit uprobe U1, but before it calls find_uprobe() this > uprobe can be unregistered *AND* another uprobe U2 can be added to > uprobes_tree at the same inode/offset. In this case handle_swbp() > will use the not-fully-initialized U2, in particular its arch.insn > for xol. > > Add the additional !UPROBE_COPY_INSN check into find_active_uprobe, > if this flag is not set we simply restart as if the new uprobe was > not inserted yet. This is not very nice, we need barriers, but we > will remove this hack when we change uprobe_register(). > > Note: with or without this patch install_breakpoint() can race with > itself, yet another reson to kill UPROBE_COPY_INSN altogether. And > even the usage of uprobe->flags is not safe. See the next patches. > > Signed-off-by: Oleg Nesterov <[email protected]> > --- > kernel/events/uprobes.c | 11 +++++++++++ > 1 files changed, 11 insertions(+), 0 deletions(-) > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index 6058231..a81080f 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -596,6 +596,7 @@ install_breakpoint(struct uprobe *uprobe, struct > mm_struct *mm, > BUG_ON((uprobe->offset & ~PAGE_MASK) + > UPROBE_SWBP_INSN_SIZE > PAGE_SIZE); > > + smp_wmb(); /* pairs with rmb() in find_active_uprobe() */ > uprobe->flags |= UPROBE_COPY_INSN; > } > > @@ -1391,6 +1392,16 @@ static struct uprobe *find_active_uprobe(unsigned long > bp_vaddr, int *is_swbp) > if (!uprobe && test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags)) > mmf_recalc_uprobes(mm); > up_read(&mm->mmap_sem); > + /* > + * TODO: move copy_insn/etc into _register and remove this hack. > + * After we hit the bp, _unregister + _register can install the > + * new and not-yet-analyzed uprobe at the same address, restart. > + */ > + smp_rmb(); /* pairs with wmb() in install_breakpoint() */ > + if (uprobe && unlikely(!(uprobe->flags & UPROBE_COPY_INSN))) { > + uprobe = NULL; > + *is_swbp = 0; > + } > > return uprobe; > }
Should we be adding this check handle_swbp() around can_skip_step()? The earliest we access arch.insn is in can_skip_step. So we give some more time for the instruction to be copied. Also it will probably be a little cleaner, (Not having to drop a uprobe reference, not having to reset is_swbp.) -- Thanks and Regards Srikar -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
