linux-kernel - Re: [RFC][PATCH 7/7] kref: Implement using refcount

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 18 Nov 2016 19:57:19 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Will Deacon <will.deacon@....com>
Cc:     "Reshetova, Elena" <elena.reshetova@...el.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "arnd@...db.de" <arnd@...db.de>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...nel.org" <mingo@...nel.org>,
        "hpa@...or.com" <hpa@...or.com>,
        "dave@...gbits.org" <dave@...gbits.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        boqun.feng@...il.com
Subject: Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

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:
> > +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);
> > +}
> 
> 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).

Duh, I actually spotted that once and still didn't fix that :/

> > +static inline __must_check
> > +bool refcount_inc_not_zero(refcount_t *r)
> > +{
> > +	unsigned int old, new, val = atomic_read(&r->refs);
> > +
> > +	for (;;) {
> > +		if (!val)
> > +			return false;
> > +
> > +		if (unlikely(val == UINT_MAX))
> > +			return true;
> > +
> > +		new = val + 1;
> > +		old = atomic_cmpxchg_relaxed(&r->refs, val, new);
> > +		if (old == val)
> > +			break;
> > +
> > +		val = old;
> 
> 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 really
> 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?

Dunno, it _should_ not, but then again, who knows.

I can certainly write it as WARN_ON(!refcount_inc_not_zero());

> > +static inline __must_check
> > +bool refcount_dec_and_test(refcount_t *r)
> > +{
> > +	unsigned int old, new, val = atomic_read(&r->refs);
> > +
> > +	for (;;) {
> > +		if (val == UINT_MAX)
> > +			return false;
> > +
> > +		new = val - 1;
> > +		if (WARN(new > val, "refcount_t: underflow; use-after-free.\n"))
> > +			return false;
> 
> Wouldn't it be clearer to compare val with 0 before doing the decrement?

Maybe, this way you can change the 1 and it'll keep working. Then again,
you can't do that with the inc side, so who cares.