From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753867AbdGJLeK (ORCPT ); Mon, 10 Jul 2017 07:34:10 -0400 Received: from out03.mta.xmission.com ([166.70.13.233]:37668 "EHLO out03.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753385AbdGJLeJ (ORCPT ); Mon, 10 Jul 2017 07:34:09 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: "Reshetova\, Elena" Cc: "linux-kernel\@vger.kernel.org" , "peterz\@infradead.org" , "gregkh\@linuxfoundation.org" , "akpm\@linux-foundation.org" , "mingo\@redhat.com" , "adobriyan\@gmail.com" , "serge\@hallyn.com" , "arozansk\@redhat.com" , "dave\@stgolabs.net" , "keescook\@chromium.org" , Hans Liljestrand , "David Windsor" References: <1499417992-3238-1-git-send-email-elena.reshetova@intel.com> <1499417992-3238-2-git-send-email-elena.reshetova@intel.com> <87bmottgo4.fsf@xmission.com> <2236FBA76BA1254E88B949DDB74E612B6FF2269B@IRSMSX102.ger.corp.intel.com> <87k23gsn4u.fsf@xmission.com> <2236FBA76BA1254E88B949DDB74E612B6FF22730@IRSMSX102.ger.corp.intel.com> Date: Mon, 10 Jul 2017 06:26:06 -0500 In-Reply-To: <2236FBA76BA1254E88B949DDB74E612B6FF22730@IRSMSX102.ger.corp.intel.com> (Elena Reshetova's message of "Mon, 10 Jul 2017 09:56:26 +0000") Message-ID: <878tjwpm7l.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=1dUWwz-00046v-5I;;;mid=<878tjwpm7l.fsf@xmission.com>;;;hst=in02.mta.xmission.com;;;ip=67.3.213.87;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX19UfnvjZxBNNaK9fm41AjvnvPj6JjYcjFo= X-SA-Exim-Connect-IP: 67.3.213.87 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.7 XMSubLong Long Subject * 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.4999] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa08 1397; Body=1 Fuz1=1 Fuz2=1] * 0.0 T_TooManySym_01 4+ unique symbols in subject * 0.0 T_TooManySym_02 5+ unique symbols in subject X-Spam-DCC: XMission; sa08 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: **;"Reshetova\, Elena" X-Spam-Relay-Country: X-Spam-Timing: total 5297 ms - load_scoreonly_sql: 0.02 (0.0%), signal_user_changed: 3.0 (0.1%), b_tie_ro: 2.1 (0.0%), parse: 0.62 (0.0%), extract_message_metadata: 9 (0.2%), get_uri_detail_list: 1.53 (0.0%), tests_pri_-1000: 4.3 (0.1%), tests_pri_-950: 0.90 (0.0%), tests_pri_-900: 0.74 (0.0%), tests_pri_-400: 22 (0.4%), check_bayes: 21 (0.4%), b_tokenize: 6 (0.1%), b_tok_get_all: 8 (0.2%), b_comp_prob: 2.0 (0.0%), b_tok_touch_all: 2.7 (0.1%), b_finish: 0.56 (0.0%), tests_pri_0: 236 (4.4%), check_dkim_signature: 0.40 (0.0%), check_dkim_adsp: 3.1 (0.1%), tests_pri_500: 5019 (94.7%), poll_dns_idle: 5014 (94.7%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 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 in02.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org "Reshetova, Elena" writes: >> "Reshetova, Elena" writes: >> >> 2>> Elena Reshetova writes: >> >> >> >> > refcount_t type and corresponding API should be >> >> > used instead of atomic_t when the variable is used as >> >> > a reference counter. This allows to avoid accidental >> >> > refcounter overflows that might lead to use-after-free >> >> > situations. >> >> >> >> In this patch you can see all of the uses of the count. >> >> What accidental refcount overflows are possible? >> > >> > Even if one can guarantee and prove that in the current implementation >> > there are no overflows possible, we can't say that for >> > sure for any future implementation. Bugs might always happen >> > unfortunately, but if we convert the refcounter to a safer >> > type we can be sure that overflows are not possible. >> > >> > Does it make sense to you? >> >> Not for code that is likely to remain unchanged for a decade no. > > Can we really be sure for any kernel code about this? And does it make > sense to trust our security on a fact like this? But refcount_t doesn't fix anything. At best it changes a bad bug to a less bad bug. So now my machine OOMS instead of allows a memory overwrite. It still doesn't work. Plus refcount_t does not provide any safety on the architectures where it is a noop. >> This looks like a large set of unautomated changes without any real >> thought put into it. > > We are soon into the end of the first year that we started to look into > refcounter overflow/underflow problem and coming up this far was > not easy enough (just check all the millions of emails on kernel-hardening > mailing list). Each refcount_t conversion candidate was first found by Coccinelle > analysis and then manually checked and converted. The story of > refcount_t API and all discussions go even further. > So you can't really claim that there is no " thought put into it " :) But the conversion of the instance happens without thought and manually. Which is a good recipe for typos. Which is what I am saying. There have been lots of conversions like that in the kernel and practically every one has introduced at least one typo. So from an engineering standpoint it is a very valid question to ask about. And I find the apparent insistence that you don't make typos very disturbing. > That almost always results in a typo somewhere >> that breaks things. >> >> So there is no benefit to the code, and a non-zero chance that there >> will be a typo breaking the code. > > The code is very active on issuing WARNs when anything goes wrong. > Using this feature we have not only found errors in conversions, but > sometimes errors in code itself. So, any bug would be actually much > faster visible than using old atomic_t interface. > > In addition by default refcount_t equals to atomic, which also gives a > possibility to make a softer transition and catch all related bugs in couple > of cycles when enabling CONFIG_REFCOUNT_FULL. But if you make a typo and change one operation for another I don't see how any of that applies. And that is what it looks like I we are looking at here. Eric