Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752527AbeAEQof (ORCPT + 1 other); Fri, 5 Jan 2018 11:44:35 -0500 Received: from mail-ot0-f180.google.com ([74.125.82.180]:42564 "EHLO mail-ot0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752348AbeAEQoU (ORCPT ); Fri, 5 Jan 2018 11:44:20 -0500 X-Google-Smtp-Source: ACJfBot1ou9ZqopLmVioXrbQ7OpHN8iIk0NrBkWlHYHH7vLWtR2bvWXcDWC1KWCtMordSoI2BmAa31APRTNiNROBjWs= MIME-Version: 1.0 In-Reply-To: <20180105144035.4hjrshigosb6s4zx@lakrids.cambridge.arm.com> References: <20180103223827.39601-1-mark.rutland@arm.com> <151502463248.33513.5960736946233335087.stgit@dwillia2-desk3.amr.corp.intel.com> <20180104010754.22ca6a74@alans-desktop> <1515035438.20588.4.camel@intel.com> <20180104114732.utkixumxt7rfuf3g@salmiak> <20180105144035.4hjrshigosb6s4zx@lakrids.cambridge.arm.com> From: Dan Williams Date: Fri, 5 Jan 2018 08:44:18 -0800 Message-ID: Subject: Re: [RFC PATCH] asm/generic: introduce if_nospec and nospec_barrier To: Mark Rutland Cc: "torvalds@linux-foundation.org" , "linux-kernel@vger.kernel.org" , "peterz@infradead.org" , "tglx@linutronix.de" , "alan@linux.intel.com" , "Reshetova, Elena" , "gnomes@lxorguk.ukuu.org.uk" , "gregkh@linuxfoundation.org" , "jikos@kernel.org" , "linux-arch@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Fri, Jan 5, 2018 at 6:40 AM, Mark Rutland wrote: > On Thu, Jan 04, 2018 at 02:09:52PM -0800, Dan Williams wrote: >> On Thu, Jan 4, 2018 at 3:47 AM, Mark Rutland wrote: >> > Hi Dan, >> > >> > Thanks for these examples. >> > >> > On Thu, Jan 04, 2018 at 03:10:51AM +0000, Williams, Dan J wrote: >> >> Note, the following is Elena's work, I'm just helping poke the upstream >> >> discussion along while she's offline. >> >> >> >> Elena audited the static analysis reports down to the following >> >> locations where userspace provides a value for a conditional branch and >> >> then later creates a dependent load on that same userspace controlled >> >> value. >> >> >> >> 'osb()', observable memory barrier, resolves to an lfence on x86. My >> >> concern with these changes is that it is not clear what content within >> >> the branch block is of concern. Peter's 'if_nospec' proposal combined >> >> with Mark's 'nospec_load' would seem to clear that up, from a self >> >> documenting code perspective, and let archs optionally protect entire >> >> conditional blocks or individual loads within those blocks. >> > >> > I'm a little concerned by having to use two helpers that need to be paired. It >> > means that we have to duplicate the bounds information, and I suspect in >> > practice that they'll often be paired improperly. >> >> Hmm, will they be often mispaired? All of the examples to date the >> load occurs in the same code block as the bound checking if() >> statement. > > Practically speaking, barriers get misused all the time, and having a > single helper minimizes the risk or misuse. I agree, but this is why if_nospec hides the barrier / makes it implicit. > >> > I think that we can avoid those problems if we use nospec_ptr() on its own. It >> > returns NULL if the pointer would be out-of-bounds, so we can use it in the >> > if-statement. >> > >> > On x86, that can contain the bounds checks followed be an OSB / lfence, like >> > if_nospec(). On arm we can use our architected sequence. In either case, >> > nospec_ptr() returns NULL if the pointer would be out-of-bounds. >> > >> > Then, rather than sequences like: >> > >> > if_nospec(idx < max) { >> > val = nospec_array_load(array, idx, max); >> > ... >> > } >> > >> > ... we could have: >> > >> > if ((elem_ptr = nospec_array_ptr(array, idx, max)) { >> > val = *elem_ptr; >> > ... >> > } >> > >> > ... which also means we don't have to annotate every single load in the branch >> > if the element is a structure with multiple fields. >> >> We wouldn't need to annotate each load in that case, right? Just the >> instance that uses idx to calculate an address. > > Correct. > >> if_nospec (idx < max_idx) { >> elem_ptr = nospec_array_ptr(array, idx, max); >> val = elem_ptr->val; >> val2 = elem_ptr->val2; >> } >> >> > Does that sound workable to you? >> >> Relative to the urgency of getting fixes upstream I'm ok with whatever >> approach generates the least debate from sub-system maintainers. >> >> One concern is that on x86 the: >> >> if ((elem_ptr = nospec_array_ptr(array, idx, max)) { >> >> ...approach produces two conditional branches whereas: >> >> if_nospec (idx < max_idx) { >> elem_ptr = nospec_array_ptr(array, idx, max); >> >> ....can be optimized to one conditional with the barrier. > > Do you mean because the NULL check is redundant? I was hoping that the > compiler would have the necessary visibility to fold that with the > bounds check (on the assumption that the array base must not be NULL). ...but there's legitimately 2 conditionals one to control the non-speculative flow, and one to sink the speculation ala the array_access() approach from Linus. Keeping those separate seems to lead to less change in the suspected areas. In any event I'll post the reworked patches and we can iterate from there.