From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751912AbdIAKxC convert rfc822-to-8bit (ORCPT ); Fri, 1 Sep 2017 06:53:02 -0400 Received: from mga14.intel.com ([192.55.52.115]:4936 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751725AbdIAKxA (ORCPT ); Fri, 1 Sep 2017 06:53:00 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,457,1498546800"; d="scan'208";a="306901321" From: "Reshetova, Elena" To: Thomas Gleixner , Peter Zijlstra CC: "linux-kernel@vger.kernel.org" , "linux-fsdevel@vger.kernel.org" , "gregkh@linuxfoundation.org" , "viro@zeniv.linux.org.uk" , "tj@kernel.org" , "mingo@redhat.com" , "hannes@cmpxchg.org" , "lizefan@huawei.com" , "acme@kernel.org" , "alexander.shishkin@linux.intel.com" , "eparis@redhat.com" , "akpm@linux-foundation.org" , "arnd@arndb.de" , "luto@kernel.org" , "keescook@chromium.org" , "dvhart@infradead.org" , "ebiederm@xmission.com" Subject: RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t Thread-Topic: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t Thread-Index: AQHTIYrzZIHADGwHSEu0EVkr0E9TAqKflyIAgAAhQgCAAAFJAIAAI4yQ Date: Fri, 1 Sep 2017 10:52:55 +0000 Message-ID: <2236FBA76BA1254E88B949DDB74E612B6FF6344E@IRSMSX102.ger.corp.intel.com> References: <1504095773-22895-1-git-send-email-elena.reshetova@intel.com> <1504095773-22895-15-git-send-email-elena.reshetova@intel.com> <20170901093852.it4d4bxoy2lmojrk@hirez.programming.kicks-ass.net> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [163.33.239.180] 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 > On Fri, 1 Sep 2017, Peter Zijlstra wrote: > > On Fri, Sep 01, 2017 at 09:39:50AM +0200, Thomas Gleixner wrote: > > > On Wed, 30 Aug 2017, Elena Reshetova wrote: > > > > atomic_t variables are currently used to implement reference > > > > counters with the following properties: > > > > - counter is initialized to 1 using atomic_set() > > > > - a resource is freed upon counter reaching zero > > > > - once counter reaches zero, its further > > > > increments aren't allowed > > > > - counter schema uses basic atomic operations > > > > (set, inc, inc_not_zero, dec_and_test, etc.) > > > > > > > > Such atomic variables should be converted to a newly provided > > > > refcount_t type and API that prevents accidental counter overflows > > > > and underflows. This is important since overflows and underflows > > > > can lead to use-after-free situation and be exploitable. > > > > > > > > The variable futex_pi_state.refcount is used as pure > > > > reference counter. Convert it to refcount_t and fix up > > > > the operations. > > > > > > > > Suggested-by: Kees Cook > > > > Reviewed-by: David Windsor > > > > Reviewed-by: Hans Liljestrand > > > > Signed-off-by: Elena Reshetova > > > > > > Reviewed-by: Thomas Gleixner > > > > So the thing to be careful with for things like futex and some of the > > other core kernel code is the memory ordering. > > > > atomic_dec_and_test() provides a full smp_mb() before and after, > > refcount_dec_and_test() only provides release semantics. > > > > This is typically sufficient, and I would argue that if we rely on more > > than that, there _should_ be a comment, however reality isn't always as > > nice. > > > > That said, I think this conversion is OK, pi_state->refcount isn't > > relied upon to provide additional memory ordering above and beyond what > > refcounting requires. > > So the changelogs should reflect that. The current one suggests that this > is a one to one replacement for atomic_t just with the extra sanity checks > added. I will update the commit texts accordingly and resend the whole series since this should be then mentioned in every commit to make sure it is not missed. Best Regards, Elena. > > Thanks, > > tglx