From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753096AbcKUEGp (ORCPT ); Sun, 20 Nov 2016 23:06:45 -0500 Received: from mail-pg0-f65.google.com ([74.125.83.65]:33046 "EHLO mail-pg0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752819AbcKUEGn (ORCPT ); Sun, 20 Nov 2016 23:06:43 -0500 Date: Mon, 21 Nov 2016 12:06:44 +0800 From: Boqun Feng To: Will Deacon Cc: Peter Zijlstra , "Reshetova, Elena" , "gregkh@linuxfoundation.org" , "keescook@chromium.org" , "arnd@arndb.de" , "tglx@linutronix.de" , "mingo@kernel.org" , "hpa@zytor.com" , "dave@progbits.org" , "linux-kernel@vger.kernel.org" Subject: Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Message-ID: <20161121040644.GE5227@tardis.cn.ibm.com> References: <20161114173946.501528675@infradead.org> <20161114174446.832175072@infradead.org> <2236FBA76BA1254E88B949DDB74E612B41C148C4@IRSMSX102.ger.corp.intel.com> <20161118113718.GP3117@twins.programming.kicks-ass.net> <20161118170655.GZ13470@arm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="HCdXmnRlPgeNBad2" Content-Disposition: inline In-Reply-To: <20161118170655.GZ13470@arm.com> User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --HCdXmnRlPgeNBad2 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Nov 18, 2016 at 05:06:55PM +0000, Will Deacon wrote: > On Fri, Nov 18, 2016 at 12:37:18PM +0100, Peter Zijlstra wrote: > > On Fri, Nov 18, 2016 at 10:07:26AM +0000, Reshetova, Elena wrote: > > >=20 > > > Peter do you have the changes to the refcount_t interface compare to > > > the version in this patch?=20 > >=20 > > > We are now starting working on atomic_t --> refcount_t conversions and > > > it would save a bit of work to have latest version from you that we > > > can be based upon.=20 > >=20 > > The latestest version below, mostly just comment changes since last > > time. > >=20 > > --- > > Subject: refcount_t: A special purpose refcount type > > From: Peter Zijlstra > > Date: Mon Nov 14 18:06:19 CET 2016 > >=20 > > Provide refcount_t, an atomic_t like primitive built just for > > refcounting. > >=20 > > It provides saturation semantics such that overflow becomes impossible > > and thereby 'spurious' use-after-free is avoided. > >=20 > > Signed-off-by: Peter Zijlstra (Intel) > > --- > > include/linux/refcount.h | 241 ++++++++++++++++++++++++++++++++++++++= +++++++++ > > 1 file changed, 241 insertions(+) > >=20 > > --- /dev/null > > +++ b/include/linux/refcount.h > > @@ -0,0 +1,241 @@ > > +#ifndef _LINUX_REFCOUNT_H > > +#define _LINUX_REFCOUNT_H > > + > > +/* > > + * Variant of atomic_t specialized for reference counts. > > + * > > + * The interface matches the atomic_t interface (to aid in porting) bu= t only > > + * provides the few functions one should use for reference counting. > > + * > > + * It differs in that the counter saturates at UINT_MAX and will not m= ove once > > + * there. This avoids wrapping the counter and causing 'spurious' > > + * use-after-free issues. > > + * > > + * Memory ordering rules are slightly relaxed wrt regular atomic_t fun= ctions > > + * and provide only what is strictly required for refcounts. > > + * > > + * The increments are fully relaxed; these will not provide ordering. = The > > + * rationale is that whatever is used to obtain the object we're incre= asing the > > + * reference count on will provide the ordering. For locked data struc= tures, > > + * its the lock acquire, for RCU/lockless data structures its the depe= ndent > > + * load. > > + * > > + * Do note that inc_not_zero() provides a control dependency which wil= l order > > + * future stores against the inc, this ensures we'll never modify the = object > > + * if we did not in fact acquire a reference. > > + * > > + * The decrements will provide release order, such that all the prior = loads and > > + * stores will be issued before, it also provides a control dependency= , which > > + * will order us against the subsequent free(). > > + * > > + * The control dependency is against the load of the cmpxchg (ll/sc) t= hat > > + * succeeded. This means the stores aren't fully ordered, but this is = fine > > + * because the 1->0 transition indicates no concurrency. > > + * > > + * Note that the allocator is responsible for ordering things between = free() > > + * and alloc(). > > + * > > + * > > + * Note: the implementation hard relies on increments, bigger than 1 a= dditions > > + * need explicit overflow -> saturation logic. > > + * > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > + > > +typedef struct refcount_struct { > > + atomic_t refs; > > +} refcount_t; > > + > > +#define REFCOUNT_INIT(n) { .refs =3D ATOMIC_INIT(n), } > > + > > +static inline void refcount_set(refcount_t *r, int n) > > +{ > > + atomic_set(&r->refs, n); > > +} > > + > > +static inline unsigned int refcount_read(const refcount_t *r) > > +{ > > + return atomic_read(&r->refs); > > +} >=20 > Minor nit, but it might be worth being consistent in our usage of int > (parameter to refcount_set) and unsigned int (return value of > refcount_read). >=20 > > + > > +/* > > + * Similar to atomic_inc(), will saturate at UINT_MAX and WARN. > > + * > > + * Provides no memory ordering, it is assumed the caller already has a > > + * reference on the object, will WARN when this is not so. > > + */ > > +static inline void refcount_inc(refcount_t *r) > > +{ > > + unsigned int old, new, val =3D atomic_read(&r->refs); > > + > > + for (;;) { > > + WARN(!val, "refcount_t: increment on 0; use-after-free.\n"); > > + > > + if (unlikely(val =3D=3D UINT_MAX)) > > + return; > > + > > + new =3D val + 1; > > + old =3D atomic_cmpxchg_relaxed(&r->refs, val, new); > > + if (old =3D=3D val) > > + break; > > + > > + val =3D old; > > + } > > + > > + WARN(new =3D=3D UINT_MAX, "refcount_t: saturated; leaking memory.\n"); > > +} > > + > > +/* > > + * Similar to atomic_inc_not_zero(), will saturate at UINT_MAX and WAR= N. > > + * > > + * Provides no memory ordering, it is assumed the caller has guarantee= d the > > + * object memory to be stable (RCU, etc.). It does provide a control d= ependency > > + * and thereby orders future stores. See the comment on top. > > + */ > > +static inline __must_check > > +bool refcount_inc_not_zero(refcount_t *r) > > +{ > > + unsigned int old, new, val =3D atomic_read(&r->refs); > > + > > + for (;;) { > > + if (!val) > > + return false; > > + > > + if (unlikely(val =3D=3D UINT_MAX)) > > + return true; > > + > > + new =3D val + 1; > > + old =3D atomic_cmpxchg_relaxed(&r->refs, val, new); > > + if (old =3D=3D val) > > + break; > > + > > + val =3D old; >=20 > Hmm, it's a shame this code is duplicated from refcount_inc, but I suppose > you can actually be racing against the counter going to zero here and rea= lly > need to check it each time round the loop. Humph. That said, given that > refcount_inc WARNs if the thing is zero, maybe that could just call > refcount_inc_not_zero and warn if it returns false? Does it matter that > we don't actually do the increment? >=20 > > + } > > + > > + WARN(new =3D=3D UINT_MAX, "refcount_t: saturated; leaking memory.\n"); > > + > > + return true; > > +} > > + > > +/* > > + * Similar to atomic_dec_and_test(), it will WARN on underflow and fai= l to > > + * decrement when saturated at UINT_MAX. >=20 > It also fails to decrement in the underflow case (which is fine, but not > obvious from the comment). Same thing below. >=20 Maybe a table in the comment like the following helps? /* * T: return true, F: return fasle * W: trigger WARNING * N: no effect * * | value before ops | * | 0 | 1 | UINT_MAX - 1 | UINT_MAX | * ---------------------+-------+-------+--------------+----------+ * inc() | W | | W | N | * inc_not_zero() | FN | T | WT | WTN | * dec_and_test() | WFN | T | F | FN | * dec_and_mutex_lock() | WFN | T | F | FN | * dec_and_spin_lock() | WFN | T | F | FN | */ Regards, Boqun > > + * > > + * Provides release memory ordering, such that prior loads and stores = are done > > + * before, and provides a control dependency such that free() must com= e after. > > + * See the comment on top. > > + */ > > +static inline __must_check > > +bool refcount_dec_and_test(refcount_t *r) > > +{ > > + unsigned int old, new, val =3D atomic_read(&r->refs); > > + > > + for (;;) { > > + if (val =3D=3D UINT_MAX) > > + return false; > > + > > + new =3D val - 1; > > + if (WARN(new > val, "refcount_t: underflow; use-after-free.\n")) > > + return false; >=20 > Wouldn't it be clearer to compare val with 0 before doing the decrement? >=20 > Will --HCdXmnRlPgeNBad2 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAABCAAGBQJYMnLQAAoJEEl56MO1B/q4Y4oH/jsmRhhWCj5TUhq3Tgpo8PjQ rIet+D1X/TmNwildCoGFyyckgFqOmr0MD+Cjs3T52ni7LpS/ptS78uIpFFyuSzEN 9IdXAYAVwe5bUIYk3XTe5nYO1Vb7fVeaY5itFGmHCxLAy03M075t6FIWLkX+6QFP hZniPVhL5I6lMi1ruG3D69ZiD6uYtJo9LVykYBHllx3Bovxs8yVw0CEu0Gs1kiBm nCes6hnlZ9CHcX1B6JeOtcrxrhF2JT3xdLIeoMOEOsjKvqRXVmFGiR8A6I37sc4b U2lOmVMAy9mrlpt3zVoSiO0Ec9mesir3XeS+Aif5IZAcn0NdASNboLHIQbmxaHs= =DVtp -----END PGP SIGNATURE----- --HCdXmnRlPgeNBad2--