On Fri, 2014-04-04 at 20:51 +0200, Oleg Nesterov wrote:
> SIGILL after the failed arch_uprobe_post_xol() should only be used as
> a last resort, we should try to restart the probed insn if possible.
>
> Currently only adjust_ret_addr() can fail, and this can only happen if
> another thread unmapped our stack after we executed "call" out-of-line.
> Most probably the application if buggy, but even in this case it can
> have a handler for SIGSEGV/etc. And in theory it can be even correct
> and do something non-trivial with its memory.
>
> Of course we can't restart unconditionally, so arch_uprobe_post_xol()
> does this only if ->post_xol() returns -ERESTART even if currently this
> is the only possible error.
When re-executing the call instruction, I'd think the stack pointer
would be wrong the second time around, unless you pop off the return
address from the first try.
>
> Note: this is not "perfect", we do not want the extra handler_chain()
> after restart, but I think this is the best solution we can realistically
> do without too much uglifications.
>
> Signed-off-by: Oleg Nesterov <o...@redhat.com>
> ---
> arch/x86/kernel/uprobes.c | 15 +++++++++++----
> 1 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index e72903e..b820668 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -443,16 +443,17 @@ static int default_post_xol_op(struct arch_uprobe
> *auprobe, struct pt_regs *regs
> {
> struct uprobe_task *utask = current->utask;
> long correction = (long)(utask->vaddr - utask->xol_vaddr);
> - int ret = 0;
>
> handle_riprel_post_xol(auprobe, regs, &correction);
> if (auprobe->fixups & UPROBE_FIX_IP)
> regs->ip += correction;
>
> - if (auprobe->fixups & UPROBE_FIX_CALL)
> - ret = adjust_ret_addr(regs->sp, correction);
> + if (auprobe->fixups & UPROBE_FIX_CALL) {
> + if (adjust_ret_addr(regs->sp, correction))
> + return -ERESTART;
> + }
>
> - return ret;
> + return 0;
> }
>
> static struct uprobe_xol_ops default_xol_ops = {
> @@ -599,6 +600,12 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe,
> struct pt_regs *regs)
> int err = auprobe->ops->post_xol(auprobe, regs);
> if (err) {
> arch_uprobe_abort_xol(auprobe, regs);
> + /*
> + * Restart the probed insn. ->post_xol() must ensure
> + * this is really possible if it returns -ERESTART.
> + */
> + if (err == -ERESTART)
> + return 0;
> return err;
> }
> }
--
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/
