From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751963AbdIALFk convert rfc822-to-8bit (ORCPT ); Fri, 1 Sep 2017 07:05:40 -0400 Received: from mga09.intel.com ([134.134.136.24]:16574 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751743AbdIALFj (ORCPT ); Fri, 1 Sep 2017 07:05:39 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,457,1498546800"; d="scan'208";a="146434093" 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: AQHTIYrzZIHADGwHSEu0EVkr0E9TAqKflyIAgAAhQgCAAAFJAIAAI4yQgAADB+A= Date: Fri, 1 Sep 2017 11:05:33 +0000 Message-ID: <2236FBA76BA1254E88B949DDB74E612B6FF6347F@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> 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. Actually on the second thought: does the above memory ordering differences really apply when we have ARCH_HAS_REFCOUNT? To me it looks like the way how it is currently implemented for x86 is the same way as it is for atomic cases. > > Best Regards, > Elena. > > > > > Thanks, > > > > tglx