Re: [RFC 8/8] ARM64: Add uprobe support

From: Oleg Nesterov
Date: Fri Jan 02 2015 - 12:25:06 EST


Hi Pratyush,

I'll try to actually read this patch (and the whole series) later, just
a couple of quick questions for now.

On 12/31, Pratyush Anand wrote:
>
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -205,6 +205,7 @@ static inline int valid_user_regs(struct user_pt_regs *regs)
>
> #define instruction_pointer(regs) ((regs)->pc)
> #define stack_pointer(regs) ((regs)->sp)
> +#define procedure_link_pointer(regs) ((regs)->regs[30])

perhaps it makes sense to change (at least) arch_prepare_kretprobe() to use
the new helper? OK, we can do this later.

> +/* Single step context for uprobe */
> +struct uprobe_step_ctx {
> + struct list_head node;
> + unsigned long match_addr;
> +};

I don't understand this... please see below.

> +struct arch_uprobe_task {
> + unsigned long saved_fault_code;
> + u64 saved_user_pc;
> + struct uprobe_step_ctx ss_ctx;
> +};

saved_user_pc looks unneeded, you can rely on uprobe_task->vaddr ?

> +++ b/arch/arm64/kernel/uprobes.c
> @@ -0,0 +1,255 @@
> +/*
> + * Copyright (C) 2014 Pratyush Anand <panand@xxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include <linux/highmem.h>
> +#include <linux/ptrace.h>
> +#include <linux/uprobes.h>
> +#include <asm/debug-monitors.h>
> +#include <asm/probes.h>
> +
> +#include "probes-arm64.h"
> +
> +#define UPROBE_INV_FAULT_CODE UINT_MAX
> +
> +static LIST_HEAD(step_ctx);
> +static DEFINE_RWLOCK(step_ctx_lock);
> +
> +static void add_ss_context(struct uprobe_task *utask)
> +{
> + struct uprobe_step_ctx *ss_ctx = &utask->autask.ss_ctx;
> +
> + ss_ctx->match_addr = utask->xol_vaddr;
> + write_lock(&step_ctx_lock);
> + list_add(&ss_ctx->node, &step_ctx);
> + write_unlock(&step_ctx_lock);
> +}
> +
> +static struct uprobe_step_ctx *find_ss_context(unsigned long vaddr)
> +{
> + struct uprobe_step_ctx *ss_ctx;
> +
> + read_lock(&step_ctx_lock);
> + list_for_each_entry(ss_ctx, &step_ctx, node) {
> + if (ss_ctx->match_addr == vaddr) {
> + read_unlock(&step_ctx_lock);
> + return ss_ctx;
> + }
> + }
> + read_unlock(&step_ctx_lock);
> +
> + return NULL;
> +}

This looks very wrong to me, but perhaps because I do not understand
why do we need these *_ss_context() helpers.

> +static void del_ss_context(struct uprobe_task *utask)
> +{
> + struct uprobe_step_ctx *ss_ctx = find_ss_context(utask->xol_vaddr);
> +
> + if (ss_ctx) {
> + write_lock(&step_ctx_lock);
> + list_del(&ss_ctx->node);
> + write_unlock(&step_ctx_lock);
> + } else {
> + WARN_ON(1);
> + }
> +}

Don't we need del_ss_context() in arch_uprobe_abort_xol() ? But this is
minor.

Why we can trust find_ss_context() ? What if another thread also called
add_ss_context() with the same (virtual) ->xol_vaddr ?

But the main question is: why do we need add/find_ss_context ?? Please
explain.

> +int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> + struct uprobe_task *utask = current->utask;
> +
> + /* saved fault code is restored in post_xol */
> + utask->autask.saved_fault_code = current->thread.fault_code;
> +
> + /* An invalid fault code between pre/post xol event */
> + current->thread.fault_code = UPROBE_INV_FAULT_CODE;
> +
> + /* Save user pc */
> + utask->autask.saved_user_pc = task_pt_regs(current)->user_regs.pc;
> +
> + /* Instruction point to execute ol */
> + instruction_pointer_set(regs, utask->xol_vaddr);
> +
> + add_ss_context(utask);
> +
> + user_enable_single_step(current);
> +
> + return 0;
> +}
> +
> +int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> + struct uprobe_task *utask = current->utask;
> +
> + WARN_ON_ONCE(current->thread.fault_code != UPROBE_INV_FAULT_CODE);
> +
> + /* restore fault code */
> + current->thread.fault_code = utask->autask.saved_fault_code;
> +
> + /* restore user pc */
> + task_pt_regs(current)->user_regs.pc = utask->autask.saved_user_pc;
> +
> + /* Instruction point to execute next to breakpoint address */
> + instruction_pointer_set(regs, utask->vaddr + 4);
> +
> + del_ss_context(utask);
> +
> + user_disable_single_step(current);
> +
> + return 0;
> +}

task_pt_regs() above looks strange. We we can't use "struct pt_regs *regs"
passed as an argument?

See also the note about .saved_user_pc above. I think you can use
utask->vaddr instead.

And why do you need to play with ->user_regs.pc?? instruction_pointer_set()
after that modifies the same word?

Or it is possible that regs != task_pt_regs(current) ? (to remind, I do not
know arm ;)

Could you also explain

instruction_pointer_set(regs, utask->vaddr + 4);

?

I mean, I do not understand why this is always correct. What if the probed
insn is "jmp" (I do not know arm64's name for jump) ?

Probably this is correct because in this case arm_probe_decode_insn() should
return INSN_GOOD_NO_SLOT and this insn will be emulated? If yes, this needs a
comment, imo.

> +static int uprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
> +{
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + uprobe_pre_sstep_notifier(regs);
> + local_irq_restore(flags);
> +
> + return 0;
> +}

Again, you do not need to disable irqs around uprobe_pre_sstep_notifier().

And I am not sure I understand the logic... "return 0" actually means
"return DBG_HOOK_HANDLED", right?

I do not understand this register_break_hook() interface and the usage
of .esr_mask/esr_val. But given that this patch adds BRK64_ESR_UPROBES
and uses BRK64_OPCODE_UPROBES, I will assume that uprobe_breakpoint_handler()
will be called if this exception was triggered by UPROBE_SWBP_INSN.

In this case, why the unconditional DBG_HOOK_HANDLED is correct? For example,
what if the application itself or debugger use UPROBE_SWBP_INSN for (self)
debugging and this task has no uprobes? In this case uprobe_pre_sstep_notifier()
will do nothing, it won't set TIF_UPROBES and handle_swbp() won't be called.

IOW, shouldn't it do

if (user_mode(regs) && uprobe_pre_sstep_notifier(regs))
return DBG_HOOK_HANDLED;
return DBG_HOOK_ERROR;

?

> +static int uprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
> +{
> + unsigned long flags;
> +
> + if (!find_ss_context(regs->pc - 4))
> + return DBG_HOOK_ERROR;
> +
> + local_irq_save(flags);
> + uprobe_post_sstep_notifier(regs);
> + local_irq_restore(flags);
> +
> + return 0;
> +}

The same. No need to clear irqs, and please explain why we can't rely
on user_mode() && uprobe_post_sstep_notifier(), and why do we
need find_ss_context().

> +void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
> + void *kaddr, unsigned long len)
> +{
> + __flush_ptrace_access(page, uaddr, kaddr, len);
> +}

I have some concerns... I'll reply to 5/8 which adds __flush_ptrace_access.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/