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

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

[Oleg Nesterov]

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

[Josh Poimboeuf]

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

[Oleg Nesterov]

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

[Josh Poimboeuf]

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

[Oleg Nesterov]

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.

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

[David Laight]

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

[Oleg Nesterov]

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

[Josh Poimboeuf]

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

[Oleg Nesterov]

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.