Messages in this thread | | | Date | Sat, 6 Oct 2012 19:25:31 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 4/7] uprobes: Fix handle_swbp() vs unregister() + register() race |
| |
On 10/06, Srikar Dronamraju wrote: > > * Oleg Nesterov <oleg@redhat.com> [2012-09-30 21:42:11]: > > > @@ -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.
handle_swbp:
if (can_skip_sstep(uprobe, regs)) goto out;
but if we hit a non-UPROBE_COPY_INSN uprobe, we need "goto restart".
> Also it will probably be a little cleaner, (Not having to drop a uprobe > reference, not having to reset is_swbp.)
We can change handler_chain() (which also checks UPROBE_RUN_HANDLER) to return "bool restart", not sure this will be more clean.
And if we change handler_chain(), I think it should return bitmask,
for (uc = uprobe->consumers; uc; uc = uc->next) ret |= uc->handler(...);
return ret;
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"?
Oleg.
| |