From: Vernon Lovejoy <[email protected]>
The commit e335bb51cc15 ("x86/unwind: Ensure stack pointer is aligned")
tried to align the stack pointer in show_trace_log_lvl(), otherwise the
"stack < stack_info.end" check can't guarantee that the last read does
not go past the end of the stack.
However, we have the same problem with the initial value of the stack
pointer, it can also be unaligned. So without this patch this trivial
kernel module
#include <linux/module.h>
static int init(void)
{
asm volatile("sub $0x4,%rsp");
dump_stack();
asm volatile("add $0x4,%rsp");
return -EAGAIN;
}
module_init(init);
MODULE_LICENSE("GPL");
crashes the kernel.
Fixes: e335bb51cc15 ("x86/unwind: Ensure stack pointer is aligned")
Signed-off-by: Vernon Lovejoy <[email protected]>
Signed-off-by: Oleg Nesterov <[email protected]>
---
arch/x86/kernel/dumpstack.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 0bf6779187dd..71ab445a29c3 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -214,6 +214,7 @@ static void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
* - hardirq stack
* - entry stack
*/
+ stack = PTR_ALIGN(stack, sizeof(long));
for ( ; stack; stack = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
const char *stack_name;
--
2.25.1.362.g51ebf55
On Thu, Apr 27, 2023 at 04:00:54PM +0200, Oleg Nesterov wrote:
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index 0bf6779187dd..71ab445a29c3 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -214,6 +214,7 @@ static void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
> * - hardirq stack
> * - entry stack
> */
> + stack = PTR_ALIGN(stack, sizeof(long));
> for ( ; stack; stack = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
> const char *stack_name;
Seems reasonable, though 'stack' is already initialized a few lines
above this, so it would be cleaner to do the PTR_ALIGN then. Or even
better, just move it all to the for loop:
for (stack = PTR_ALIGN(stack ? : get_stack_pointer(task, regs));
stack;
stack = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
--
Josh
On 04/27, Josh Poimboeuf wrote:
>
> On Thu, Apr 27, 2023 at 04:00:54PM +0200, Oleg Nesterov wrote:
> > + stack = PTR_ALIGN(stack, sizeof(long));
> > for ( ; stack; stack = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
> > const char *stack_name;
>
> Seems reasonable, though 'stack' is already initialized a few lines
> above this, so it would be cleaner to do the PTR_ALIGN then. Or even
> better, just move it all to the for loop:
>
> for (stack = PTR_ALIGN(stack ? : get_stack_pointer(task, regs));
> stack;
> stack = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
We decided to make the simplest one-liner fix, but I was thinking about
for ( stack = stack ? : get_stack_pointer(task, regs);
(stack = PTR_ALIGN(stack, sizeof(long)));
stack = stack_info.next_sp)
{
...
to factout out the annoying PTR_ALIGN(). Will it work for you?
Oleg.
On Fri, Apr 28, 2023 at 08:55:13AM +0200, Oleg Nesterov wrote:
> On 04/27, Josh Poimboeuf wrote:
> >
> > On Thu, Apr 27, 2023 at 04:00:54PM +0200, Oleg Nesterov wrote:
> > > + stack = PTR_ALIGN(stack, sizeof(long));
> > > for ( ; stack; stack = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
> > > const char *stack_name;
> >
> > Seems reasonable, though 'stack' is already initialized a few lines
> > above this, so it would be cleaner to do the PTR_ALIGN then. Or even
> > better, just move it all to the for loop:
> >
> > for (stack = PTR_ALIGN(stack ? : get_stack_pointer(task, regs));
> > stack;
> > stack = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
>
> We decided to make the simplest one-liner fix, but I was thinking about
>
> for ( stack = stack ? : get_stack_pointer(task, regs);
> (stack = PTR_ALIGN(stack, sizeof(long)));
> stack = stack_info.next_sp)
> {
> ...
>
> to factout out the annoying PTR_ALIGN(). Will it work for you?
I'd rather not, that's a little *too* clever, IMO.
--
Josh
On 04/28, Josh Poimboeuf wrote:
>
> On Fri, Apr 28, 2023 at 08:55:13AM +0200, Oleg Nesterov wrote:
> >
> > We decided to make the simplest one-liner fix, but I was thinking about
> >
> > for ( stack = stack ?: get_stack_pointer(task, regs);
> > (stack = PTR_ALIGN(stack, sizeof(long)));
> > stack = stack_info.next_sp)
> > {
> > ...
> >
> > to factout out the annoying PTR_ALIGN(). Will it work for you?
>
> I'd rather not, that's a little *too* clever, IMO.
To me
for (stack = PTR_ALIGN(stack ?: get_stack_pointer(task, regs), sizeof(long));
stack;
stack = PTR_ALIGN(stack_info.next_sp, sizeof(long)))
certainly looks less readable (and more "clever" ;) but I won't argue with
maintainer. Please see V2.
Oleg.
From: Josh Poimboeuf
> Sent: 29 April 2023 00:58
>
> On Fri, Apr 28, 2023 at 08:55:13AM +0200, Oleg Nesterov wrote:
> > On 04/27, Josh Poimboeuf wrote:
> > >
> > > On Thu, Apr 27, 2023 at 04:00:54PM +0200, Oleg Nesterov wrote:
> > > > + stack = PTR_ALIGN(stack, sizeof(long));
> > > > for ( ; stack; stack = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
> > > > const char *stack_name;
> > >
> > > Seems reasonable, though 'stack' is already initialized a few lines
> > > above this, so it would be cleaner to do the PTR_ALIGN then. Or even
> > > better, just move it all to the for loop:
> > >
> > > for (stack = PTR_ALIGN(stack ? : get_stack_pointer(task, regs));
> > > stack;
> > > stack = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
> >
> > We decided to make the simplest one-liner fix, but I was thinking about
> >
> > for ( stack = stack ? : get_stack_pointer(task, regs);
> > (stack = PTR_ALIGN(stack, sizeof(long)));
> > stack = stack_info.next_sp)
> > {
> > ...
> >
> > to factout out the annoying PTR_ALIGN(). Will it work for you?
>
> I'd rather not, that's a little *too* clever, IMO.
I'd leave the initialisation outside the loop and move
the PTR_ALIGN() into the loop so that the 'for' fits on one line:
if (!stack)
stack = get_stack_pointer(task, regs);
for (; stack; stack = stack_info.next_sp) {
const char ...
stack = PTR_ALIGN(stack, sizeof(long));
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On 04/30, David Laight wrote:
>
> From: Josh Poimboeuf
> > Sent: 29 April 2023 00:58
> >
> > On Fri, Apr 28, 2023 at 08:55:13AM +0200, Oleg Nesterov wrote:
> > > On 04/27, Josh Poimboeuf wrote:
> > > >
> > > > On Thu, Apr 27, 2023 at 04:00:54PM +0200, Oleg Nesterov wrote:
> > > > > + stack = PTR_ALIGN(stack, sizeof(long));
> > > > > for ( ; stack; stack = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
> > > > > const char *stack_name;
> > > >
> > > > Seems reasonable, though 'stack' is already initialized a few lines
> > > > above this, so it would be cleaner to do the PTR_ALIGN then. Or even
> > > > better, just move it all to the for loop:
> > > >
> > > > for (stack = PTR_ALIGN(stack ? : get_stack_pointer(task, regs));
> > > > stack;
> > > > stack = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
> > >
> > > We decided to make the simplest one-liner fix, but I was thinking about
> > >
> > > for ( stack = stack ? : get_stack_pointer(task, regs);
> > > (stack = PTR_ALIGN(stack, sizeof(long)));
> > > stack = stack_info.next_sp)
> > > {
> > > ...
> > >
> > > to factout out the annoying PTR_ALIGN(). Will it work for you?
> >
> > I'd rather not, that's a little *too* clever, IMO.
>
> I'd leave the initialisation outside the loop and move
> the PTR_ALIGN() into the loop so that the 'for' fits on one line:
> if (!stack)
> stack = get_stack_pointer(task, regs);
> for (; stack; stack = stack_info.next_sp) {
> const char ...
> stack = PTR_ALIGN(stack, sizeof(long));
Well to me this looks better than V2 I've sent. Although to be honest I'd
prefer the initial one-liner fix, but this doesn't mater.
I am fine either way, I guess Vernon too. So I leave this to you and Josh,
please tell us if we should send V3 or not.
Oleg.
On Sun, Apr 30, 2023 at 11:59:17AM +0000, David Laight wrote:
> From: Josh Poimboeuf
> > Sent: 29 April 2023 00:58
> >
> > On Fri, Apr 28, 2023 at 08:55:13AM +0200, Oleg Nesterov wrote:
> > > On 04/27, Josh Poimboeuf wrote:
> > > >
> > > > On Thu, Apr 27, 2023 at 04:00:54PM +0200, Oleg Nesterov wrote:
> > > > > + stack = PTR_ALIGN(stack, sizeof(long));
> > > > > for ( ; stack; stack = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
> > > > > const char *stack_name;
> > > >
> > > > Seems reasonable, though 'stack' is already initialized a few lines
> > > > above this, so it would be cleaner to do the PTR_ALIGN then. Or even
> > > > better, just move it all to the for loop:
> > > >
> > > > for (stack = PTR_ALIGN(stack ? : get_stack_pointer(task, regs));
> > > > stack;
> > > > stack = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
> > >
> > > We decided to make the simplest one-liner fix, but I was thinking about
> > >
> > > for ( stack = stack ? : get_stack_pointer(task, regs);
> > > (stack = PTR_ALIGN(stack, sizeof(long)));
> > > stack = stack_info.next_sp)
> > > {
> > > ...
> > >
> > > to factout out the annoying PTR_ALIGN(). Will it work for you?
> >
> > I'd rather not, that's a little *too* clever, IMO.
>
> I'd leave the initialisation outside the loop and move
> the PTR_ALIGN() into the loop so that the 'for' fits on one line:
> if (!stack)
> stack = get_stack_pointer(task, regs);
> for (; stack; stack = stack_info.next_sp) {
> const char ...
> stack = PTR_ALIGN(stack, sizeof(long));
I do like that better, except... put the initialization in the 'for':
for (stack = stack ? : get_stack_pointer(task, regs);
stack;
stack = stack_info.next_sp) {
const char ...
stack = PTR_ALIGN(stack, sizeof(long));
A multi-line 'for' is fine, it's better to put the initialization in the
conventional spot.
--
Josh
On 05/11, Josh Poimboeuf wrote:
>
> I do like that better, except... put the initialization in the 'for':
>
> for (stack = stack ? : get_stack_pointer(task, regs);
> stack;
> stack = stack_info.next_sp) {
> const char ...
> stack = PTR_ALIGN(stack, sizeof(long));
>
please see v3.
Oleg.