* Oleg Nesterov <o...@redhat.com> [2012-10-06 20:53:37]:
> 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 <sri...@linux.vnet.ibm.com>
>
> 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?
Yes, this is what I meant. Thanks for the relooking into it.
This will mean, change in one hunk in patch 7/7.
Acked-by: Srikar Dronamraju <sri...@linux.vnet.ibm.com>
>
> ------------------------------------------------------------------------------
> [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 <o...@redhat.com>
> Acked-by: Srikar Dronamraju <sri...@linux.vnet.ibm.com>
> ---
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
