From owner-svn-src-all@freebsd.org Sat May 5 04:21:16 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 902C0FC8A29; Sat, 5 May 2018 04:21:16 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail109.syd.optusnet.com.au (mail109.syd.optusnet.com.au [211.29.132.80]) by mx1.freebsd.org (Postfix) with ESMTP id 867AE6E8F3; Sat, 5 May 2018 04:21:15 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail109.syd.optusnet.com.au (Postfix) with ESMTPS id C411DD6F3AD; Sat, 5 May 2018 14:21:06 +1000 (AEST) Date: Sat, 5 May 2018 14:21:05 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Warner Losh cc: Steven Hartland , Mateusz Guzik , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r333266 - head/sys/amd64/amd64 In-Reply-To: Message-ID: <20180505135330.Y2917@besplex.bde.org> References: <201805042241.w44MfC9E090893@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=cIaQihWN c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=oXPNzy3kUUnsnLgNDLwA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 05 May 2018 04:21:16 -0000 On Fri, 4 May 2018, Warner Losh wrote: > While i wholeheartedly agree, an earlier commit explained why in this case. > It allows the compiler to inline for small copies. [This should have been killfiled due to top-posting.] No, it has nothing to do with that. The compiler can inline either bcopy() or memcpy(), and does so after related recent changes that implement bcopy() using builtins. What this does is work around bugs in the recent changes, and compiler bugs. bcopy() handles overlapping copies, but memcpy() doesn't, so it is not just a style bug to use memcpy() instead of bcopy(). However, if bcopy() is exposed to the compiler using builtins (note that -ffreestanding prevents even memcpy() being known to the compiler unless memcpy() is spelled __builtin_memcpy(), and there are similar but larger complications for bcopy()) then the compiler can almost always see when overlap occurs and further reduce bcopy() to __builtin_memcpy(). The full reduction should be: bcopy() -> __builtin_memmove() -> __builtin_memcpy() -> { either inline code or a call to __memcpy() } but the actual reduction is: bcopy() -> { __builtin_memmove() for small sizes, else bcopy() (bug1) } -> { __builtin_memcpy() if inlined, else a call to memmove() (bugs2,3) }, else bcopy() where bug1 is an implementation bug (not telling the compiler about large copies), bug2 is a compiler bug (forgetting that it reduced to *memcpy when calling the library to handle large sizes), and bug3 is a compiler bug (calling the library function memmove() which might be unrelated in the freestanding case). Fixing the implementation and compiler would optimize thousands of calls to bcopy() without churning the source code, but I'm a bit worried about security bugs from this. The security bugs are larger for bzero(). When bzero() was not exposed to the compiler as __builtin_memset(), it was guaranteed to always zero memory so there was no need for explicit_bzero() or explicit_memset() in the kernel, and using these was just a style bug, as is using memset() to 0 instead of bzero(). Usually the compiler doesn't have enough information to optimize away __builtin_memset(). But changing thousands of bzero()s to __builtin_memset()s is likely to find a couple of cases where the compiler does do this optimization and it breaks security. Bruce