From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753286AbcBWOcw (ORCPT ); Tue, 23 Feb 2016 09:32:52 -0500 Received: from mail.efficios.com ([78.47.125.74]:54470 "EHLO mail.efficios.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753108AbcBWOcu (ORCPT ); Tue, 23 Feb 2016 09:32:50 -0500 Date: Tue, 23 Feb 2016 14:32:43 +0000 (UTC) From: Mathieu Desnoyers To: "Paul E. McKenney" Cc: Peter Zijlstra , Will Deacon , Linus Torvalds , LKML , Stephan Mueller , Daniel Borkmann , "Theodore Ts'o" , Hannes Frederic Sowa , mancha security , Mark Charlebois , Behan Webster , Herbert Xu Message-ID: <675593567.5328.1456237963352.JavaMail.zimbra@efficios.com> In-Reply-To: <20160223142336.GK3522@linux.vnet.ibm.com> References: <1168693945.5302.1456236146207.JavaMail.zimbra@efficios.com> <20160223142336.GK3522@linux.vnet.ibm.com> Subject: Re: interesting commit about llvm introducing barrier_data() MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [78.47.125.74] X-Mailer: Zimbra 8.6.0_GA_1178 (ZimbraWebClient - FF44 (Linux)/8.6.0_GA_1178) Thread-Topic: interesting commit about llvm introducing barrier_data() Thread-Index: KFukXGuMy0bxfNU1dH/5c981seFV5g== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org ----- On Feb 23, 2016, at 9:23 AM, Paul E. McKenney paulmck@linux.vnet.ibm.com wrote: > On Tue, Feb 23, 2016 at 02:02:26PM +0000, Mathieu Desnoyers wrote: >> commit 7829fb09a2b4268b30dd9bc782fa5ebee278b137 >> Author: Daniel Borkmann >> Date: Thu Apr 30 04:13:52 2015 +0200 >> >> lib: make memzero_explicit more robust against dead store elimination >> >> ^ interesting commit. Any idea on the impact of this on kernel RCU >> implementation and liburcu cmm_barrier() ? > > First I knew of it! But I bet that more like this are needed. ;-) I recommend you check my IRC discussion with peterz on the matter of this new "barrier_data()". Let us know if we are missing anything, (opening up the discussion to a larger audience) 09:03 < Compudj> peterz: have you seem commit 7829fb09a2b4268b30dd9bc782fa5ebee278b137 "lib: make memzero_explicit more robust against dead store elimination" which introduces a barrier_data() ? Any thoughts on its impact on preempt disable ? 09:04 < Compudj> (icc/llvm related) 09:04 * peterz goes look 09:06 < peterz> yuck 09:06 < Compudj> yeah, that sucks :/ 09:09 < Compudj> but it makes me wonder if that patch is not just bogus... 09:09 < Compudj> I tries to keep a memset to zero from being eliminated as a "dead store" 09:10 < Compudj> but if the compiler can prove that the zeroed memory is only ever in registers, and never used otherwise (in read), it could very well eliminate the zeroing 09:10 < Compudj> the only issue I see is with DMA writes 09:10 < peterz> but the whole point of barrier() was to be a compiler barrier, that is force emit prior stores and reads, such that subsequent code can rely on it 09:11 < peterz> imagine it being a function call 09:11 < peterz> no saying what inside the function call will touch buf 09:11 < Compudj> yes, but DMA-aside, if no subsequent code need to access what has been written (proven by the compiler), do we care about the store ? 09:11 < Compudj> peterz: in this case I would expect the compiler to be conservative, and assume nothing 09:12 < peterz> hmm, so the example uses a function local buffer, whose address is never taken 09:13 < Compudj> peterz: yes... hard to be more local than that 09:14 < Compudj> it really starts to look like a bogus "fix" for an issue that has never been really observed outside of this peculiar trivial example 09:14 < peterz> and I suppose the whole point here is to wipe keys from memory, even if local? 09:14 < Compudj> "Arguably, in the current kernel tree it's more of a theoretical issue, but imho, it's better to be pedantic about it. 09:14 < Compudj> " 09:15 < Compudj> not even sure why they care 09:15 < peterz> no, if llvm goes around and breaks all kinds of assumptions, it simply isn't a viable compiler for the kernel, full stop 09:16 < Compudj> perhaps, but I don't see a clear explanation of *why* this would be a bogus behavior in the patch changelog 09:16 < Compudj> besides the trivial main() example with an array on the stack 09:17 < Compudj> I would expect the compiler to be able to optimize away this store without hurting anything if it can prove that nobody cares about this stack content 09:19 < peterz> right, so the 'real' case I can imagine, is if the memset is the last time before the variable goes out of scope 09:19 < peterz> s/time/thing/ 09:19 -!- acme [~acme@187.65.76.58] has joined #linux-rt 09:19 < peterz> given the purpose of not leaking the 'key' for longer than needed, you want that memset to happen 09:20 < peterz> but the compiler can see its a 'pointless' store, since the variable is about to not exist after that 09:20 < Compudj> peterz: I can see the security concern indeed 09:20 < Compudj> but not as a correctness one.. ? 09:20 < peterz> right, not yet 09:20 < Compudj> so the only case where we should care about it is for memset then... AFAIU 09:20 < peterz> they need to do more crazy for correctness to be an issue 09:21 < Compudj> (that should be clearly documented above barrier_data()) 09:22 < Compudj> peterz: moreover, it would only "leak" a key in registers 09:22 < Compudj> not in memory 09:22 < Compudj> I'm kind of doubtful that it's really a threat 09:22 < peterz> no, it would actually leak it in memory 09:22 < peterz> it makes an 'as-if' argument 09:23 < Compudj> even with the memory asm clobber ? 09:24 < Compudj> that clobber really acts as if any memory could be touched 09:24 < Compudj> which cannot allow the compiler to assume the zeroes are untouched (if sitting in memory) 09:25 < peterz> "Yes, we will do this even if you use more bytes in 'buf' than you have bytes of registers (whether it's really in registers or memory is immaterial, what matters is that the compiler has proven that the inline asm may not rely on 'buf' being in memory)." -- comment 5 Thanks, Mathieu > > Thanx, Paul -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com