| Message ID | 20170425203745.19946-6-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 25, 2017 at 01:37:40PM -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 triggers a use-after-free, unregister the debugfs attributes > before a queue reaches the "dead" state. Still not happy with this commit message. I'd prefer: We currently call blk_mq_free_queue() from blk_cleanup_queue() before we unregister the debugfs attributes for that queue in blk_release_queue(). This leaves a window open during which accessing most of the mq debugfs attributes would cause a use-after-free. Additionally, the "state" attribute allows running the queue, which we should not do after the queue has entered the "dead" state. Fix both of these cases by unregistering the debugfs attributes before this. Jens, could you ack that dropping the lock is okay? > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Reviewed-by: Hannes Reinecke <hare@suse.com> > Reviewed-by: Omar Sandoval <osandov@fb.com> > --- > block/blk-core.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/block/blk-core.c b/block/blk-core.c > index a49b0830aaaf..33c91a4bee97 100644 > --- 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); > > -- > 2.12.2 >
On 04/25/2017 02:30 PM, Omar Sandoval wrote: > On Tue, Apr 25, 2017 at 01:37:40PM -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 triggers a use-after-free, unregister the debugfs attributes >> before a queue reaches the "dead" state. > > Still not happy with this commit message. I'd prefer: > > We currently call blk_mq_free_queue() from blk_cleanup_queue() before we > unregister the debugfs attributes for that queue in blk_release_queue(). > This leaves a window open during which accessing most of the mq debugfs > attributes would cause a use-after-free. Additionally, the "state" > attribute allows running the queue, which we should not do after the > queue has entered the "dead" state. Fix both of these cases by > unregistering the debugfs attributes before this. > > Jens, could you ack that dropping the lock is okay? Looks fine to me. However, I think there's room for improvement here. Why don't we just make it: if (!q->mq_ops) { spin_lock_irq(lock); __blk_drain_queue(q, true); } else { blk_mq_debugfs_unregister_mq(q); spin_lock_irq(lock); } queue_flag_set(QUEUE_FLAG_DEAD, q); [...] Would seem much more readable to me, and less dropping/acquiring for cases where we don't need it.
On Tue, 2017-04-25 at 14:30 -0700, Omar Sandoval wrote: > On Tue, Apr 25, 2017 at 01:37:40PM -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 triggers a use-after-free, unregister the debugfs attributes > > before a queue reaches the "dead" state. > > Still not happy with this commit message. I'd prefer: > > We currently call blk_mq_free_queue() from blk_cleanup_queue() before we > unregister the debugfs attributes for that queue in blk_release_queue(). > This leaves a window open during which accessing most of the mq debugfs > attributes would cause a use-after-free. Additionally, the "state" > attribute allows running the queue, which we should not do after the > queue has entered the "dead" state. Fix both of these cases by > unregistering the debugfs attributes before this. Hello Omar, That's a very verbose description. How about this? Unregister the debugfs attributes before freeing of request queue resources starts to avoid that a use-after-free can be triggered through one of the debugfs attributes. Bart.
On 04/25/2017 03:24 PM, Bart Van Assche wrote: > On Tue, 2017-04-25 at 14:30 -0700, Omar Sandoval wrote: >> On Tue, Apr 25, 2017 at 01:37:40PM -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 triggers a use-after-free, unregister the debugfs attributes >>> before a queue reaches the "dead" state. >> >> Still not happy with this commit message. I'd prefer: >> >> We currently call blk_mq_free_queue() from blk_cleanup_queue() before we >> unregister the debugfs attributes for that queue in blk_release_queue(). >> This leaves a window open during which accessing most of the mq debugfs >> attributes would cause a use-after-free. Additionally, the "state" >> attribute allows running the queue, which we should not do after the >> queue has entered the "dead" state. Fix both of these cases by >> unregistering the debugfs attributes before this. > > Hello Omar, > > That's a very verbose description. How about this? > > Unregister the debugfs attributes before freeing of request queue > resources starts to avoid that a use-after-free can be triggered > through one of the debugfs attributes. Personally I find Omar's commit message much cleaner to read, and more easily understandable. We really don't need to be laconic in commit messages.
On Tue, Apr 25, 2017 at 10:24:48PM +0000, Bart Van Assche wrote: > On Tue, 2017-04-25 at 14:30 -0700, Omar Sandoval wrote: > > On Tue, Apr 25, 2017 at 01:37:40PM -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 triggers a use-after-free, unregister the debugfs attributes > > > before a queue reaches the "dead" state. > > > > Still not happy with this commit message. I'd prefer: > > > > We currently call blk_mq_free_queue() from blk_cleanup_queue() before we > > unregister the debugfs attributes for that queue in blk_release_queue(). > > This leaves a window open during which accessing most of the mq debugfs > > attributes would cause a use-after-free. Additionally, the "state" > > attribute allows running the queue, which we should not do after the > > queue has entered the "dead" state. Fix both of these cases by > > unregistering the debugfs attributes before this. > > Hello Omar, > > That's a very verbose description. How about this? > > Unregister the debugfs attributes before freeing of request queue > resources starts to avoid that a use-after-free can be triggered > through one of the debugfs attributes. > > Bart. Are you aware that there is nothing wrong with a descriptive commit message?
On Tue, 2017-04-25 at 14:30 -0700, Omar Sandoval wrote:
> Jens, could you ack that dropping the lock is okay?
Hello Omar,
If you have a look at the block layer history then you will see that the
current approach for shutting down block layer queues is an approach that
I had introduced myself. See also commits 3f3299d5c026, 807592a4fafb and
c246e80d8673.
Bart.
On 04/26/2017 01:32 PM, Bart Van Assche wrote: > On Tue, 2017-04-25 at 14:30 -0700, Omar Sandoval wrote: >> Jens, could you ack that dropping the lock is okay? > > Hello Omar, > > If you have a look at the block layer history then you will see that the > current approach for shutting down block layer queues is an approach that > I had introduced myself. See also commits 3f3299d5c026, 807592a4fafb and > c246e80d8673. Bart, did you see my reply on the topic?
On Wed, Apr 26, 2017 at 08:32:31PM +0000, Bart Van Assche wrote: > On Tue, 2017-04-25 at 14:30 -0700, Omar Sandoval wrote: > > Jens, could you ack that dropping the lock is okay? > > Hello Omar, > > If you have a look at the block layer history then you will see that the > current approach for shutting down block layer queues is an approach that > I had introduced myself. See also commits 3f3299d5c026, 807592a4fafb and > c246e80d8673. > > Bart. Thanks for the reference. Jens' suggestion looks the cleanest to me.
On Tue, 2017-04-25 at 14:41 -0700, Jens Axboe wrote: > Looks fine to me. However, I think there's room for improvement here. > Why don't we just make it: > > if (!q->mq_ops) { > spin_lock_irq(lock); > __blk_drain_queue(q, true); > } else { > blk_mq_debugfs_unregister_mq(q); > spin_lock_irq(lock); > } > > queue_flag_set(QUEUE_FLAG_DEAD, q); > [...] > > Would seem much more readable to me, and less dropping/acquiring for > cases where we don't need it. Hello Jens, This looks fine to me. I will update the patch. Bart.
diff --git a/block/blk-core.c b/block/blk-core.c index a49b0830aaaf..33c91a4bee97 100644 --- 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);