Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1448169 > unrolled thread

[PATCH 03/19] x86/dumpstack: remove unnecessary stack pointer arguments

Started byJosh Poimboeuf <jpoimboe@redhat.com>
First post2016-07-21 23:30 +0200
Last post2016-07-22 05:10 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 03/19] x86/dumpstack: remove unnecessary stack pointer arguments Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
    Re: [PATCH 03/19] x86/dumpstack: remove unnecessary stack pointer arguments Andy Lutomirski <luto@amacapital.net> - 2016-07-22 00:00 +0200
      Re: [PATCH 03/19] x86/dumpstack: remove unnecessary stack pointer  arguments Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-22 03:50 +0200
        Re: [PATCH 03/19] x86/dumpstack: remove unnecessary stack pointer arguments Andy Lutomirski <luto@amacapital.net> - 2016-07-22 04:30 +0200
        Re: [PATCH 03/19] x86/dumpstack: remove unnecessary stack pointer arguments Brian Gerst <brgerst@gmail.com> - 2016-07-22 05:10 +0200

#1448169 — [PATCH 03/19] x86/dumpstack: remove unnecessary stack pointer arguments

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2016-07-21 23:30 +0200
Subject[PATCH 03/19] x86/dumpstack: remove unnecessary stack pointer arguments
Message-ID<rXtHz-4Eb-9@gated-at.bofh.it>
When calling show_stack_log_lvl() or dump_trace() with a regs argument,
providing a stack pointer or frame pointer is redundant.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>d
---
 arch/x86/kernel/dumpstack.c    | 2 +-
 arch/x86/kernel/dumpstack_32.c | 2 +-
 arch/x86/kernel/dumpstack_64.c | 5 +----
 arch/x86/oprofile/backtrace.c  | 4 +---
 4 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 145f18b..75d21ac 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -200,7 +200,7 @@ void show_stack(struct task_struct *task, unsigned long *sp)
 
 void show_stack_regs(struct pt_regs *regs)
 {
-	show_stack_log_lvl(current, regs, (unsigned long *)regs->sp, regs->bp, "");
+	show_stack_log_lvl(current, regs, NULL, 0, "");
 }
 
 static arch_spinlock_t die_lock = __ARCH_SPIN_LOCK_UNLOCKED;
diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
index 358fe1c..c533b8b 100644
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -122,7 +122,7 @@ void show_regs(struct pt_regs *regs)
 		u8 *ip;
 
 		pr_emerg("Stack:\n");
-		show_stack_log_lvl(NULL, regs, &regs->sp, 0, KERN_EMERG);
+		show_stack_log_lvl(NULL, regs, NULL, 0, KERN_EMERG);
 
 		pr_emerg("Code:");
 
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index bc08e8b..360f2e8 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -286,9 +286,7 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
 void show_regs(struct pt_regs *regs)
 {
 	int i;
-	unsigned long sp;
 
-	sp = regs->sp;
 	show_regs_print_info(KERN_DEFAULT);
 	__show_regs(regs, 1);
 
@@ -303,8 +301,7 @@ void show_regs(struct pt_regs *regs)
 		u8 *ip;
 
 		printk(KERN_DEFAULT "Stack:\n");
-		show_stack_log_lvl(NULL, regs, (unsigned long *)sp,
-				   0, KERN_DEFAULT);
+		show_stack_log_lvl(NULL, regs, NULL, 0, KERN_DEFAULT);
 
 		printk(KERN_DEFAULT "Code: ");
 
diff --git a/arch/x86/oprofile/backtrace.c b/arch/x86/oprofile/backtrace.c
index cb31a44..c594768 100644
--- a/arch/x86/oprofile/backtrace.c
+++ b/arch/x86/oprofile/backtrace.c
@@ -113,10 +113,8 @@ x86_backtrace(struct pt_regs * const regs, unsigned int depth)
 	struct stack_frame *head = (struct stack_frame *)frame_pointer(regs);
 
 	if (!user_mode(regs)) {
-		unsigned long stack = kernel_stack_pointer(regs);
 		if (depth)
-			dump_trace(NULL, regs, (unsigned long *)stack, 0,
-				   &backtrace_ops, &depth);
+			dump_trace(NULL, regs, NULL, 0, &backtrace_ops, &depth);
 		return;
 	}
 
-- 
2.7.4

[toc] | [next] | [standalone]

#1448200

FromAndy Lutomirski <luto@amacapital.net>
Date2016-07-22 00:00 +0200
Message-ID<rXuaB-4P9-1@gated-at.bofh.it>
In reply to#1448169
On Thu, Jul 21, 2016 at 2:21 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> When calling show_stack_log_lvl() or dump_trace() with a regs argument,
> providing a stack pointer or frame pointer is redundant.
>

> diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
> index 358fe1c..c533b8b 100644
> --- a/arch/x86/kernel/dumpstack_32.c
> +++ b/arch/x86/kernel/dumpstack_32.c
> @@ -122,7 +122,7 @@ void show_regs(struct pt_regs *regs)
>                 u8 *ip;
>
>                 pr_emerg("Stack:\n");
> -               show_stack_log_lvl(NULL, regs, &regs->sp, 0, KERN_EMERG);
> +               show_stack_log_lvl(NULL, regs, NULL, 0, KERN_EMERG);

This is weird -- note the &.  You're at some risk of exposing a bug in
x86_32's kernel_stack_pointer() function, which is a mess.  (I don't
see why it's written the way it is -- the actual return stack pointer
given a pt_regs is quite well defined -- if regs->cs & 3 != 0, then
it's regs->sp, else it's &regs->sp.)

That being said, this isn't a big deal, so:

Reviewed-by: Andy Lutomirski <luto@kernel.org>

If you want to make this all a bit more reliably on x86_32, you could
fix kernel_stack_pointer().

[toc] | [prev] | [next] | [standalone]

#1448299 — Re: [PATCH 03/19] x86/dumpstack: remove unnecessary stack pointer arguments

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2016-07-22 03:50 +0200
SubjectRe: [PATCH 03/19] x86/dumpstack: remove unnecessary stack pointer arguments
Message-ID<rXxLb-7m5-17@gated-at.bofh.it>
In reply to#1448200
On Thu, Jul 21, 2016 at 02:56:52PM -0700, Andy Lutomirski wrote:
> On Thu, Jul 21, 2016 at 2:21 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > When calling show_stack_log_lvl() or dump_trace() with a regs argument,
> > providing a stack pointer or frame pointer is redundant.
> >
> 
> > diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
> > index 358fe1c..c533b8b 100644
> > --- a/arch/x86/kernel/dumpstack_32.c
> > +++ b/arch/x86/kernel/dumpstack_32.c
> > @@ -122,7 +122,7 @@ void show_regs(struct pt_regs *regs)
> >                 u8 *ip;
> >
> >                 pr_emerg("Stack:\n");
> > -               show_stack_log_lvl(NULL, regs, &regs->sp, 0, KERN_EMERG);
> > +               show_stack_log_lvl(NULL, regs, NULL, 0, KERN_EMERG);
> 
> This is weird -- note the &.  You're at some risk of exposing a bug in
> x86_32's kernel_stack_pointer() function, which is a mess.  (I don't
> see why it's written the way it is -- the actual return stack pointer
> given a pt_regs is quite well defined -- if regs->cs & 3 != 0, then
> it's regs->sp, else it's &regs->sp.)
> 
> That being said, this isn't a big deal, so:
> 
> Reviewed-by: Andy Lutomirski <luto@kernel.org>
> 
> If you want to make this all a bit more reliably on x86_32, you could
> fix kernel_stack_pointer().

Ok.  The whole '&regs->sp' thing threw me for a loop.  I have no idea
what kernel_stack_pointer() is trying to do.  I just assumed it was
correct.  I'll take a look at it and try to fix it in another patch.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1448307

FromAndy Lutomirski <luto@amacapital.net>
Date2016-07-22 04:30 +0200
Message-ID<rXynT-7XR-3@gated-at.bofh.it>
In reply to#1448299
On Thu, Jul 21, 2016 at 6:41 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Thu, Jul 21, 2016 at 02:56:52PM -0700, Andy Lutomirski wrote:
>> On Thu, Jul 21, 2016 at 2:21 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > When calling show_stack_log_lvl() or dump_trace() with a regs argument,
>> > providing a stack pointer or frame pointer is redundant.
>> >
>>
>> > diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
>> > index 358fe1c..c533b8b 100644
>> > --- a/arch/x86/kernel/dumpstack_32.c
>> > +++ b/arch/x86/kernel/dumpstack_32.c
>> > @@ -122,7 +122,7 @@ void show_regs(struct pt_regs *regs)
>> >                 u8 *ip;
>> >
>> >                 pr_emerg("Stack:\n");
>> > -               show_stack_log_lvl(NULL, regs, &regs->sp, 0, KERN_EMERG);
>> > +               show_stack_log_lvl(NULL, regs, NULL, 0, KERN_EMERG);
>>
>> This is weird -- note the &.  You're at some risk of exposing a bug in
>> x86_32's kernel_stack_pointer() function, which is a mess.  (I don't
>> see why it's written the way it is -- the actual return stack pointer
>> given a pt_regs is quite well defined -- if regs->cs & 3 != 0, then
>> it's regs->sp, else it's &regs->sp.)
>>
>> That being said, this isn't a big deal, so:
>>
>> Reviewed-by: Andy Lutomirski <luto@kernel.org>
>>
>> If you want to make this all a bit more reliably on x86_32, you could
>> fix kernel_stack_pointer().
>
> Ok.  The whole '&regs->sp' thing threw me for a loop.  I have no idea
> what kernel_stack_pointer() is trying to do.  I just assumed it was
> correct.  I'll take a look at it and try to fix it in another patch.
>

On further inspection, it's probably correct except in cases of stack
overflow, so I wouldn't worry about it.  It's certainly
overcomplicated.

--Andy

[toc] | [prev] | [next] | [standalone]

#1448330

FromBrian Gerst <brgerst@gmail.com>
Date2016-07-22 05:10 +0200
Message-ID<rXz0B-8sT-13@gated-at.bofh.it>
In reply to#1448299
On Thu, Jul 21, 2016 at 9:41 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Thu, Jul 21, 2016 at 02:56:52PM -0700, Andy Lutomirski wrote:
>> On Thu, Jul 21, 2016 at 2:21 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > When calling show_stack_log_lvl() or dump_trace() with a regs argument,
>> > providing a stack pointer or frame pointer is redundant.
>> >
>>
>> > diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
>> > index 358fe1c..c533b8b 100644
>> > --- a/arch/x86/kernel/dumpstack_32.c
>> > +++ b/arch/x86/kernel/dumpstack_32.c
>> > @@ -122,7 +122,7 @@ void show_regs(struct pt_regs *regs)
>> >                 u8 *ip;
>> >
>> >                 pr_emerg("Stack:\n");
>> > -               show_stack_log_lvl(NULL, regs, &regs->sp, 0, KERN_EMERG);
>> > +               show_stack_log_lvl(NULL, regs, NULL, 0, KERN_EMERG);
>>
>> This is weird -- note the &.  You're at some risk of exposing a bug in
>> x86_32's kernel_stack_pointer() function, which is a mess.  (I don't
>> see why it's written the way it is -- the actual return stack pointer
>> given a pt_regs is quite well defined -- if regs->cs & 3 != 0, then
>> it's regs->sp, else it's &regs->sp.)
>>
>> That being said, this isn't a big deal, so:
>>
>> Reviewed-by: Andy Lutomirski <luto@kernel.org>
>>
>> If you want to make this all a bit more reliably on x86_32, you could
>> fix kernel_stack_pointer().
>
> Ok.  The whole '&regs->sp' thing threw me for a loop.  I have no idea
> what kernel_stack_pointer() is trying to do.  I just assumed it was
> correct.  I'll take a look at it and try to fix it in another patch.

On 32-bit, when an interrupt doesn't change CPL, SS:ESP is not pushed.
So, effectively, the old stack pointer is &regs->sp.

--
Brian Gerst

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web