linux-kernel - Re: [RFC 5/8] x86: refcount: prevent gcc distortions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <5AFBDA6302000078001C31AC@prv1-mh.provo.novell.com>
Date:   Wed, 16 May 2018 01:14:43 -0600
From:   "Jan Beulich" <JBeulich@...e.com>
To:     "Nadav Amit" <namit@...are.com>
Cc:     <keescook@...omium.org>, <nadav.amit@...il.com>,
        "the arch/x86 maintainers" <x86@...nel.org>, <tglx@...utronix.de>,
        "Josh Poimboeuf" <jpoimboe@...hat.com>, <mingo@...hat.com>,
        <linux-kernel@...r.kernel.org>, <hpa@...or.com>
Subject: Re: [RFC 5/8] x86: refcount: prevent gcc distortions

>>> On 15.05.18 at 16:11, <namit@...are.com> wrote:
> --- a/arch/x86/include/asm/refcount.h
> +++ b/arch/x86/include/asm/refcount.h
> @@ -14,34 +14,43 @@
>   * central refcount exception. The fixup address for the exception points
>   * back to the regular execution flow in .text.
>   */
> -#define _REFCOUNT_EXCEPTION				\
> -	".pushsection .text..refcount\n"		\
> -	"111:\tlea %[counter], %%" _ASM_CX "\n"		\
> -	"112:\t" ASM_UD2 "\n"				\
> -	ASM_UNREACHABLE					\
> -	".popsection\n"					\
> -	"113:\n"					\
> +
> +asm ("\n"
> +	".macro __REFCOUNT_EXCEPTION counter:vararg\n\t"
> +	".pushsection .text..refcount\n"
> +	"111:\tlea \\counter, %" _ASM_CX "\n"
> +	"112:\t" ASM_UD2 "\n\t"
> +	ASM_UNREACHABLE
> +	".popsection\n\t"
> +	"113:\n"
>  	_ASM_EXTABLE_REFCOUNT(112b, 113b)
> +	".endm");

A few comments on assembly code formatting - while gas at present is
relatively lax in this regard, I wouldn't exclude that there might be a
more strict mode in the future, and that such a mode might eventually
become the default. Furthermore these formatting aspects affect
readability of the assembly produced, should anyone ever find a need
to look at it (perhaps because of some breakage) - I certainly do every
once in a while.

Labels should be placed without any indentation (but of course there
may be more than one on a line, in which case subsequent ones may
of course be arbitrarily indented). Instructions and directives, otoh,
should be placed with at least a single tab or space of indentation
(unless preceded by a label, in which case the extra white space still
helps readability).

I'm also not sure about the purpose of the leading plain newline here.
gcc annotates code resulting from inline assembly anyway iirc, so
proper visual separation should already be available.

If I was the maintainer of this code, I would also object to the
mis-alignment your file scope asm()-s have ("asm (" is 5 characters,
which doesn't equal a tab's width).

Jan