Re: [PATCH v4 05/10] blk-mq: Unregister debugfs attributes earlier

[Omar Sandoval <osandov@osandov.com>]

On Mon, Apr 24, 2017 at 05:12:05PM +0000, Bart Van Assche wrote:
> On Mon, 2017-04-24 at 09:55 -0700, Omar Sandoval wrote:
> > On Fri, Apr 21, 2017 at 04:40:21PM -0700, Bart Van Assche wrote:
> > > One of the debugfs attributes allows to run a queue. Since running
> > > a queue after a queue has entered the "dead" state is not allowed
> > > and even can cause a kernel crash, unregister the debugfs attributes
> > > before a queue reaches the "dead" state.
> > 
> > More important than this case, I think, is that blk_cleanup_queue()
> > calls blk_mq_free_queue(q), so most of the debugfs entries would lead to
> > use-after-frees. If you add that to the commit message and address my
> > comment below,
> > 
> > Reviewed-by: Omar Sandoval <osandov@fb.com>
> 
> Thanks! I will update the commit message.
> 
> > > --- a/block/blk-core.c
> > > +++ b/block/blk-core.c
> > > @@ -566,6 +566,11 @@ void blk_cleanup_queue(struct request_queue *q)
> > >  	spin_lock_irq(lock);
> > >  	if (!q->mq_ops)
> > >  		__blk_drain_queue(q, true);
> > > +	spin_unlock_irq(lock);
> > > +
> > > +	blk_mq_debugfs_unregister_mq(q);
> > > +
> > > +	spin_lock_irq(lock);
> > >  	queue_flag_set(QUEUE_FLAG_DEAD, q);
> > >  	spin_unlock_irq(lock);
> > 
> > Do we actually have to hold the queue lock when we set QUEUE_FLAG_DEAD?
> 
> It's way easier to keep that spin_lock()/spin_unlock() pair than to analyze
> the block driver core and all block drivers to see whether or not any
> concurrent queue flag changes could occur.

Ah, I didn't realize that queue_flag_set() did a non-atomic set. I'm
wondering if anything bad could happen if something raced between when
we drop the lock and regrab it. Maybe just move the
blk_mq_debugfs_unregister_mq() before we grab the lock the first time
instead?