Message-ID: <87pqwvm8cl.fsf@basil.nowhere.org>
Date:	Fri, 03 Sep 2010 12:26:02 +0200
From:	Andi Kleen <andi@...stfloor.org>
To:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Cc:	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...e.hu>,
	Steven Rostedt <rostedt@...dmis.org>,
	Randy Dunlap <rdunlap@...otime.net>,
	Arnaldo Carvalho de Melo <acme@...radead.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Christoph Hellwig <hch@...radead.org>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Oleg Nesterov <oleg@...hat.com>,
	Mark Wielaard <mjw@...hat.com>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Naren A Devaiah <naren.devaiah@...ibm.com>,
	Jim Keniston <jkenisto@...ux.vnet.ibm.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	"Frank Ch. Eigler" <fche@...hat.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCHv11 2.6.36-rc2-tip 4/15]  4: uprobes: x86 specific functions for user space breakpointing.

Srikar Dronamraju <srikar@...ux.vnet.ibm.com> writes:

Quick high level review. I did not attempt to validate the basic
algorithm, more the kernel interface.

> Provides x86 specific functions for instruction analysis and
> instruction validation and x86 specific pre-processing and
> post-processing of singlestep especially for RIP relative
> instructions. Uses "x86: instruction decoder API" for validation and
> analysis of user space instructions. This analysis is used at the time
> of post-processing of breakpoint hit to do the necessary fix-ups.
> There is support for breakpointing RIP relative instructions. However
> there are still few instructions that cannot be singlestepped.

One general comment here: since with uprobes the instruction
decoder becomes security critical did you do any fuzz tests
on it (e.g. like using it on crashme or on code that has 
been corrupted with a few bitflips) ?

> +typedef u8 user_bkpt_opcode_t;

Maybe it's me, but I would prefer breakpoint instead of bkpt
> +#ifdef CONFIG_X86_32
> +#define is_32bit_app(tsk) 1
> +#else
> +#define is_32bit_app(tsk) (test_tsk_thread_flag(tsk, TIF_IA32))
> +#endif

This probably should be elsewhere.

> +
> +#define UPROBES_FIX_RIP_AX	0x8000
> +#define UPROBES_FIX_RIP_CX	0x4000
> +
> +/* Adaptations for mhiramat x86 decoder v14. */
> +#define OPCODE1(insn) ((insn)->opcode.bytes[0])
> +#define OPCODE2(insn) ((insn)->opcode.bytes[1])
> +#define OPCODE3(insn) ((insn)->opcode.bytes[2])
> +#define MODRM_REG(insn) X86_MODRM_REG(insn->modrm.value)
> +
> +/*
> + * @reg: reflects the saved state of the task
> + * @vaddr: the virtual address to jump to.
> + * Return 0 on success or a -ve number on error.
> + */
> +void set_ip(struct pt_regs *regs, unsigned long vaddr)
> +{
> +	regs->ip = vaddr;
> +}
> +
> +#ifdef CONFIG_X86_64
> +static bool is_riprel_insn(struct user_bkpt *user_bkpt)
> +{
> +	return ((user_bkpt->fixups &
> +			(UPROBES_FIX_RIP_AX | UPROBES_FIX_RIP_CX)) != 0);
> +}
> +

Shouldn't all this stuff be in the instruction decoder? 

It seems weird to have the knowledge spread over multiple files.

> +
> +static void report_bad_prefix(void)
> +{
> +	printk(KERN_ERR "uprobes does not currently support probing "
> +		"instructions with any of the following prefixes: "
> +		"cs:, ds:, es:, ss:, lock:\n");
> +}
> +
> +static void report_bad_1byte_opcode(int mode, user_bkpt_opcode_t op)
> +{
> +	printk(KERN_ERR "In %d-bit apps, "
> +		"uprobes does not currently support probing "
> +		"instructions whose first byte is 0x%2.2x\n", mode, op);
> +}
> +
> +static void report_bad_2byte_opcode(user_bkpt_opcode_t op)
> +{
> +	printk(KERN_ERR "uprobes does not currently support probing "
> +		"instructions with the 2-byte opcode 0x0f 0x%2.2x\n", op);
> +}

These functions that just do a single printk seem weird. I would
do that in the caller. Also the message could be shortened I guess
and should just dump the bytes.

> +
> +/**
> + * analyze_insn - instruction analysis including validity and fixups.
> + * @tsk: the probed task.
> + * @user_bkpt: the probepoint information.
> + * Return 0 on success or a -ve number on error.
> + */
> +int analyze_insn(struct task_struct *tsk, struct user_bkpt *user_bkpt)
> +{
> +	int ret;
> +	struct insn insn;
> +
> +	user_bkpt->fixups = 0;
> +#ifdef CONFIG_X86_64
> +	user_bkpt->arch_info.rip_target_address = 0x0;
> +#endif
> +
> +	if (is_32bit_app(tsk))

This check is not fully correct because it's valid to have
32bit code in 64bit programs and vice versa.  The only good
way to check that is to look at the code segment at runtime
though (and it gets complicated if you want to handle LDTs,
but that could be optional). May be difficult to do though.

Also the compat bit is not necessarily set if no system call is
executing. You would rather need to check the exec_domain.
> + */
> +static int adjust_ret_addr(struct task_struct *tsk, unsigned long sp,
> +							long correction)
> +{
> +	int rasize, ncopied;
> +	long ra = 0;
> +
> +	if (is_32bit_app(tsk))
> +		rasize = 4;
> +	else
> +		rasize = 8;
> +	ncopied = uprobes_read_vm(tsk, (void __user *) sp, &ra, rasize);
> +	if (unlikely(ncopied != rasize))
> +		goto fail;

goto is automatically unlikely and unlikely is deprecated anyways.

-Andi
-- 
ak@...ux.intel.com -- Speaking for myself only.
--
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