Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 19 Aug 2020 11:14:34 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Arvind Sankar <nivedita@alum.mit.edu>,
        Ingo Molnar <mingo@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org,
        Nick Terrell <nickrterrell@gmail.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Yann Collet <yann.collet.73@gmail.com>,
        Gao Xiang <xiang@kernel.org>,
        Sven Schmidt <4sschmid@informatik.uni-hamburg.de>
Subject: Re: [PATCH 1/1] x86/boot/compressed: Use builtin mem functions for
 decompressor
Message-ID: <202008191111.4244B09D26@keescook>
References: <9753118F-84DD-44C4-8B8C-F45B8EE0FC14@fb.com>
 <20200804234817.3922187-1-nivedita@alum.mit.edu>
 <20200804234817.3922187-2-nivedita@alum.mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200804234817.3922187-2-nivedita@alum.mit.edu>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Aug 04, 2020 at 07:48:17PM -0400, Arvind Sankar wrote:
> Since commits
> 
>   c041b5ad8640 ("x86, boot: Create a separate string.h file to provide standard string functions")
>   fb4cac573ef6 ("x86, boot: Move memcmp() into string.h and string.c")
> 
> the decompressor stub has been using the compiler's builtin memcpy,
> memset and memcmp functions, _except_ where it would likely have the
> largest impact, in the decompression code itself.
> 
> Remove the #undef's of memcpy and memset in misc.c so that the
> decompressor code also uses the compiler builtins.
> 
> The rationale given in the comment doesn't really apply: just because
> some functions use the out-of-line version is no reason to not use the
> builtin version in the rest.
> 
> Replace the comment with an explanation of why memzero and memmove are
> being #define'd.
> 
> Drop the suggestion to #undef in boot/string.h as well: the out-of-line
> versions are not really optimized versions, they're generic code that's
> good enough for the preboot environment. The compiler will likely
> generate better code for constant-size memcpy/memset/memcmp if it is
> allowed to.
> 
> Most decompressors' performance is unchanged, with the exception of LZ4
> and 64-bit ZSTD.
> 
> 	Before	After ARCH
> LZ4	  73ms	 10ms   32
> LZ4	 120ms	 10ms	64
> ZSTD	  90ms	 74ms	64
> 
> Measurements on QEMU on 2.2GHz Broadwell Xeon, using defconfig kernels.
> 
> Decompressor code size has small differences, with the largest being
> that 64-bit ZSTD decreases just over 2k. The largest code size increase
> was on 64-bit XZ, of about 400 bytes.
> 
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> Suggested-by: Nick Terrell <nickrterrell@gmail.com>
> Tested-by: Nick Terrell <nickrterrell@gmail.com>

Did anyone pick this up? (Ingo can you snag it, or maybe akpm who took
the LZ4-specific patch already?) This looks sane to me and provides some
surprising performance benefits. :)

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  arch/x86/boot/compressed/misc.c | 7 ++-----
>  arch/x86/boot/string.h          | 5 +----
>  2 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index 39e592d0e0b4..e478e40fbe5a 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -30,12 +30,9 @@
>  #define STATIC		static
>  
>  /*
> - * Use normal definitions of mem*() from string.c. There are already
> - * included header files which expect a definition of memset() and by
> - * the time we define memset macro, it is too late.
> + * Provide definitions of memzero and memmove as some of the decompressors will
> + * try to define their own functions if these are not defined as macros.
>   */
> -#undef memcpy
> -#undef memset
>  #define memzero(s, n)	memset((s), 0, (n))
>  #define memmove		memmove
>  
> diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
> index 995f7b7ad512..a232da487cd2 100644
> --- a/arch/x86/boot/string.h
> +++ b/arch/x86/boot/string.h
> @@ -11,10 +11,7 @@ void *memcpy(void *dst, const void *src, size_t len);
>  void *memset(void *dst, int c, size_t len);
>  int memcmp(const void *s1, const void *s2, size_t len);
>  
> -/*
> - * Access builtin version by default. If one needs to use optimized version,
> - * do "undef memcpy" in .c file and link against right string.c
> - */
> +/* Access builtin version by default. */
>  #define memcpy(d,s,l) __builtin_memcpy(d,s,l)
>  #define memset(d,c,l) __builtin_memset(d,c,l)
>  #define memcmp	__builtin_memcmp
> -- 
> 2.26.2
> 

-- 
Kees Cook