From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966219AbcKKA31 (ORCPT ); Thu, 10 Nov 2016 19:29:27 -0500 Received: from out4-smtp.messagingengine.com ([66.111.4.28]:50780 "EHLO out4-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965254AbcKKA30 (ORCPT ); Thu, 10 Nov 2016 19:29:26 -0500 X-ME-Sender: X-Sasl-enc: xxHvf9ly8drAOrQVvYygViRQ7LbGwqG4jh0/Bqox4o7b 1478824163 Message-ID: <1478824161.7326.5.camel@cvidal.org> Subject: Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC From: Colin Vidal To: kernel-hardening@lists.openwall.com, Kees Cook , Peter Zijlstra Cc: Greg KH , Will Deacon , Elena Reshetova , Arnd Bergmann , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , LKML Date: Fri, 11 Nov 2016 01:29:21 +0100 In-Reply-To: <20161110235714.GR3568@worktop.programming.kicks-ass.net> References: <1478809488-18303-1-git-send-email-elena.reshetova@intel.com> <20161110203749.GV3117@twins.programming.kicks-ass.net> <20161110204838.GE17134@arm.com> <20161110211310.GX3117@twins.programming.kicks-ass.net> <20161110222744.GD8086@kroah.com> <20161110235714.GR3568@worktop.programming.kicks-ass.net> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.5 (3.20.5-1.fc24) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 2016-11-11 at 00:57 +0100, Peter Zijlstra wrote: > On Thu, Nov 10, 2016 at 03:15:44PM -0800, Kees Cook wrote: > > > > On Thu, Nov 10, 2016 at 2:27 PM, Greg KH wrote: > > > > > > On Thu, Nov 10, 2016 at 10:13:10PM +0100, Peter Zijlstra wrote: > > > > > > > > > > > > > > As it stands kref is a pointless wrapper. If it were to provide > > > > something actually useful, like wrap protection, then it might actually > > > > make sense to use it. > > > > > > It provides the correct cleanup ability for a reference count and the > > > object it is in, so it's not all that pointless :) > > I really fail to see the point of: > > kref_put(&obj->ref, obj_free); > > over: > > if (atomic_dec_and_test(&obj->ref)) > obj_free(obj); > > Utter pointless wrappery afaict. > > > > > > > > > But I'm always willing to change it to make it work better for people, > > > if kref did the wrapping protection (i.e. used a non-wrapping atomic > > > type), then you would have that. I thought that was what this patchset > > > provided... > > So kref could simply do something like the below patch. But this patch > set completely rapes the atomic interface. > > > > > > > > > And yes, this is a horridly large patchset. I've looked at these > > > changes, and in almost all of them, people are using atomic_t as merely > > > a "counter" for something (sequences, rx/tx stats, etc), to get away > > > without having to lock it with an external lock. > > > > > > So, does it make more sense to just provide a "pointless" api for this > > > type of "counter" pattern: > > > counter_inc() > > > counter_dec() > > > counter_read() > > > counter_set() > > > counter_add() > > > counter_subtract() > > > Those would use the wrapping atomic type, as they can wrap all they want > > > and no one really is in trouble. Once those changes are done, just make > > > atomic_t not wrap and all should be fine, no other code should need to > > > be changed. > > Still hate; yes there's a lot of stats which are just fine to wrap. But > there's a lot more atomic out there than refcounts and stats. > > The locking primitives for example use atomic_t, and they really don't > want the extra overhead associated with this overflow crap, or the > namespace pollution. > > > > > reference counters (say, "refcount" implemented with new atomic_nowrap_t) > > > > statistic counters (say, "statcount" implemented with new atomic_wrap_t) > > > > everything else (named "atomic_t", implemented as either > > atomic_nowrap_t or atomic_wrap_t, depending on CONFIG) > > So the problem is that atomic_t has _much_ _much_ more than just add/sub > operations, which are the only ones modified for this patch set. > > The whole wrap/nowrap thing doesn't make any bloody sense what so ever > for !arith operators like bitops or just plain cmpxchg. I wonder if we didn't make a confusion between naming and specifications. I have thought about Kees idea and what you're saying: - The name "atomic_t" name didn't tell anything about if the variable   can wrap or not. It just tells there is no race condition on   concurrent access, nothing else, and users are well with that. OK   then, we don't modify atomic_t, it makes sense. - Hence, let's say a new type "refcount_t". It names exactly what we   try to protect in this patch set. A much more simpler interface than   atomic_t would be needed, and it protects on race condition and   overflows (precisely what is expected of a counter reference). Not   an opt-in solution, but it is much less invasive since we "just"   have to modify the kref implementation and some vfs reference   counters. That didn't tell us how actually implements refcount_t: reuse some atomic_t code or not (it would be simpler anyways, since we don't have to implement the whole atomic_t interface). Still, this is another problem. Sounds better? Thanks, Colin