From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751285AbdBWOFr convert rfc822-to-8bit (ORCPT ); Thu, 23 Feb 2017 09:05:47 -0500 Received: from mga04.intel.com ([192.55.52.120]:62218 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751070AbdBWOFn (ORCPT ); Thu, 23 Feb 2017 09:05:43 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,198,1484035200"; d="scan'208";a="68193315" From: "Reshetova, Elena" To: David Howells CC: "linux-kernel@vger.kernel.org" , "linux-afs@lists.infradead.org" , "peterz@infradead.org" , "gregkh@linuxfoundation.org" , Hans Liljestrand , "Kees Cook" , David Windsor Subject: RE: [PATCH 3/4] fs, afs: convert afs_server.usage from atomic_t to refcount_t Thread-Topic: [PATCH 3/4] fs, afs: convert afs_server.usage from atomic_t to refcount_t Thread-Index: AQHSjFlOfNbOt5meQUKAYX5GtvffIaF1ObgAgAFjCpA= Date: Thu, 23 Feb 2017 14:05:39 +0000 Message-ID: <2236FBA76BA1254E88B949DDB74E612B41C4F570@IRSMSX102.ger.corp.intel.com> References: <1487691808-11289-4-git-send-email-elena.reshetova@intel.com> <1487691808-11289-1-git-send-email-elena.reshetova@intel.com> <483.1487781128@warthog.procyon.org.uk> In-Reply-To: <483.1487781128@warthog.procyon.org.uk> 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="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 > -----Original Message----- > From: David Howells [mailto:dhowells@redhat.com] > Sent: Wednesday, February 22, 2017 6:32 PM > To: Reshetova, Elena > Cc: dhowells@redhat.com; linux-kernel@vger.kernel.org; linux- > afs@lists.infradead.org; peterz@infradead.org; gregkh@linuxfoundation.org; > Hans Liljestrand ; Kees Cook > ; David Windsor > Subject: Re: [PATCH 3/4] fs, afs: convert afs_server.usage from atomic_t to > refcount_t > > Elena Reshetova wrote: > > > 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. > > Although I don't see an assertion for this (the window is too small), it is > possible for a dead server record to get resurrected. Take a look at > afs_put_server() and note there's a check around the move to the graveyard. This one is harder to transform correctly using +1 scheme. As far as I understand, I would need to take an additional lock in afs_put_server() to accommodate this section: @@ -230,12 +230,17 @@ void afs_put_server(struct afs_server *server) _debug("PUT SERVER %d", refcount_read(&server->usage)); - ASSERTCMP(refcount_read(&server->usage), >, 0); + ASSERTCMP(refcount_read(&server->usage), >, 1); - if (likely(!refcount_dec_and_test(&server->usage))) { + /* need a write lock for &server the following section */ + refcount_dec(&server->usage); + + if (likely(refcount_read(&server->usage) > 1)) { + spin_unlock(&old_server->fs_lock); _leave(""); return; } + /* end of lock section */ And smth tells me that you don't want to have any locks here... Right? Same story is with afs_vlocation_lookup(). Best Regards, Elena. > > So, please hold this patch also. > > David