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

[Oleg Nesterov]

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/