On 10/06, Srikar Dronamraju wrote: > > > > > for the future changes... (say, we can remove bp if consumers do not > > want to trace this task). Not sure it makes sense to change it right > > now. > > > > So. Should I leave this patch as is? Or do you want me to move this > > check into handler_chain() and make it return "bool restart"? > > Lets keep it as is for now. > > Acked-by: Srikar Dronamraju <[email protected]>
Thanks... But I am starting to think that I misunderstood your comment, you did not suggest to add this check into skip_sstep() as I wrongly thought. And yes, I agree it would be more clean to move it out from find_active_uprobe() and avoid put_uprobe && clear_swbp.... So how about v2 below? ------------------------------------------------------------------------------ [PATCH 4/7] uprobes: Fix handle_swbp() vs unregister() + register() race 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 handle_swbp(), 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]> Acked-by: Srikar Dronamraju <[email protected]> --- kernel/events/uprobes.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-) diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index cfa22c4..dbbca3a 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; } @@ -1436,6 +1437,14 @@ static void handle_swbp(struct pt_regs *regs) } return; } + /* + * 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 (unlikely(!(uprobe->flags & UPROBE_COPY_INSN))) + goto restart; utask = current->utask; if (!utask) { -- 1.5.5.1 -- 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/
