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.
