Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753291AbdGJJ4c convert rfc822-to-8bit (ORCPT ); Mon, 10 Jul 2017 05:56:32 -0400 Received: from mga03.intel.com ([134.134.136.65]:64799 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752457AbdGJJ4b (ORCPT ); Mon, 10 Jul 2017 05:56:31 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,339,1496127600"; d="scan'208";a="1149939058" From: "Reshetova, Elena" To: "Eric W. Biederman" 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" Subject: RE: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t Thread-Topic: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t Thread-Index: AQHS+VfSSQPfUhM6vkqQ9ug58kizeqJM0FqA Date: Mon, 10 Jul 2017 09:56:26 +0000 Message-ID: <2236FBA76BA1254E88B949DDB74E612B6FF22730@IRSMSX102.ger.corp.intel.com> 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> In-Reply-To: <87k23gsn4u.fsf@xmission.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2540 Lines: 67 > "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? > > 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 " :) 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. Best Regards, Elena. > > All to harden the code for an unlikely future when the code is > updated with a full test cycle and people paying attention. > > Introduce a bug now to avoid a bug in the future. That seems like a > very poor engineering trade off. > > Eric