Re: [PATCH] x86: Use v8086_mode helper, trivial unification

Re: [PATCH] x86: Use v8086_mode helper, trivial unification

[H. Peter Anvin]

Harvey Harrison wrote:
> Use v8086_mode inline in fault_32.c, no functional change
> also ifdef the section for 32-bit only and add to fault_64.c

The #ifdef is unnecessary, since v8086_mode() is now a constant zero on 
x86-64.  gcc will remove the if clause.

	-hpa

[PATCH] x86: Use v8086_mode helper, trivial unification

[Harvey Harrison]

Use v8086_mode inline in fault_32.c, no functional change
also ifdef the section for 32-bit only and add to fault_64.c

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 arch/x86/mm/fault_32.c |    4 +++-
 arch/x86/mm/fault_64.c |   11 +++++++++++
 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/arch/x86/mm/fault_32.c b/arch/x86/mm/fault_32.c
index 77915ac..6d8a0bb 100644
--- a/arch/x86/mm/fault_32.c
+++ b/arch/x86/mm/fault_32.c
@@ -547,14 +547,16 @@ good_area:
 	else
 		tsk->min_flt++;
 
+#ifdef CONFIG_X86_32
 	/*
 	 * Did it hit the DOS screen memory VA from vm86 mode?
 	 */
-	if (regs->flags & VM_MASK) {
+	if (v8086_mode(regs)) {
 		unsigned long bit = (address - 0xA0000) >> PAGE_SHIFT;
 		if (bit < 32)
 			tsk->thread.screen_bitmap |= 1 << bit;
 	}
+#endif
 	up_read(&mm->mmap_sem);
 	return;
 
diff --git a/arch/x86/mm/fault_64.c b/arch/x86/mm/fault_64.c
index 84531d8..8393275 100644
--- a/arch/x86/mm/fault_64.c
+++ b/arch/x86/mm/fault_64.c
@@ -551,6 +551,17 @@ good_area:
 		tsk->maj_flt++;
 	else
 		tsk->min_flt++;
+
+#ifdef CONFIG_X86_32
+	/*
+	 * Did it hit the DOS screen memory VA from vm86 mode?
+	 */
+	if (v8086_mode(regs)) {
+		unsigned long bit = (address - 0xA0000) >> PAGE_SHIFT;
+		if (bit < 32)
+			tsk->thread.screen_bitmap |= 1 << bit;
+	}
+#endif
 	up_read(&mm->mmap_sem);
 	return;
 
-- 
1.5.4.rc2.1164.g6451

Re: [PATCH] x86: Use v8086_mode helper, trivial unification

[Harvey Harrison]

On Thu, 2008-01-17 at 18:02 -0500, H. Peter Anvin wrote:
> Harvey Harrison wrote:
> > Use v8086_mode inline in fault_32.c, no functional change
> > also ifdef the section for 32-bit only and add to fault_64.c
> 
> The #ifdef is unnecessary, since v8086_mode() is now a constant zero on 
> x86-64.  gcc will remove the if clause.
> 

Sorry, missed that detail in ptrace.h, I notice now.

Is there some better way this could be organized, would the following
be an improvement, as opposed to two long ifdef sections?

Patch will follow if you think it's a good idea.

static inline int user_mode(struct pt_regs *regs)
{
#ifdef CONFIG_X86_32
	return (regs->cs & SEGMENT_RPL_MASK) == USER_RPL;
#else
	return !!(regs->cs & 3);
#endif
}

static inline int user_mode_vm(struct pt_regs *regs)
{
#ifdef CONFIG_X86_32
	return ((regs->cs & SEGMENT_RPL_MASK) |
		(regs->flags & VM_MASK)) >= USER_RPL;
#else
	return user_mode(regs);
#endif
}

static inline int v8086_mode(struct pt_regs *regs)
{
#ifdef CONFIG_X86_32
	return (regs->flags & VM_MASK);
#else
	return 0;
#endif
}

/* OK, maybe this should just be deleted, and use
regs->ip directly in code*/
static inline unsigned long instruction_pointer(struct pt_regs *regs)
{
	return regs->ip;
}

/* OK, maybe this should just be deleted, and use
regs->bp directly in code*/
static inline unsigned long frame_pointer(struct pt_regs *regs)
{
	return regs->bp;
}

static inline unsigned long stack_pointer(struct pt_regs *regs)
{
#ifdef CONFIG_X86_32
	return (unsigned long)regs;
#else
	return regs->sp;
#endif
}

/* still need a define here, as one is long and one is unsigned long.
 * but this is another target for unification I guess. */
#define regs_return_value(regs) ((regs)->ax)

Harvey

Re: [PATCH] x86: Use v8086_mode helper, trivial unification

[H. Peter Anvin]

Harvey Harrison wrote:
> 
> Sorry, missed that detail in ptrace.h, I notice now.
> 
> Is there some better way this could be organized, would the following
> be an improvement, as opposed to two long ifdef sections?
> 
> Patch will follow if you think it's a good idea.

It is actually quite a bit easier to read.

> 
> static inline unsigned long stack_pointer(struct pt_regs *regs)
> {
> #ifdef CONFIG_X86_32
> 	return (unsigned long)regs;
> #else
> 	return regs->sp;
> #endif
> }

This one is kind of strange.  In particular, the 32-bit definition isn't 
exactly what one would expect.  It makes me concerned that it actually 
refers to two different kinds of stack pointers?

> /* still need a define here, as one is long and one is unsigned long.
>  * but this is another target for unification I guess. */
> #define regs_return_value(regs) ((regs)->ax)

Indeed...

	-hpa

Re: [PATCH] x86: Use v8086_mode helper, trivial unification

[H. Peter Anvin]

Harvey Harrison wrote:
> 
>>> /* still need a define here, as one is long and one is unsigned long.
>>>  * but this is another target for unification I guess. */
>>> #define regs_return_value(regs) ((regs)->ax)
>> Indeed...
> 
> I think this comes out of Roland's patches unifying some names eip/rip,
> eax/rax, etc.
> 
> CC'd in case he felt like more work ;-)
> 

Funny, I thought those were mine :-/

	-hpa

Re: [PATCH] x86: Use v8086_mode helper, trivial unification

[Harvey Harrison]

On Thu, 2008-01-17 at 19:59 -0500, H. Peter Anvin wrote:
> Harvey Harrison wrote:
> > 
> > Sorry, missed that detail in ptrace.h, I notice now.
> > 
> > Is there some better way this could be organized, would the following
> > be an improvement, as opposed to two long ifdef sections?
> > 
> > Patch will follow if you think it's a good idea.
> 
> It is actually quite a bit easier to read.

I'll send along a patch along soon, any thoughts on how to order it in
the file?

> 
> > 
> > static inline unsigned long stack_pointer(struct pt_regs *regs)
> > {
> > #ifdef CONFIG_X86_32
> > 	return (unsigned long)regs;
> > #else
> > 	return regs->sp;
> > #endif
> > }
> 
> This one is kind of strange.  In particular, the 32-bit definition isn't 
> exactly what one would expect.  It makes me concerned that it actually 
> refers to two different kinds of stack pointers?

This tripped up the kprobes unification as well, see the stack_addr()
helper that was introduced there.  Would be good to figure this out
and put a big fat comment on it.

kprobes.c

#ifdef CONFIG_X86_64
#define stack_addr(regs) ((unsigned long *)regs->sp)
#else
/*
 * "&regs->sp" looks wrong, but it's correct for x86_32.  x86_32 CPUs
 * don't save the ss and esp registers if the CPU is already in kernel
 * mode when it traps.  So for kprobes, regs->sp and regs->ss are not
 * the [nonexistent] saved stack pointer and ss register, but rather
 * the top 8 bytes of the pre-int3 stack.  So &regs->sp happens to
 * point to the top of the pre-int3 stack.
 */
#define stack_addr(regs) ((unsigned long *)&regs->sp)
#endif

> > /* still need a define here, as one is long and one is unsigned long.
> >  * but this is another target for unification I guess. */
> > #define regs_return_value(regs) ((regs)->ax)
> 
> Indeed...

I think this comes out of Roland's patches unifying some names eip/rip,
eax/rax, etc.

CC'd in case he felt like more work ;-)

Harvey

Re: [PATCH] x86: Use v8086_mode helper, trivial unification

[Harvey Harrison]

Roland actually put on CC this time.

On Thu, 2008-01-17 at 19:59 -0500, H. Peter Anvin wrote:
> Harvey Harrison wrote:
> > 
> > Sorry, missed that detail in ptrace.h, I notice now.
> > 
> > Is there some better way this could be organized, would the following
> > be an improvement, as opposed to two long ifdef sections?
> > 
> > Patch will follow if you think it's a good idea.
> 
> It is actually quite a bit easier to read.

I'll send along a patch along soon, any thoughts on how to order it in
the file?

> 
> > 
> > static inline unsigned long stack_pointer(struct pt_regs *regs)
> > {
> > #ifdef CONFIG_X86_32
> > 	return (unsigned long)regs;
> > #else
> > 	return regs->sp;
> > #endif
> > }
> 
> This one is kind of strange.  In particular, the 32-bit definition isn't 
> exactly what one would expect.  It makes me concerned that it actually 
> refers to two different kinds of stack pointers?

This tripped up the kprobes unification as well, see the stack_addr()
helper that was introduced there.  Would be good to figure this out
and put a big fat comment on it.

kprobes.c

#ifdef CONFIG_X86_64
#define stack_addr(regs) ((unsigned long *)regs->sp)
#else
/*
 * "&regs->sp" looks wrong, but it's correct for x86_32.  x86_32 CPUs
 * don't save the ss and esp registers if the CPU is already in kernel
 * mode when it traps.  So for kprobes, regs->sp and regs->ss are not
 * the [nonexistent] saved stack pointer and ss register, but rather
 * the top 8 bytes of the pre-int3 stack.  So &regs->sp happens to
 * point to the top of the pre-int3 stack.
 */
#define stack_addr(regs) ((unsigned long *)&regs->sp)
#endif

> > /* still need a define here, as one is long and one is unsigned long.
> >  * but this is another target for unification I guess. */
> > #define regs_return_value(regs) ((regs)->ax)
> 
> Indeed...

I think this comes out of Roland's patches unifying some names eip/rip,
eax/rax, etc.

CC'd in case he felt like more work ;-)

Harvey

Re: [PATCH] x86: Use v8086_mode helper, trivial unification

[Harvey Harrison]

On Thu, 2008-01-17 at 20:14 -0500, H. Peter Anvin wrote:
> Harvey Harrison wrote:
> > 
> >>> /* still need a define here, as one is long and one is unsigned long.
> >>>  * but this is another target for unification I guess. */
> >>> #define regs_return_value(regs) ((regs)->ax)
> >> Indeed...
> > 
> > I think this comes out of Roland's patches unifying some names eip/rip,
> > eax/rax, etc.
> > 
> > CC'd in case he felt like more work ;-)
> > 
> 
> Funny, I thought those were mine :-/
> 

*Ahem* my apologies, they are in the x86.git lineup right before the
ptrace work Roland did, guess it's time to get a bigger gitk font.

My bad,

Harvey

Re: [PATCH] x86: Use v8086_mode helper, trivial unification

[H. Peter Anvin]

Roland McGrath wrote:
> It's indeed true that &pt_regs is truly the esp value for x86-32
> kernel-mode trap frames.  Because this nonobvious calculation is 
> only right for a kernel mode pt_regs and not for a user-mode one,
> I think it would be better to use a name for the inline/macro that
> makes this quite clear, rather than one so generic as "stack_addr".

Indeed.  This was certainly highly nonobvious in the current code.

	-hpa

Re: [PATCH] x86: Use v8086_mode helper, trivial unification

[Roland McGrath]

It's indeed true that &pt_regs is truly the esp value for x86-32
kernel-mode trap frames.  Because this nonobvious calculation is 
only right for a kernel mode pt_regs and not for a user-mode one,
I think it would be better to use a name for the inline/macro that
makes this quite clear, rather than one so generic as "stack_addr".

> I think this comes out of Roland's patches unifying some names eip/rip,
> eax/rax, etc.
> 
> CC'd in case he felt like more work ;-)

That was hpa, not me.  Harmonizing the signedness of the struct members
does seem to me like a reasonable and deobfuscatory thing to do.
But I don't plan to get involved in doing it.


Thanks,
Roland

Re: [PATCH] x86: Use v8086_mode helper, trivial unification

[Harvey Harrison]

On Thu, 2008-01-17 at 20:36 -0500, H. Peter Anvin wrote:
> Roland McGrath wrote:
> > It's indeed true that &pt_regs is truly the esp value for x86-32
> > kernel-mode trap frames.  Because this nonobvious calculation is 
> > only right for a kernel mode pt_regs and not for a user-mode one,
> > I think it would be better to use a name for the inline/macro that
> > makes this quite clear, rather than one so generic as "stack_addr".
> 
> Indeed.  This was certainly highly nonobvious in the current code.
> 

What do you think of kernel_sp()

Harvey

Re: [PATCH] x86: Use v8086_mode helper, trivial unification

[Harvey Harrison]

On Thu, 2008-01-17 at 20:36 -0500, H. Peter Anvin wrote:
> Roland McGrath wrote:
> > It's indeed true that &pt_regs is truly the esp value for x86-32
> > kernel-mode trap frames.  Because this nonobvious calculation is 
> > only right for a kernel mode pt_regs and not for a user-mode one,
> > I think it would be better to use a name for the inline/macro that
> > makes this quite clear, rather than one so generic as "stack_addr".
> 
> Indeed.  This was certainly highly nonobvious in the current code.
> 

What do you think of:

/*
 * (unsigned long)regs looks strange, but it's correct for x86_32.  x86_32 CPUs
 * don't save the ss and esp registers if the CPU is already in kernel mode
 * when it traps.  So &regs happens to be esp.  Valid only for kernel-mode
 * pt_regs.
 */
static inline unsigned long stack_pointer(struct pt_regs *regs)
{
#ifdef CONFIG_X86_32
	return (unsigned long)regs;
#else
	return regs->sp;
#endif
}

Harvey

Re: [PATCH] x86: Use v8086_mode helper, trivial unification

[H. Peter Anvin]

Harvey Harrison wrote:
> 
> What do you think of:
> 
> /*
>  * (unsigned long)regs looks strange, but it's correct for x86_32.  x86_32 CPUs
>  * don't save the ss and esp registers if the CPU is already in kernel mode
>  * when it traps.  So &regs happens to be esp.  Valid only for kernel-mode
>  * pt_regs.
>  */
> static inline unsigned long stack_pointer(struct pt_regs *regs)
> {
> #ifdef CONFIG_X86_32
> 	return (unsigned long)regs;
> #else
> 	return regs->sp;
> #endif
> }
> 

Please highlight the last sentence better.  That's a user restriction 
and a highly important one.

	-hpa

[PATCH] x86: Rename stack_pointer to kernel_trap_sp

[Harvey Harrison]

Choose a less generic name for such a special case.  Add
a comment explaining the odd use in X86_32.

Change the one user of stack_pointer.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 arch/x86/oprofile/backtrace.c |    2 +-
 include/asm-x86/ptrace.h      |    8 +++++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/oprofile/backtrace.c b/arch/x86/oprofile/backtrace.c
index 0ca4815..e2095cb 100644
--- a/arch/x86/oprofile/backtrace.c
+++ b/arch/x86/oprofile/backtrace.c
@@ -77,7 +77,7 @@ void
 x86_backtrace(struct pt_regs * const regs, unsigned int depth)
 {
 	struct frame_head *head = (struct frame_head *)frame_pointer(regs);
-	unsigned long stack = stack_pointer(regs);
+	unsigned long stack = kernel_trap_sp(regs);
 
 	if (!user_mode_vm(regs)) {
 		if (depth)
diff --git a/include/asm-x86/ptrace.h b/include/asm-x86/ptrace.h
index 79d5b8f..d9e04b4 100644
--- a/include/asm-x86/ptrace.h
+++ b/include/asm-x86/ptrace.h
@@ -182,7 +182,13 @@ static inline int v8086_mode(struct pt_regs *regs)
 #endif
 }
 
-static inline unsigned long stack_pointer(struct pt_regs *regs)
+/*
+ * X86_32 CPUs don't save ss and esp if the CPU is already in kernel mode
+ * when it traps.  So regs will be the current sp.
+ *
+ * This is valid only for kernel mode traps.
+ */
+static inline unsigned long kernel_trap_sp(struct pt_regs *regs)
 {
 #ifdef CONFIG_X86_32
 	return (unsigned long)regs;
-- 
1.5.4.rc2.1164.g6451

Re: [PATCH] x86: Rename stack_pointer to kernel_trap_sp

[Ingo Molnar]

* Harvey Harrison <harvey.harrison@gmail.com> wrote:

>  	struct frame_head *head = (struct frame_head *)frame_pointer(regs);
> -	unsigned long stack = stack_pointer(regs);
> +	unsigned long stack = kernel_trap_sp(regs);

thanks, applied.

	Ingo