linux-kernel - Re: [PATCH -v2] blk-mq: Start to fix memory ordering...

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:   Wed, 6 Sep 2017 22:19:57 +0800
From:   Boqun Feng <boqun.feng@...il.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Peter Zijlstra <peterz@...radead.org>, Jens Axboe <axboe@...com>,
        parri.andrea@...il.com, linux-kernel@...r.kernel.org,
        tom.leiming@...il.com, hch@....de, paulmck@...ux.vnet.ibm.com,
        will.deacon@....com
Subject: Re: [PATCH -v2] blk-mq: Start to fix memory ordering...

On Wed, Sep 06, 2017 at 10:02:00AM -0400, Alan Stern wrote:
> On Wed, 6 Sep 2017, Peter Zijlstra wrote:
> 
> > Attempt to untangle the ordering in blk-mq. The patch introducing the
> > single smp_mb__before_atomic() is obviously broken in that it doesn't
> > clearly specify a pairing barrier and an obtained guarantee.
> > 
> > The comment is further misleading in that it hints that the
> > deadline store and the COMPLETE store also need to be ordered, but
> > AFAICT there is no such dependency. However what does appear to be
> > important is the clear happening _after_ the store, and that worked by
> > pure accident.
> > 
> > This clarifies blk_mq_start_request() -- we should not get there with
> > STARTING set -- this simplifies the code and makes the barrier usage
> > sane (the old code could be read to allow not having _any_ atomic after
> > the barrier, in which case the barrier hasn't got anything to order). We
> > then also introduce the missing pairing barrier for it.
> > 
> > Also down-grade the barrier to smp_wmb(), this is cheaper for
> > PowerPC/ARM and doesn't cost anything extra on x86.
> > 
> > And it documents the STARTING vs COMPLETE ordering. Although I've not
> > been entirely successful in reverse engineering the blk-mq state
> > machine so there might still be more funnies around timeout vs
> > requeue.
> > 
> > If I got anything wrong, feel free to educate me by adding comments to
> > clarify things ;-)
> > 
> > Cc: Alan Stern <stern@...land.harvard.edu>
> > Cc: Will Deacon <will.deacon@....com>
> > Cc: Ming Lei <tom.leiming@...il.com>
> > Cc: Christoph Hellwig <hch@....de>
> > Cc: Jens Axboe <axboe@...com>
> > Cc: Andrea Parri <parri.andrea@...il.com>
> > Cc: Boqun Feng <boqun.feng@...il.com>
> > Cc: Bart Van Assche <bart.vanassche@....com>
> > Cc: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
> > Fixes: 538b75341835 ("blk-mq: request deadline must be visible before marking rq as started")
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> > ---
> >  - spelling; Andrea and Bart
> >  - compiles (urgh!)
> >  - smp_wmb(); Adrea
> > 
> > 
> >  block/blk-mq.c      | 52 ++++++++++++++++++++++++++++++++++++++++------------
> >  block/blk-timeout.c |  2 +-
> >  2 files changed, 41 insertions(+), 13 deletions(-)
> > 
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index 4603b115e234..506a0f355117 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -558,22 +558,32 @@ void blk_mq_start_request(struct request *rq)
> >  
> >  	blk_add_timer(rq);
> >  
> > -	/*
> > -	 * Ensure that ->deadline is visible before set the started
> > -	 * flag and clear the completed flag.
> > -	 */
> > -	smp_mb__before_atomic();
> > +	WARN_ON_ONCE(test_bit(REQ_ATOM_STARTED, &rq->atomic_flags));
> >  
> >  	/*
> >  	 * Mark us as started and clear complete. Complete might have been
> >  	 * set if requeue raced with timeout, which then marked it as
> >  	 * complete. So be sure to clear complete again when we start
> >  	 * the request, otherwise we'll ignore the completion event.
> > +	 *
> > +	 * Ensure that ->deadline is visible before we set STARTED, such that
> > +	 * blk_mq_check_expired() is guaranteed to observe our ->deadline when
> > +	 * it observes STARTED.
> >  	 */
> > -	if (!test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
> > -		set_bit(REQ_ATOM_STARTED, &rq->atomic_flags);
> > -	if (test_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags))
> > +	smp_wmb();
> > +	set_bit(REQ_ATOM_STARTED, &rq->atomic_flags);
> > +	if (test_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags)) {
> > +		/*
> > +		 * Coherence order guarantees these consecutive stores to a
> > +		 * single variable propagate in the specified order. Thus the
> > +		 * clear_bit() is ordered _after_ the set bit. See
> > +		 * blk_mq_check_expired().
> > +		 *
> > +		 * (the bits must be part of the same byte for this to be
> > +		 * true).
> 
> Adding this comment is well and good, but for more security you should 
> also add a comment (maybe even a compile-time check) to the place where 
> REQ_ATOM_STARTED and REQ_ATOM_COMPLETE are defined.  Otherwise they 
> might eventually get moved into separate bytes.
> 

How about adding:

	BUILD_BUG_ON((REQ_ATOM_STARTED / BITS_PER_BYTE) != (REQ_ATOM_COMPLETE / BITS_PER_BYTE));

here?

Regards,
Boqun

> Alan Stern
> 

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)