Re: [PATCH v5 1/2] x86/delay: Fix the wrong asm constraint in `delay_loop()`

From: Borislav Petkov <bp@alien8.de>
To: Ammar Faizi <ammarfaizi2@gnuweeb.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Alviro Iskandar Setiawan <alviro.iskandar@gmail.com>,
	Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>,
	Tony Luck <tony.luck@intel.com>,
	Yazen Ghannam <yazen.ghannam@amd.com>,
	linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org, gwml@vger.gnuweeb.org, x86@kernel.org,
	David Laight <David.Laight@aculab.com>,
	Jiri Hladky <hladky.jiri@googlemail.com>
Subject: Re: [PATCH v5 1/2] x86/delay: Fix the wrong asm constraint in `delay_loop()`
Date: Sun, 27 Mar 2022 23:38:43 +0200	[thread overview]
Message-ID: <YkDZY8n1k5SJw9st@zn.tnic> (raw)
In-Reply-To: <20220310015306.445359-2-ammarfaizi2@gnuweeb.org>

On Thu, Mar 10, 2022 at 08:53:05AM +0700, Ammar Faizi wrote:
> The asm constraint does not reflect that the asm statement can modify
> the value of @loops. But the asm statement in delay_loop() does modify
> the @loops.
> 
> Specifiying the wrong constraint may lead to undefined behavior, it may
> clobber random stuff (e.g. local variable, important temporary value in
> regs, etc.).

This is especially dangerous when the compiler decides to inline the
function and since it doesn't know that the value gets modified, it
might decide to use it from a register directly without reloading it.

Add that to the commit message pls.

> Fix this by changing the constraint from "a" (as an input) to "+a" (as
> an input and output).
> 
> Cc: David Laight <David.Laight@ACULAB.COM>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Jiri Hladky <hladky.jiri@googlemail.com>

All those Ccs in the commit message are not really needed -
get_maintainers.pl gives the correct list already.

> Cc: stable@vger.kernel.org # v2.6.27+

I don't see the need for the stable Cc. Or do you have a case where
a corruption really does happen?

> Fixes: e01b70ef3eb ("x86: fix bug in arch/i386/lib/delay.c file, delay_loop function")

Commit sha1 (e01b70ef3eb) needs to be at least 12 chars long:

e01b70ef3eb3 ("x86: fix bug in arch/i386/lib/delay.c file, delay_loop function")

This is best fixed by doing:

[core]
        abbrev = 12

in your .git/config

> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> ---
>  arch/x86/lib/delay.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/lib/delay.c b/arch/x86/lib/delay.c
> index 65d15df6212d..0e65d00e2339 100644
> --- a/arch/x86/lib/delay.c
> +++ b/arch/x86/lib/delay.c
> @@ -54,8 +54,8 @@ static void delay_loop(u64 __loops)
>  		"	jnz 2b		\n"
>  		"3:	dec %0		\n"
>  
> -		: /* we don't need output */
> -		:"a" (loops)
> +		: "+a" (loops)
> +		:
>  	);

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette