x86/show_trace_log_lvl: ensure stack pointer is aligned, again

PATCH Superseded Oleg Nesterov 4 participants 9 messages Apr 27, 2023 – May 12, 2023

Subsystems (2)

x86 x86 stack unwinding

lore

Share & tools

lore

Oleg Nesterov

author

2023-04-27 14:02 UTC #0 lore raw

PATCH 1/1 x86/show_trace_log_lvl: ensure stack pointer is aligned, again

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(+)

arch/x86/kernel/dumpstack.c +1 -0

index 0bf6779187dd..71ab445a29c3 100644
@@ -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;

Signature · 1 line

2.25.1.362.g51ebf55

Josh Poimboeuf

maintainer

2023-04-28 04:32 UTC #1 lore raw

arch/x86/kernel/dumpstack.c static void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs, · line 219

Quoted 11 lines · Oleg Nesterov

arch/x86/kernel/dumpstack.c +1 -0

index 0bf6779187dd..71ab445a29c3 100644
@@ -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))) {

Signature · 1 line

Josh

Oleg Nesterov

author

2023-04-28 06:56 UTC #2 lore raw

Quoted 10 lines · Josh Poimboeuf

arch/x86/kernel/dumpstack.c static void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs, · line 219
Quoted 3 lines · Oleg Nesterov
arch/x86/kernel/dumpstack.c +1 -0
+	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:

Quoted 3 lines

	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.

Josh Poimboeuf

maintainer

2023-04-28 23:57 UTC #3 lore raw

Quoted 17 lines · Oleg Nesterov

Quoted 10 lines · Josh Poimboeuf
arch/x86/kernel/dumpstack.c static void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs, · line 219
Quoted 3 lines · Oleg Nesterov
arch/x86/kernel/dumpstack.c +1 -0
+	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:
Quoted 3 lines
	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

Quoted 7 lines

	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.

Signature · 1 line

Josh

Oleg Nesterov

author

2023-04-29 10:09 UTC #4 lore raw

Quoted 11 lines · Josh Poimboeuf

Quoted 9 lines · Oleg Nesterov
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)
	{
		...

Quoted 3 lines

Quoted 1 line
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.

David Laight

2023-04-30 11:59 UTC #5 lore raw

In reply to Josh Poimboeuf

From: Josh Poimboeuf

Quoted 26 lines

Sent: 29 April 2023 00:58
Quoted 23 lines · Oleg Nesterov
Quoted 10 lines · Josh Poimboeuf
arch/x86/kernel/dumpstack.c static void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs, · line 219
Quoted 3 lines · Oleg Nesterov
arch/x86/kernel/dumpstack.c +1 -0
+	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:
Quoted 3 lines
	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)
	{
		...

Quoted 3 lines

Quoted 1 line
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)

Oleg Nesterov

author

2023-04-30 19:01 UTC #6 lore raw

Quoted 32 lines · David Laight

From: Josh Poimboeuf
Quoted 26 lines
Sent: 29 April 2023 00:58
Quoted 23 lines · Oleg Nesterov
Quoted 10 lines · Josh Poimboeuf
arch/x86/kernel/dumpstack.c static void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs, · line 219
Quoted 3 lines · Oleg Nesterov
arch/x86/kernel/dumpstack.c +1 -0
+	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:
Quoted 3 lines
	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)
	{
		...
Quoted 3 lines
Quoted 1 line
to factout out the annoying PTR_ALIGN(). Will it work for you?
I'd rather not, that's a little *too* clever, IMO.

Quoted 7 lines

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.

Josh Poimboeuf

2023-05-12 02:22 UTC #7 lore raw

In reply to David Laight

Quoted 31 lines · David Laight

From: Josh Poimboeuf
Quoted 26 lines
Sent: 29 April 2023 00:58
Quoted 23 lines · Oleg Nesterov
Quoted 10 lines · Josh Poimboeuf
arch/x86/kernel/dumpstack.c static void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs, · line 219
Quoted 3 lines · Oleg Nesterov
arch/x86/kernel/dumpstack.c +1 -0
+	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:
Quoted 3 lines
	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)
	{
		...
Quoted 3 lines
Quoted 1 line
to factout out the annoying PTR_ALIGN(). Will it work for you?
I'd rather not, that's a little *too* clever, IMO.

Quoted 7 lines

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.

Signature · 1 line

Josh

Oleg Nesterov

author

2023-05-12 10:45 UTC #8 lore raw

Quoted 9 lines · Josh Poimboeuf

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.