From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751065AbdCQOb3 (ORCPT ); Fri, 17 Mar 2017 10:31:29 -0400 Received: from us-smtp-delivery-194.mimecast.com ([63.128.21.194]:52424 "EHLO us-smtp-delivery-194.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751023AbdCQObY (ORCPT ); Fri, 17 Mar 2017 10:31:24 -0400 From: Trond Myklebust To: "elena.reshetova@intel.com" , "netdev@vger.kernel.org" , "jlayton@poochiereds.net" CC: "herbert@gondor.apana.org.au" , "linux-kernel@vger.kernel.org" , "peterz@infradead.org" , "ralf@linux-mips.org" , "linux-rdma@vger.kernel.org" , "ishkamiel@gmail.com" , "bfields@fieldses.org" , "steffen.klassert@secunet.com" , "nhorman@tuxdriver.com" , "linux-nfs@vger.kernel.org" , "jreuter@yaina.de" , "keescook@chromium.org" , "linux-hams@vger.kernel.org" , "dwindsor@gmail.com" , "zyan@redhat.com" , "sage@redhat.com" , "davem@davemloft.net" , "linux-sctp@vger.kernel.org" , "vyasevich@gmail.com" , "linux-x25@vger.kernel.org" , "santosh.shilimkar@oracle.com" , "ceph-devel@vger.kernel.org" Subject: Re: [PATCH 01/23] net, sunrpc: convert rpc_cred.cr_count from atomic_t to refcount_t Thread-Topic: [PATCH 01/23] net, sunrpc: convert rpc_cred.cr_count from atomic_t to refcount_t Thread-Index: AQHSnxgB6OeN+VGtcU2oa3t5Q+2fbaGY+9eAgAADYACAABgbgA== Date: Fri, 17 Mar 2017 14:28:37 +0000 Message-ID: <1489760913.8441.1.camel@primarydata.com> References: <1489752646-8749-1-git-send-email-elena.reshetova@intel.com> <1489752646-8749-2-git-send-email-elena.reshetova@intel.com> <1489755011.6453.1.camel@primarydata.com> <1489755736.2810.10.camel@poochiereds.net> In-Reply-To: <1489755736.2810.10.camel@poochiereds.net> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-messagesentrepresentingtype: 1 x-originating-ip: [68.49.162.121] x-ms-office365-filtering-correlation-id: 811b7e3b-4ded-43b6-baf7-08d46d41e6ae x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(2017030254049);SRVR:BN6PR11MB1347; x-microsoft-exchange-diagnostics: 1;BN6PR11MB1347;7:bvBofH2ofNnG6M0fxzSseMjgYY27smUAWv5rXU+bZAHoxPdKLgxZraKGscquADv3m1k6u/Gopl6qyE2CEdRmWC6fZZIKADyEpWxXL6wGtmG2v6CPleUbViZA8OcYnbU3Ziqj0gSNoG3LwzkJvhCv5tgDouOVqhln2WmB7k6P1l55kOX+8i9C7M0WLwyB82PLR37v84IE4PSCNbH/cuv18ywF3LjyrRAOzep5iYB7oeaQrH/1YsILA7aRU2Phb3BS8AX5j/7/uRLEZ+ydX1/DVSkmjMLzBTR1AsC05vJvfndkWvxi7hFklofXwEA9UZYmEv7niFg/I7jpTK6iE6v39A==;20:NAWiMnSLPZwIv5qOUiFCHODE2yNy5Dw3dkho3AoBIVbCIH+GU5KmgW4QFepa6yRDveN5YPoAxuTfE6dZlj37V/uttadaaASg2Z5nXjf22uXYxaywh1dUOSfyLRzHPI4Hpr2zTtWuUjWU/D9pbJjxR1MUlKwWfb17vXkwQZlw9RE= x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(228905959029699); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(6040375)(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001)(6041248)(2016111802025)(20161123560025)(20161123558025)(20161123564025)(20161123562025)(20161123555025)(6043046)(6072148);SRVR:BN6PR11MB1347;BCL:0;PCL:0;RULEID:;SRVR:BN6PR11MB1347; x-forefront-prvs: 0249EFCB0B x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(6009001)(39450400003)(39840400002)(39410400002)(377424004)(24454002)(36756003)(99286003)(4326008)(6506006)(2906002)(3660700001)(50986999)(25786008)(86362001)(54906002)(66066001)(575784001)(2950100002)(102836003)(229853002)(3280700002)(2201001)(6512007)(103116003)(38730400002)(8676002)(76176999)(39060400002)(5660300001)(7416002)(6486002)(189998001)(2501003)(93886004)(6246003)(77096006)(6116002)(122556002)(3846002)(305945005)(7736002)(54356999)(8936002)(81166006)(53936002)(2900100001)(6436002)(33646002);DIR:OUT;SFP:1102;SCL:1;SRVR:BN6PR11MB1347;H:BN6PR11MB1348.namprd11.prod.outlook.com;FPR:;SPF:None;MLV:ovrnspm;PTR:InfoNoRecords;LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-ID: MIME-Version: 1.0 X-OriginatorOrg: primarydata.com X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Mar 2017 14:28:37.3022 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 03193ed6-8726-4bb3-a832-18ab0d28adb7 X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR11MB1347 X-MC-Unique: ne4W2dObNUKMg3mzOdw6xA-1 Content-Type: text/plain; charset=UTF-8 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 v2HEVgVG017970 On Fri, 2017-03-17 at 09:02 -0400, Jeff Layton wrote: > On Fri, 2017-03-17 at 12:50 +0000, Trond Myklebust wrote: > > On Fri, 2017-03-17 at 14:10 +0200, 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. > > > > > > Signed-off-by: Elena Reshetova > > > Signed-off-by: Hans Liljestrand > > > Signed-off-by: Kees Cook > > > Signed-off-by: David Windsor > > > --- > > >  include/linux/sunrpc/auth.h |  8 ++++---- > > >  net/sunrpc/auth.c           | 12 ++++++------ > > >  2 files changed, 10 insertions(+), 10 deletions(-) > > > > > > diff --git a/include/linux/sunrpc/auth.h > > > b/include/linux/sunrpc/auth.h > > > index b1bc62b..bd36e0b 100644 > > > --- a/include/linux/sunrpc/auth.h > > > +++ b/include/linux/sunrpc/auth.h > > > @@ -15,7 +15,7 @@ > > >  #include > > >  #include > > >   > > > -#include > > > +#include > > >  #include > > >  #include > > >  #include > > > @@ -68,7 +68,7 @@ struct rpc_cred { > > >  #endif > > >   unsigned long cr_expire; /* when > > > to gc > > > */ > > >   unsigned long cr_flags; /* various > > > flags */ > > > - atomic_t cr_count; /* ref count */ > > > + refcount_t cr_count; /* ref count > > > */ > > > > > > > NACK. That's going to be hitting > > WARN_ONCE(!refcount_inc_not_zero(r), > > "refcount_t: increment on 0; use-after-free.\n") like there's no > > tomorrow... > > > > Please stop with these automated conversions. They are going to > > cause a > > lot more bugs than they fix. > > > > Agreed. These patchsets are touching places where we've already > banged > out most of the refcounting bugs. I'm against doing large scale > conversions like this without a damned good reason. > > I think it may be best to do this sort of thing in a more piecemeal > fashion. Pick a subsystem or two and do the conversions there to > prove > that they're better than what we have. If the subsystem already has > problems with its refcounting, then so much the better. Point to bugs > that this new infrastructure helped find. > > Encourage people to adopt your new infrastructure as new refcounted > objects are introduced into the kernel. You might even consider a LWN > article about this. > > Eventually we'll get around to changing existing code to use it, once > there is a sufficient advantage to doing so. Most likely when we're > reworking the code for other reasons, or when we're chasing some > horrid > refcounting bug and think that this might help find it. The main issue is that this "refcount_t" implementation appears to be assuming that there is one and only one model for refcounts (the one where a value of "0" means "free me immediately"). The kernel has a plethora of object caching implementations where this is simply not the case; the dcache is a prime example, and this cache is another. In both these implementation, the atomic_t variable is being used more as a semaphore-style lock that prevents freeing of the object while it is in active use as opposed to being freeable, but cached. This is why these automated conversions are a nuisance and a source of bugs. -- Trond Myklebust Linux NFS client maintainer, PrimaryData trond.myklebust@primarydata.com