linux-kernel - Re: [V9fs-developer] [PATCH 4/6] 9p: Remove an unnecessary memory barrier

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: <20180628140358.GG7646@bombadil.infradead.org>
Date:   Thu, 28 Jun 2018 07:03:58 -0700
From:   Matthew Wilcox <willy@...radead.org>
To:     Dominique Martinet <asmadeus@...ewreck.org>
Cc:     v9fs-developer@...ts.sourceforge.net,
        Latchesar Ionkov <lucho@...kov.net>,
        Eric Van Hensbergen <ericvh@...il.com>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        Ron Minnich <rminnich@...dia.gov>
Subject: Re: [V9fs-developer] [PATCH 4/6] 9p: Remove an unnecessary memory
 barrier

On Thu, Jun 28, 2018 at 03:40:29PM +0200, Dominique Martinet wrote:
> Matthew Wilcox wrote on Thu, Jun 28, 2018:
> > --- a/net/9p/client.c
> > +++ b/net/9p/client.c
> > @@ -436,13 +436,9 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status)
> >  {
> >  	p9_debug(P9_DEBUG_MUX, " tag %d\n", req->tc->tag);
> >  
> > -	/*
> > -	 * This barrier is needed to make sure any change made to req before
> > -	 * the other thread wakes up will indeed be seen by the waiting side.
> > -	 */
> > -	smp_wmb();
> >  	req->status = status;
> >  
> > +	/* wake_up is an implicit write memory barrier */
> 
> Nope.
> Please note the wmb is _before_ setting status, basically it protects
> from cpu optimizations where status could be set before other fields,
> then other core opportunistically checking and finding status is good so
> other thread continuing.
> 
> I could only reproduce this bug with infiniband network, but it is very
> definitely needed. Here is the commit message of when I added that barrier:
> -----
> 9P: Add memory barriers to protect request fields over cb/rpc threads handoff
> 
> We need barriers to guarantee this pattern works as intended:
> [w] req->rc, 1          [r] req->status, 1
> wmb                     rmb
> [w] req->status, 1      [r] req->rc
> 
> Where the wmb ensures that rc gets written before status,
> and the rmb ensures that if you observe status == 1, rc is the new value.
> -----
> 
> It might need an update to the comment though, if you thought about
> removing it...

Ah!  Yes, that situation is different from what the comment documents.

How about this?

	/*
	 * This barrier is needed to make sure any change made to req before
-	 * the other thread wakes up will indeed be seen by the waiting side.
+	 * the status change is visible to another thread
	 */