lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20090530081303.GB15755@infradead.org>
Date:	Sat, 30 May 2009 04:13:03 -0400
From:	Christoph Hellwig <hch@...radead.org>
To:	Masami Hiramatsu <mhiramat@...hat.com>
Cc:	Ingo Molnar <mingo@...e.hu>, Steven Rostedt <rostedt@...dmis.org>,
	lkml <linux-kernel@...r.kernel.org>,
	systemtap <systemtap@...rces.redhat.com>,
	kvm <kvm@...r.kernel.org>,
	DLE <dle-develop@...ts.sourceforge.net>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Roland McGrath <roland@...hat.com>, linux-arch@...r.kernel.org
Subject: Re: [PATCH -tip v8 5/7] x86: add pt_regs register and stack access
	APIs

You might want to run this past linux-arch to make sure this is suitable
for other architectures.

> --- a/arch/x86/include/asm/ptrace.h
> +++ b/arch/x86/include/asm/ptrace.h
> @@ -7,6 +7,7 @@
>  
>  #ifdef __KERNEL__
>  #include <asm/segment.h>
> +#include <asm/page_types.h>
>  #endif

I really wonder if we should split asm/ptrace.h into one file
just defining pt_regs both for userspace and the kernel, and one with
all kinds of register access helpers and maybe another one for the
ptrace architecture interface.

Unforuntately we would have to keep the ptrace.h name for the one
carrying pt_regs as it's exposed to userland.

> +static inline unsigned long get_register(struct pt_regs *regs, unsigned offset)
> +{

I woner if all these names aren't a bit generic.  Shoud we maybe add a
regs_ prefix to make it clear it operates on a pt_regs register set?

Also some kerneldoc documentation for all these helpers would be nice.

> +/* Get Nth argument at function call */
> +static inline unsigned long get_argument_nth(struct pt_regs *regs, unsigned n)
> +{
> +#ifdef CONFIG_X86_32
> +#define NR_REGPARMS 3

I think completely separate version for 32 vs 64 bit for this one would
be cleaner.

> +	if (n < NR_REGPARMS) {
> +		switch (n) {
> +		case 0: return regs->ax;
> +		case 1: return regs->dx;
> +		case 2: return regs->cx;
> +		}

Normal kernel style would be

		switch (n) {
		case 0:
			return regs->ax;
		case 1:
			return regs->dx;
		case 2:
			return regs->cx;
		}

> +#define REG_OFFSET(r) offsetof(struct pt_regs, r)
> +#define REG_OFFSET_NAME(r) {.name = #r, .offset = REG_OFFSET(r)}
> +#define REG_OFFSET_END {.name = NULL, .offset = 0}

At least the REG_OFFSET macro seems superflous to me.

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

Powered by Openwall GNU/*/Linux Powered by OpenVZ