linux-kernel - Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to 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: <20170904120009.ah2qu3lbgdqdgz6i@hirez.programming.kicks-ass.net>
Date:   Mon, 4 Sep 2017 14:00:09 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     "Reshetova, Elena" <elena.reshetova@...el.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
        "tj@...nel.org" <tj@...nel.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "hannes@...xchg.org" <hannes@...xchg.org>,
        "lizefan@...wei.com" <lizefan@...wei.com>,
        "acme@...nel.org" <acme@...nel.org>,
        "alexander.shishkin@...ux.intel.com" 
        <alexander.shishkin@...ux.intel.com>,
        "eparis@...hat.com" <eparis@...hat.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "arnd@...db.de" <arnd@...db.de>,
        "luto@...nel.org" <luto@...nel.org>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "dvhart@...radead.org" <dvhart@...radead.org>,
        "ebiederm@...ssion.com" <ebiederm@...ssion.com>
Subject: Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to
 refcount_t

On Mon, Sep 04, 2017 at 10:31:54AM +0000, Reshetova, Elena wrote:
> > > But can they make "fast" implementation on ARM that would give stronger
> > > memory guarantees?
> > 
> > Whatever for?
> 
> Well, maybe just by default when arch.-specific implementation is
> done. But I was just trying to speculate to understand. I will resend
> this one with new comment added. 

So the generic lib/refcount.c already has weak ordering. It doesn't make
sense for an arch specific implementation (on a weakly ordered machine)
to provide stronger guarantees (it would make things slower).

The weaker ordering of the refcount_t primitives is sufficient if we're
talking pure refcounts. If for some reason code relies on stronger
ordering there _SHOULD_ be a comment with describing the additional
ordering requirements.

But that's a fairly big 'should'. I can well imagine the comment not
being there. In fact, see below.

> Still not sure if I need to resend the whole series with updated
> commits or break this up by individual patches further for the
> separate merges. 

I've yet to look at the ones targeted at subsystems I do, I'm forever
and terminally behind on review :/

I called out the issue on futex in particular because it is fairly
tricky code that.

Now Thomas would like you to mention the fact that refcount_t doesn't
provide the exact same ordering as the atomic_t usages it replaces and
I think it would be good if you could hand-wave an argument on why the
futex code doesn't care.

Now, suppose we were to convert i_count to refcount_t (yes, I know, my
initial conversion wasn't well received), then we need to add
futex_get_inode() similar to futex_get_mm().

That is, smp_mb__{before,after}_atomic() works as expected and can be
used to fortify the implied barriers by refcount_t.

---
Subject: fs,inode: Add comment explaining additional ordering

Add a note to ihold() to document the ordering futex relies upon.

Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
---
 fs/inode.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/inode.c b/fs/inode.c
index 50370599e371..17192ba92fef 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -395,6 +395,10 @@ void __iget(struct inode *inode)
  */
 void ihold(struct inode *inode)
 {
+	/*
+	 * Note: futex.c:get_futex_key_refs() relies on this function
+	 * implying an smp_mb().
+	 */
 	WARN_ON(atomic_inc_return(&inode->i_count) < 2);
 }
 EXPORT_SYMBOL(ihold);