| Message ID | 20170411205842.28137-2-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 11, 2017 at 01:58:37PM -0700, Bart Van Assche wrote: > The blk-mq debugfs attributes are removed after blk_cleanup_queue() > has finished. Since running a queue after a queue has entered the > "dead" state is not allowed, disallow this. This patch avoids that > an attempt to run a dead queue triggers a kernel crash. > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Cc: Omar Sandoval <osandov@fb.com> > Cc: Hannes Reinecke <hare@suse.com> > --- > block/blk-mq-debugfs.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c > index df9b688b877c..a1ce823578c7 100644 > --- a/block/blk-mq-debugfs.c > +++ b/block/blk-mq-debugfs.c > @@ -111,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; > -- > 2.12.0 > Hi, Bart, Looking at this, I think we have similar issues with most of the other debugfs files. Should we move the debugfs cleanup earlier?
On Thu, Apr 13, 2017 at 04:01:02PM -0700, Omar Sandoval wrote: > On Tue, Apr 11, 2017 at 01:58:37PM -0700, Bart Van Assche wrote: > > The blk-mq debugfs attributes are removed after blk_cleanup_queue() > > has finished. Since running a queue after a queue has entered the > > "dead" state is not allowed, disallow this. This patch avoids that > > an attempt to run a dead queue triggers a kernel crash. > > > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > > Cc: Omar Sandoval <osandov@fb.com> > > Cc: Hannes Reinecke <hare@suse.com> > > --- > > block/blk-mq-debugfs.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c > > index df9b688b877c..a1ce823578c7 100644 > > --- a/block/blk-mq-debugfs.c > > +++ b/block/blk-mq-debugfs.c > > @@ -111,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; > > -- > > 2.12.0 > > > > Hi, Bart, > > Looking at this, I think we have similar issues with most of the other > debugfs files. Should we move the debugfs cleanup earlier? In particular, I think we can call blk_mq_debugfs_unregister_hctxs() (which is somewhat poorly named, as it removes the whole mq directory) before we call blk_mq_free_queue(). I was under the impression that that's what it already did, but I think I was wrong.
On Thu, 2017-04-13 at 16:01 -0700, Omar Sandoval wrote: > On Tue, Apr 11, 2017 at 01:58:37PM -0700, Bart Van Assche wrote: > > The blk-mq debugfs attributes are removed after blk_cleanup_queue() > > has finished. Since running a queue after a queue has entered the > > "dead" state is not allowed, disallow this. This patch avoids that > > an attempt to run a dead queue triggers a kernel crash. > > > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > > Cc: Omar Sandoval <osandov@fb.com> > > Cc: Hannes Reinecke <hare@suse.com> > > --- > > block/blk-mq-debugfs.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c > > index df9b688b877c..a1ce823578c7 100644 > > --- a/block/blk-mq-debugfs.c > > +++ b/block/blk-mq-debugfs.c > > @@ -111,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; > > Looking at this, I think we have similar issues with most of the other > debugfs files. Should we move the debugfs cleanup earlier? Hello Omar, That's a good question. However, while I was debugging it was very convenient to be able to access the queue state after it had reached the "dead" state. Performing the cleanup earlier would be an alternative solution but would make debugging a bit harder ... Bart.
On Thu, Apr 13, 2017 at 11:05:32PM +0000, Bart Van Assche wrote: > On Thu, 2017-04-13 at 16:01 -0700, Omar Sandoval wrote: > > On Tue, Apr 11, 2017 at 01:58:37PM -0700, Bart Van Assche wrote: > > > The blk-mq debugfs attributes are removed after blk_cleanup_queue() > > > has finished. Since running a queue after a queue has entered the > > > "dead" state is not allowed, disallow this. This patch avoids that > > > an attempt to run a dead queue triggers a kernel crash. > > > > > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > > > Cc: Omar Sandoval <osandov@fb.com> > > > Cc: Hannes Reinecke <hare@suse.com> > > > --- > > > block/blk-mq-debugfs.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c > > > index df9b688b877c..a1ce823578c7 100644 > > > --- a/block/blk-mq-debugfs.c > > > +++ b/block/blk-mq-debugfs.c > > > @@ -111,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; > > > > Looking at this, I think we have similar issues with most of the other > > debugfs files. Should we move the debugfs cleanup earlier? > > Hello Omar, > > That's a good question. However, while I was debugging it was very convenient > to be able to access the queue state after it had reached the "dead" state. > Performing the cleanup earlier would be an alternative solution but would > make debugging a bit harder ... > > Bart. What useful information were you getting out of debugfs once the queue was already dead? Wasn't the interesting stuff freed at that point?
On Fri, 2017-04-14 at 00:40 -0700, Omar Sandoval wrote: > On Thu, Apr 13, 2017 at 11:05:32PM +0000, Bart Van Assche wrote: > > On Thu, 2017-04-13 at 16:01 -0700, Omar Sandoval wrote: > > > Looking at this, I think we have similar issues with most of the other > > > debugfs files. Should we move the debugfs cleanup earlier? > > > > That's a good question. However, while I was debugging it was very convenient > > to be able to access the queue state after it had reached the "dead" state. > > Performing the cleanup earlier would be an alternative solution but would > > make debugging a bit harder ... > > What useful information were you getting out of debugfs once the queue > was already dead? Wasn't the interesting stuff freed at that point? Hello Omar, I'm currently chasing a stall of dm-rq + dm-mpath that occurs after the queues below it have reached the "dead" state. I will look for another way to obtain the information I need such that we can remove the block layer queue debugfs information before these queues reach the "dead" state. Bart.
On Fri, Apr 14, 2017 at 04:12:01PM +0000, Bart Van Assche wrote: > On Fri, 2017-04-14 at 00:40 -0700, Omar Sandoval wrote: > > On Thu, Apr 13, 2017 at 11:05:32PM +0000, Bart Van Assche wrote: > > > On Thu, 2017-04-13 at 16:01 -0700, Omar Sandoval wrote: > > > > Looking at this, I think we have similar issues with most of the other > > > > debugfs files. Should we move the debugfs cleanup earlier? > > > > > > That's a good question. However, while I was debugging it was very convenient > > > to be able to access the queue state after it had reached the "dead" state. > > > Performing the cleanup earlier would be an alternative solution but would > > > make debugging a bit harder ... > > > > What useful information were you getting out of debugfs once the queue > > was already dead? Wasn't the interesting stuff freed at that point? > > Hello Omar, > > I'm currently chasing a stall of dm-rq + dm-mpath that occurs after the > queues below it have reached the "dead" state. I will look for another > way to obtain the information I need such that we can remove the block > layer queue debugfs information before these queues reach the "dead" > state. > > Bart. Thanks, Bart. In this case, the absence of the "mq" directory should tell you that the queue is dead. Will you move the cleanup in v2 or should I submit a separate patch?
On Fri, 2017-04-14 at 10:13 -0700, Omar Sandoval wrote: > Thanks, Bart. In this case, the absence of the "mq" directory should > tell you that the queue is dead. Will you move the cleanup in v2 or > should I submit a separate patch? Hello Omar, I will include a patch in v2 of this patch series that unregisters the debugfs attributes earlier. Bart.
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c index df9b688b877c..a1ce823578c7 100644 --- a/block/blk-mq-debugfs.c +++ b/block/blk-mq-debugfs.c @@ -111,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;
The blk-mq debugfs attributes are removed after blk_cleanup_queue() has finished. Since running a queue after a queue has entered the "dead" state is not allowed, disallow this. This patch avoids that an attempt to run a dead queue triggers a kernel crash. Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> Cc: Omar Sandoval <osandov@fb.com> Cc: Hannes Reinecke <hare@suse.com> --- block/blk-mq-debugfs.c | 8 ++++++++ 1 file changed, 8 insertions(+)