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]

Message-ID: <20161117102959.GH3142@twins.programming.kicks-ass.net>
Date:   Thu, 17 Nov 2016 11:29:59 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Boqun Feng <boqun.feng@...il.com>
Cc:     gregkh@...uxfoundation.org, keescook@...omium.org,
        will.deacon@....com, elena.reshetova@...el.com, arnd@...db.de,
        tglx@...utronix.de, mingo@...nel.org, hpa@...or.com,
        dave@...gbits.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Thu, Nov 17, 2016 at 05:48:51PM +0800, Boqun Feng wrote:
> > > But as I said, we actually only need the pairing of orderings:
> > > 
> > > 1) load part of cmpxchg -> free() 
> > > 2) object accesses -> store part of cmpxchg
> > > 
> > > Ordering #1 can be achieved via control dependency as you pointed out
> > > that free()s very much includes stores. And ordering #2 can be achieved
> > > with RELEASE.
> > > 
> > > So the code is right, I just thought the comment may be misleading. The
> > > reason we use cmpxchg_release() is just for achieving ordering #2, and
> > > not to order "prior loads and stores" with "a subsequent free".
> > > 
> > > Am I missing some subtle orderings here?
> > 
> > I would want to further quality 1), it must be no earlier than the load
> > of the last / successful ll/sc round.
> > 
> 
> Great, that's more accurate!
> 
> > At that point we're guaranteed a reference count of 1 that _will_ drop
> > to 0, and thus nobody else (should) reference that memory anymore.
> > 
> > If we agree on this, I'll update the comment :-) Will, do you too agree?
> 
> Agreed ;-)
> 
> Control dependencies and RELEASE are totally enough for the internal
> correctness of refcount_t along with its interactivity with free().
> People better not reply order guarantees other than this ;-)

Hurm.. let me ruin my own argument.

Since the free() stores could leak upwards until that ll, and object
stores can be delayed until the sc, we still have a problem. Just not
with the thread that free()s or any other thread that knew about the
object.

The problem comes from any other thread doing an allocation, since its
possible to observe the memory as freed while there are stores pending
to it, we can have those delayed stores trample on our freshly allocated
and initialized object.

The stores must really not be before the SC, so I fear we must either
add an smp_wmb() after the release, or punt and use the fully ordered
cmpxchg().