lkml.org 
[lkml]   [2012]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 4/7] uprobes: Fix handle_swbp() vs unregister() + register() race
* Oleg Nesterov <oleg@redhat.com> [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 <oleg@redhat.com>
> ---
> 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



\
 
 \ /
  Last update: 2012-10-06 12:21    [W:0.271 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site