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 <[email protected]>
---
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
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
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
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
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
/*
* "®s->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 ®s->sp happens to
* point to the top of the pre-int3 stack.
*/
#define stack_addr(regs) ((unsigned long *)®s->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
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
/*
* "®s->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 ®s->sp happens to
* point to the top of the pre-int3 stack.
*/
#define stack_addr(regs) ((unsigned long *)®s->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
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
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
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
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
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
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 ®s 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
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 ®s 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
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 <[email protected]>
---
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
* Harvey Harrison <[email protected]> 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