From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752282AbdCPSAT (ORCPT ); Thu, 16 Mar 2017 14:00:19 -0400 Received: from mga04.intel.com ([192.55.52.120]:31644 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751838AbdCPSAQ (ORCPT ); Thu, 16 Mar 2017 14:00:16 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,173,1486454400"; d="scan'208";a="68117321" From: "Reshetova, Elena" To: James Bottomley , Michael Ellerman , "gregkh@linuxfoundation.org" CC: "linux-kernel@vger.kernel.org" , "xen-devel@lists.xenproject.org" , "netdev@vger.kernel.org" , "linux1394-devel@lists.sourceforge.net" , "linux-bcache@vger.kernel.org" , "linux-raid@vger.kernel.org" , "linux-media@vger.kernel.org" , "devel@linuxdriverproject.org" , "linux-pci@vger.kernel.org" , "linux-s390@vger.kernel.org" , "fcoe-devel@open-fcoe.org" , "linux-scsi@vger.kernel.org" , "open-iscsi@googlegroups.com" , "devel@driverdev.osuosl.org" , "target-devel@vger.kernel.org" , "linux-serial@vger.kernel.org" , "linux-usb@vger.kernel.org" , "peterz@infradead.org" , Hans Liljestrand , Kees Cook , David Windsor , "linuxppc-dev@lists.ozlabs.org" Subject: RE: [PATCH 08/29] drivers, md: convert mddev.active from atomic_t to refcount_t Thread-Topic: [PATCH 08/29] drivers, md: convert mddev.active from atomic_t to refcount_t Thread-Index: AQHSloUPCUjqQQHTokeJQd6UNRIBGqGUS0uAgAADd8CAACs5gIAC+lzw Date: Thu, 16 Mar 2017 18:00:07 +0000 Message-ID: <2236FBA76BA1254E88B949DDB74E612B41C59FF8@IRSMSX102.ger.corp.intel.com> References: <1488810076-3754-1-git-send-email-elena.reshetova@intel.com> <1488810076-3754-9-git-send-email-elena.reshetova@intel.com> <87lgs8ukfq.fsf@concordia.ellerman.id.au> <2236FBA76BA1254E88B949DDB74E612B41C588E8@IRSMSX102.ger.corp.intel.com> <1489503539.3214.17.camel@HansenPartnership.com> In-Reply-To: <1489503539.3214.17.camel@HansenPartnership.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v2GI0Wjk022030 > On Tue, 2017-03-14 at 12:29 +0000, Reshetova, Elena wrote: > > > 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. > > > > > > > > Signed-off-by: Elena Reshetova > > > > Signed-off-by: Hans Liljestrand > > > > Signed-off-by: Kees Cook > > > > Signed-off-by: David Windsor > > > > --- > > > > drivers/md/md.c | 6 +++--- > > > > drivers/md/md.h | 3 ++- > > > > 2 files changed, 5 insertions(+), 4 deletions(-) > > > > > > When booting linux-next (specifically 5be4921c9958ec) I'm seeing > > > the > > > backtrace below. I suspect this patch is just exposing an existing > > > issue? > > > > Yes, we have actually been following this issue in the another > > thread. > > It looks like the object is re-used somehow, but I can't quite > > understand how just by reading the code. > > This was what I put into the previous thread: > > > > "The log below indicates that you are using your refcounter in a bit > > weird way in mddev_find(). > > However, I can't find the place (just by reading the code) where you > > would increment refcounter from zero (vs. setting it to one). > > It looks like you either iterate over existing nodes (and increment > > their counters, which should be >= 1 at the time of increment) or > > create a new node, but then mddev_init() sets the counter to 1. " > > > > If you can help to understand what is going on with the object > > creation/destruction, would be appreciated! > > > > Also Shaohua Li stopped this patch coming from his tree since the > > issue was caught at that time, so we are not going to merge this > > until we figure it out. > > Asking on the correct list (dm-devel) would have got you the easy > answer: The refcount behind mddev->active is a genuine atomic. It has > refcount properties but only if the array fails to initialise (in that > case, final put kills it). Once it's added to the system as a gendisk, > it cannot be freed until md_free(). Thus its ->active count can go to > zero (when it becomes inactive; usually because of an unmount). On a > simple allocation regardless of outcome, the last executed statement in > md_alloc is mddev_put(): that destroys the device if we didn't manage > to create it or returns 0 and adds an inactive device to the system > which the user can get with mddev_find(). Thank you James for explaining this! I guess in this case, the conversion doesn't make sense. And sorry about not asking in a correct place: we are handling many similar patches now and while I try to reach the right audience using get_maintainer script, it doesn't always succeeds. Best Regards, Elena. > > James >