On Mon, Jul 30, 2012 at 04:16:38PM +0200, Oleg Nesterov wrote:
> On 07/30, Ananth N Mavinakayanahalli wrote:
> >
> > On Thu, Jul 26, 2012 at 05:20:43PM +0200, Sebastian Andrzej Siewior wrote:
> > > If someone is using single stepping over uprobe brackpoint then after
> > > we pass the uprobe single step, single stepping is disabled and the user
> > > who enebaled them in the first place does not know anything about this.
> > >
> > > This patch avoids enabling / disabling the single step mode if it is
> > > already enabled.
> >
> > This could happen any time 2 different entities call the
> > user_(en/dis)able_single_step() helpers on the same thread.
>
> Yes. But nobody except ptrace should do use these helpers, I think.
Right now, yes.
> > Wouldn't the right way to fix it be to teach these helpers
> > to honor what the TIF_SINGLESTEP
>
> Well, I think uprobes should not use TIF_SINGLESTEP at all. This
> bit is (mostly) needed to handle the stepping over syscall. But
> I guess you didn't actually mean TIF_SINGLESTEP...
>
> > flag setting was in the first place?
>
> Perhaps, but I don't think so. If nothing else, we do not want
> to add the new counter/whatever in task_struct, while uprobes
> already has uprobe_task which can "remember" the state of _TF
> bit and more.
>
> And this can't solve other problems. Suppose that gdb does
> PTRACE_SINGLESTEP but the original "popf" insn was already replaced
> by "int3", this will obviously confuse is_setting_trap_flag().
>
> And we need the additional SIGTRAP from handle_singlestep().
> And we have more problems with DEBUGCTLMSR_BTF. And we do
> not want access_process_vm() from uprobes code.
IIUC you'd want uprobes to do similar to what kprobes does today (see
prepare_singlestep() in arch/xxx/kernel/kprobes.c).
> So I think we need arch_uprobe_*able_step(struct uprobe_task *utask).
> Ignoring all problems except the one this patch tries to fix, x86
> can simply do:
>
> arch_uprobe_enble_step(utask, struct arch_uprobe *auprobe)
> {
> utask->clear_tf =
> !(regs->flags & X86_EFLAGS_TF) &&
> (auprobe->insn != "popf");
> regs->flags |= X86_EFLAGS_TF;
> }
>
> arch_uprobe_disable_step(utask)
> {
> if (utask->clear_tf)
> regs->flags &= ~X86_EFLAGS_TF;
> }
Right.
> However. This all needs more discussion (and help from Roland I guess).
>
> Sebastian, I think your patch is simple and certainly makes the things
> better, just it is not correct (you already realized you can't use
> uprobe->flags) and it is not arch-friendly.
>
> I'd suggest you to make 2 patches:
>
> - 1/2 creates arch_uprobe_*_step(...) __weak helpers in
> kernel/events/uprobes.c which simply call
> user_*_single_step() and updates the callers
>
> Not strictly necessary, but imho makes sense...
>
> - 2/2 adds the x86 implementation in arch/x86/kernel/uprobes.c
> which still uses user_*_single_step() but checks
> TIF_SINGLESTEP. As your patch does, but you should use
> utask, not uprobe.
>
> IOW, I simply suggest to make your patch x86-specific. Then we
> will try to do more fixes/improvements.
>
>
> Sebastian, Ananth, what do you think?
Agreed.
Ananth
--
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/
