| Message ID | 51f5cd27-4ae4-0a21-63e2-c7a2ec95e257@sandisk.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 10, 2017 at 11:40:49AM -0700, Bart Van Assche wrote: > On 04/10/2017 11:28 AM, Jens Axboe wrote: > > On 03/30/2017 12:21 PM, Bart Van Assche wrote: > >> This is a short patch series with one patch that exports the queue state > >> and another patch that shows symbolic names for hctx state and flags > >> instead of a numerical bitmask. > >> > >> Please consider these patches for kernel v4.12. > > > > Thanks, added for 4.12. > > Hello Jens, > > Thanks! This infrastructure was essential while analyzing queue stalls. > > After I had posted this series I improved and extended the blk-mq debugfs > functionality further. Please consider including the patch below in v4.12. > > Thanks, > > Bart. > > > From: Bart Van Assche <bart.vanassche@sandisk.com> > Subject: [PATCH] blk-mq: Two fixes for the code that exports the queue state > > Remove the array entry for QUEUE_FLAG_RESTART since that flag has > been removed after the blk-mq-debugfs patch that introduced this > array entry was posted. > > Avoid that querying the queue state of a dead queue triggers a > kernel crash. > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > --- > block/blk-mq-debugfs.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c > index 91d09f58a596..a1ce823578c7 100644 > --- a/block/blk-mq-debugfs.c > +++ b/block/blk-mq-debugfs.c > @@ -92,7 +92,6 @@ static const char *const blk_queue_flag_name[] = { > [QUEUE_FLAG_FLUSH_NQ] = "FLUSH_NQ", > [QUEUE_FLAG_DAX] = "DAX", > [QUEUE_FLAG_STATS] = "STATS", > - [QUEUE_FLAG_RESTART] = "RESTART", > [QUEUE_FLAG_POLL_STATS] = "POLL_STATS", > [QUEUE_FLAG_REGISTERED] = "REGISTERED", > }; > @@ -112,6 +111,14 @@ static ssize_t blk_queue_flags_store(struct file *file, const char __user *ubuf, > struct request_queue *q = file_inode(file)->i_private; > char op[16] = { }, *s; > > + /* > + * The debugfs attributes are removed after blk_cleanup_queue() has > + * called blk_mq_free_queue(). Return if QUEUE_FLAG_DEAD has been set > + * to avoid triggering a use-after-free. > + */ > + if (blk_queue_dead(q)) > + return -ENOENT; > + Can you just move blk_queue_flags_fops to blk_mq_debugfs_queue_attrs instead of adding blk_queue_attrs?
On Mon, 2017-04-10 at 13:00 -0700, Omar Sandoval wrote: > Can you just move blk_queue_flags_fops to blk_mq_debugfs_queue_attrs > instead of adding blk_queue_attrs? Hello Omar, Are you aware that that change will move the "state" attribute one level down in the hierarchy? blk_mq_debugfs_queue_attrs attributes are created under "mq" while blk_queue_flags_fops attributes are created at the same level as the "mq" attribute. I had added blk_queue_flags_fops because the "state" attribute is not related to blk-mq. That attribute is also relevant for single-queue block layer queues. Bart.
On Mon, Apr 10, 2017 at 08:12:00PM +0000, Bart Van Assche wrote: > On Mon, 2017-04-10 at 13:00 -0700, Omar Sandoval wrote: > > Can you just move blk_queue_flags_fops to blk_mq_debugfs_queue_attrs > > instead of adding blk_queue_attrs? > > Hello Omar, > > Are you aware that that change will move the "state" attribute one level > down in the hierarchy? blk_mq_debugfs_queue_attrs attributes are created > under "mq" while blk_queue_flags_fops attributes are created at the same > level as the "mq" attribute. I had added blk_queue_flags_fops because the > "state" attribute is not related to blk-mq. That attribute is also relevant > for single-queue block layer queues. Yes, I am aware of that. We don't set up debugfs for single-queue queues anyways, so the top-level directory is really just for blktrace. It simplifies the lifetime and cleanup to have everything under mq, so please move it there.
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c index 91d09f58a596..a1ce823578c7 100644 --- a/block/blk-mq-debugfs.c +++ b/block/blk-mq-debugfs.c @@ -92,7 +92,6 @@ static const char *const blk_queue_flag_name[] = { [QUEUE_FLAG_FLUSH_NQ] = "FLUSH_NQ", [QUEUE_FLAG_DAX] = "DAX", [QUEUE_FLAG_STATS] = "STATS", - [QUEUE_FLAG_RESTART] = "RESTART", [QUEUE_FLAG_POLL_STATS] = "POLL_STATS", [QUEUE_FLAG_REGISTERED] = "REGISTERED", }; @@ -112,6 +111,14 @@ static ssize_t blk_queue_flags_store(struct file *file, const char __user *ubuf, struct request_queue *q = file_inode(file)->i_private; char op[16] = { }, *s; + /* + * The debugfs attributes are removed after blk_cleanup_queue() has + * called blk_mq_free_queue(). Return if QUEUE_FLAG_DEAD has been set + * to avoid triggering a use-after-free. + */ + if (blk_queue_dead(q)) + return -ENOENT; + len = min(len, sizeof(op) - 1); if (copy_from_user(op, ubuf, len)) return -EFAULT;