Re: [PATCH 12/X] uprobes: x86: introduce abort_xol()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hey Oleg,

> A separate "patch", just to emphasize that I do not know what
> actually abort_xol() should do! I do not understand this asm
> magic.
> 
> This patch simply changes regs->ip back to the probed insn,
> obviously this is not enough to handle UPROBES_FIX_*. Please
> take care.
> 
> If it is not clear, abort_xol() is needed when we should
> re-execute the original insn (replaced with int3), see the
> next patch.

We should be removing the breakpoint in abort_xol().
Otherwise if we just set the instruction pointer to int3 and signal a
sigill, then the user may be confused why a breakpoint is generating
SIGILL.

> ---
>  arch/x86/include/asm/uprobes.h |    1 +
>  arch/x86/kernel/uprobes.c      |    9 +++++++++
>  2 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> index f0fbdab..6209da1 100644
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -51,6 +51,7 @@ extern void set_instruction_pointer(struct pt_regs *regs, unsigned long vaddr);
>  extern int pre_xol(struct uprobe *uprobe, struct pt_regs *regs);
>  extern int post_xol(struct uprobe *uprobe, struct pt_regs *regs);
>  extern bool xol_was_trapped(struct task_struct *tsk);
> +extern void abort_xol(struct pt_regs *regs);
>  extern int uprobe_exception_notify(struct notifier_block *self,
>  				       unsigned long val, void *data);
>  #endif	/* _ASM_UPROBES_H */
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index c861c27..bc11a89 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -511,6 +511,15 @@ bool xol_was_trapped(struct task_struct *tsk)
>  	return false;
>  }
> 
> +void abort_xol(struct pt_regs *regs)
> +{
> +	// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
> +	// !!! Dear Srikar and Ananth, please implement me !!!
> +	// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
> +	struct uprobe_task *utask = current->utask;
> +	regs->ip = utask->vaddr;

nit:
Shouldnt we be setting the ip to the next instruction after this
instruction?

> +}
> +
>  /*
>   * Called after single-stepping. To avoid the SMP problems that can
>   * occur when we temporarily put back the original opcode to


I have applied all your patches and ran tests, the tests are all
passing.

I will fold them into my patches and send them out.

-- 
Thanks and Regards
Srikar

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>


[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]