From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751238AbdE2JSK (ORCPT ); Mon, 29 May 2017 05:18:10 -0400 Received: from out03.mta.xmission.com ([166.70.13.233]:52526 "EHLO out03.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750866AbdE2JSH (ORCPT ); Mon, 29 May 2017 05:18:07 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Christoph Hellwig Cc: Kees Cook , Andrew Morton , Elena Reshetova , Peter Zijlstra , Greg KH , Ingo Molnar , Alexey Dobriyan , "Serge E. Hallyn" , arozansk@redhat.com, Davidlohr Bueso , Manfred Spraul , "axboe\@kernel.dk" , James Bottomley , "x86\@kernel.org" , Ingo Molnar , Arnd Bergmann , "David S. Miller" , Rik van Riel , linux-arch , "kernel-hardening\@lists.openwall.com" , LKML References: <1487590189-18151-1-git-send-email-elena.reshetova@intel.com> <20170303162352.b6af1c0c3115b3f5f1e7aed3@linux-foundation.org> <20170529083903.GA17735@infradead.org> Date: Mon, 29 May 2017 04:11:13 -0500 In-Reply-To: <20170529083903.GA17735@infradead.org> (Christoph Hellwig's message of "Mon, 29 May 2017 01:39:04 -0700") Message-ID: <87h904xc26.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1dFGoD-0007gh-Ei;;;mid=<87h904xc26.fsf@xmission.com>;;;hst=in01.mta.xmission.com;;;ip=97.121.81.159;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX18qDv3ASZy0vW2052lHacBFmm4fwLOjYxA= X-SA-Exim-Connect-IP: 97.121.81.159 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.0 TVD_RCVD_IP Message was received from an IP address * 1.5 XMNoVowels Alpha-numberic number with no vowels * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa07 1397; Body=1 Fuz1=1 Fuz2=1] * 1.0 T_XMDrugObfuBody_04 obfuscated drug references X-Spam-DCC: XMission; sa07 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: **;Christoph Hellwig X-Spam-Relay-Country: X-Spam-Timing: total 5542 ms - load_scoreonly_sql: 0.06 (0.0%), signal_user_changed: 3.3 (0.1%), b_tie_ro: 2.4 (0.0%), parse: 1.16 (0.0%), extract_message_metadata: 13 (0.2%), get_uri_detail_list: 1.87 (0.0%), tests_pri_-1000: 7 (0.1%), tests_pri_-950: 1.19 (0.0%), tests_pri_-900: 1.01 (0.0%), tests_pri_-400: 26 (0.5%), check_bayes: 25 (0.4%), b_tokenize: 9 (0.2%), b_tok_get_all: 8 (0.1%), b_comp_prob: 2.5 (0.0%), b_tok_touch_all: 2.9 (0.1%), b_finish: 0.62 (0.0%), tests_pri_0: 293 (5.3%), check_dkim_signature: 0.55 (0.0%), check_dkim_adsp: 3.1 (0.1%), tests_pri_500: 5193 (93.7%), poll_dns_idle: 5187 (93.6%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH 0/3] ipc subsystem refcounter conversions X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Christoph Hellwig writes: > On Sat, May 27, 2017 at 12:58:14PM -0700, Kees Cook wrote: >> FAST_REFCOUNT=n: use function-based refcount_t with cmpxvhg and >> full-verification >> FAST_REFCOUNT=y without arch-specific implementation: use atomic_t >> with no verification (i.e. no functional change from now) >> FAST_REFCOUNT=y with arch-specific implementation: use atomic_t with >> overflow protection >> >> which means FAST_REFCOUNT would need to be default-on so that mm, >> block, net users will remain happy. >> >> Does that sound reasonable? > > I'd rather turn the options around so that the atomic_t or fast > arch implementations are the defaul. But either way it needs to > be configurable. Once that is done we can spread refcount_t everywhere > and everyone will be better off, if only for the documentation value > of the type when they use the atomic_t based implementation. Agreed on the opposite default as opting into common library implementations is how we typically handle things in linux. Kees I I have a concern: __must_check bool refcount_add_not_zero(unsigned int i, refcount_t *r) { unsigned int new, val = atomic_read(&r->refs); do { if (!val) return false; if (unlikely(val == UINT_MAX)) return true; new = val + i; if (new < val) new = UINT_MAX; } while (!atomic_try_cmpxchg_relaxed(&r->refs, &val, new)); WARN_ONCE(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n"); return true; } Why in the world do you succeed when you the value saturates???? >>From a code perspective that is bizarre. The code already has to handle the case when the counter does not increment. So since we already have to have that code to handle the failure to increment I think it would make much more sense if the counters did not only saturate but report failure when the counter can not increment. Right now I am inclined to NACK refcount_t conversions because their semantics don't make sense. Which would seem to make the code less correct by introducing bizarre corner cases instead of letting the code use the it's existing handling of the failure to increment or decrement the counter. Fixing the return value would move refcount_t into the realm of something that is desirable because it has bettern semantics and is more useful just on a day to day correctness point of view. Even ignoring the security implications. I suspect that would also make it easier for refcount_t implementations to produce efficient code. Eric