Date:	Sun, 04 Jan 2015 19:19:33 +0530
From:	Pratyush Anand <panand@...hat.com>
To:	Oleg Nesterov <oleg@...hat.com>
CC:	linux-arm-kernel@...ts.infradead.org, linux@....linux.org.uk,
	tixy@...aro.org, ananth@...ibm.com, sandeepa.prabhu@...aro.org,
	catalin.marinas@....com, will.deacon@....com,
	linux-kernel@...r.kernel.org, anil.s.keshavamurthy@...el.com,
	masami.hiramatsu.pt@...achi.com, wcohen@...hat.com
Subject: Re: [RFC 8/8] ARM64: Add uprobe support



On Friday 02 January 2015 10:53 PM, Oleg Nesterov wrote:
> 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.

Yes.

>
>> +/* 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 ?

Probably yes. Will change.

>
>> +++ b/arch/arm64/kernel/uprobes.c
>> @@ -0,0 +1,255 @@
>> +/*
>> + * Copyright (C) 2014 Pratyush Anand <panand@...hat.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.

Thanks. Yes, we need del_ss_context() in arch_uprobe_abort_xol().

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

OK..So xol_vaddr is not unique, then need to look for something else 
which could be unique for each probe.

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

See arch/arm64/kernel/debug-monitors.c: call_step_hook

Unlike breakpoint exception, there is no ESR info check for step 
exception. So, it is the responsibility of step handler 
(uprobe_single_step_handler) to make sure that exception was generated 
for it.

>> +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);
>
> ?
>

Correct, saved_user_pc is not needed and also no need to play with 
->user_regs.pc. instruction_pointer_set should be sufficient.


> 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.
>

Yes, a branch instruction like b or bl or ret will be simulated.

>> +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().

OK. (Actually took it from arch/arm/kernel/uprobe.c)

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

Yes. So better I will put return DBG_HOOK_HANDLED;

>
> 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.

Correct.

>
> 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;
>
> ?

Thanks for pointing out this bug with the code.
Your point seems pretty valid.

>
>> +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().
>

I had not looked into uprobe_post_sstep_notifier implementtaion.I think, 
you are right.


Thanks for all these valuable comments.

~Pratyush


>> +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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists