1. Patchew
  2. linux
  3. View series

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

Oleg Nesterov posted 1 patch 2 years, 9 months ago
There is a newer version of this series
arch/x86/kernel/dumpstack.c | 1 +
1 file changed, 1 insertion(+)
[PATCH] x86/show_trace_log_lvl: ensure stack pointer is aligned, again
Posted by Oleg Nesterov 2 years, 9 months ago 
From: Vernon Lovejoy <vlovejoy@redhat.com>

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 <vlovejoy@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 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
Re: [PATCH] x86/show_trace_log_lvl: ensure stack pointer is aligned, again
Posted by Josh Poimboeuf 2 years, 9 months ago 
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
Re: [PATCH] x86/show_trace_log_lvl: ensure stack pointer is aligned, again
Posted by Oleg Nesterov 2 years, 9 months ago 
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.
Re: [PATCH] x86/show_trace_log_lvl: ensure stack pointer is aligned, again
Posted by Josh Poimboeuf 2 years, 9 months ago 
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
RE: [PATCH] x86/show_trace_log_lvl: ensure stack pointer is aligned, again
Posted by David Laight 2 years, 9 months ago 
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)
Re: [PATCH] x86/show_trace_log_lvl: ensure stack pointer is aligned, again
Posted by Josh Poimboeuf 2 years, 9 months ago 
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
Re: [PATCH] x86/show_trace_log_lvl: ensure stack pointer is aligned, again
Posted by Oleg Nesterov 2 years, 9 months ago 
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.
Re: [PATCH] x86/show_trace_log_lvl: ensure stack pointer is aligned, again
Posted by Oleg Nesterov 2 years, 9 months ago 
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.
Re: [PATCH] x86/show_trace_log_lvl: ensure stack pointer is aligned, again
Posted by Oleg Nesterov 2 years, 9 months ago 
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.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.