Messages in this thread | | | Date | Wed, 28 Sep 2022 15:03:43 -0700 | From | Kees Cook <> | Subject | Re: [PATCH v4] x86, mem: move memmove to out of line assembler |
| |
On Wed, Sep 28, 2022 at 02:05:12PM -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 and %dx, and possibly %cl. 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. > 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 and local labels would > provide this code with a fresh coat of paint. > > 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") > > Also, add a test that tickles the `rep movsl` implementation to test it > for correctness, since it has implicit operands.
Yeah, thanks for poking this in particular. I was bothered that the side-effect test caught a corner case and was planning to expand the memcpy tests even more; thank you for doing that! I've got some more coming and can confirm they tickled the same bug.
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
This time I've looked at the binary differences between the functions generated by both GCC[1] and Clang[2]. GCC is a little more difficult to compare, since it does some register swaps, but the Clang output is same excepting the order of push/pop, and different nops.
Reviewed-by: Kees Cook <keescook@chromium.org>
Nick's tests pass, and my newly written tests also pass; I'll send those as a follow-up.
Tested-by: Kees Cook <keescook@chromium.org>
-Kees
[1] https://paste.debian.net/hidden/b6298e62/ [2] https://paste.debian.net/hidden/d8343143/
-- Kees Cook
| |