Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 1 Nov 2022 15:36:23 -0700
From:   Kees Cook <keescook@chromium.org>
To:     bp@alien8.de, tglx@linutronix.de, mingo@kernel.org,
        peterz@infradead.org
Cc:     Nick Desaulniers <ndesaulniers@google.com>,
        Nathan Chancellor <nathan@kernel.org>,
        dave.hansen@linux.intel.com, David.Laight@aculab.com,
        hpa@zytor.com, linux-kernel@vger.kernel.org,
        linux@rasmusvillemoes.dk, llvm@lists.linux.dev, luto@kernel.org,
        mingo@redhat.com, torvalds@linux-foundation.org, x86@kernel.org
Subject: Re: [RESEND PATCH v5] x86, mem: move memmove to out of line assembler
Message-ID: <202211011534.C31FC5ED6@keescook>
References: <20221018172155.287409-1-ndesaulniers@google.com>
 <Y08NJohEeoYX2aIf@thelio-3990X>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y08NJohEeoYX2aIf@thelio-3990X>
Precedence: bulk

On Tue, Oct 18, 2022 at 01:31:34PM -0700, Nathan Chancellor wrote:
> On Tue, Oct 18, 2022 at 10:21:55AM -0700, Nick Desaulniers wrote:
> > When building ARCH=i386 with CONFIG_LTO_CLANG_FULL=y, it's possible
> > (depending on additional configs which I have not been able to isolate)
> > to observe a failure during register allocation:
> > 
> >   error: inline assembly requires more registers than available
> > 
> > when memmove is inlined into tcp_v4_fill_cb() or tcp_v6_fill_cb().
> > 
> > memmove is quite large and probably shouldn't be inlined due to size
> > alone. A noinline function attribute would be the simplest fix, but
> > there's a few things that stand out with the current definition:
> > 
> > In addition to having complex constraints that can't always be resolved,
> > the clobber list seems to be missing %bx. By using numbered operands
> > rather than symbolic operands, the constraints are quite obnoxious to
> > refactor.
> > 
> > Having a large function be 99% inline asm is a code smell that this
> > function should simply be written in stand-alone out-of-line assembler.
> > 
> > Moving this to out of line assembler guarantees that the
> > compiler cannot inline calls to memmove.
> > 
> > This has been done previously for 64b:
> > commit 9599ec0471de ("x86-64, mem: Convert memmove() to assembly file
> > and fix return value bug")
> > 
> > That gives the opportunity for other cleanups like fixing the
> > inconsistent use of tabs vs spaces and instruction suffixes, and the
> > label 3 appearing twice.  Symbolic operands, local labels, and
> > additional comments would provide this code with a fresh coat of paint.
> > 
> > Finally, add a test that tickles the `rep movsl` implementation to test
> > it for correctness, since it has implicit operands.
> > 
> > Suggested-by: Ingo Molnar <mingo@kernel.org>
> > Suggested-by: David Laight <David.Laight@aculab.com>
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Tested-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> 
> I ran
> 
> $ tools/testing/kunit/kunit.py run --arch i386 --cross_compile x86_64-linux- memcpy
> 
> with GCC 6 through 12 from
> https://mirrors.edge.kernel.org/pub/tools/crosstool/ (my GCC 5 container
> is based on Ubuntu Xenial, which does not have a new enough Python for
> kunit.py) and
> 
> $ tools/testing/kunit/kunit.py run --arch i386 --make_options LLVM=1 memcpy
> 
> with LLVM 11 through 16 from Debian with this change on top of Kees's
> expanding of the memcpy() KUnit tests [1] and everything passed.
> 
> Tested-by: Nathan Chancellor <nathan@kernel.org>

Can an x86 maintainer please pick this up for -tip?

Thanks!

-Kees

-- 
Kees Cook