Messages in this thread | | | Date | Fri, 2 Jan 2015 18:23:33 +0100 | From | Oleg Nesterov <> | Subject | Re: [RFC 8/8] ARM64: Add uprobe support |
| |
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@redhat.com> > + * > + * 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.
| |