Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] block: rename 'q->debugfs_dir' in blk_unregister_queue()
@ 2020-02-11  3:51 yu kuai
  2020-02-12  3:27 ` Bart Van Assche
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: yu kuai @ 2020-02-11  3:51 UTC (permalink / raw)
  To: axboe, ming.lei
  Cc: yukuai3, yi.zhang, zhangxiaoxu5, luoshijie1, linux-block, linux-kernel

syzbot is reporting use after free bug in debugfs_remove[1].

This is because in request_queue, 'q->debugfs_dir' and
'q->blk_trace->dir' could be the same dir. And in __blk_release_queue(),
blk_mq_debugfs_unregister() will remove everything inside the dir.

With futher investigation of the reporduce repro, the problem can be
reporduced by following procedure:

1. LOOP_CTL_ADD, create a request_queue q1, blk_mq_debugfs_register() will
create the dir.
2. LOOP_CTL_REMOVE, blk_release_queue() will add q1 to release queue.
3. LOOP_CTL_ADD, create another request_queue q2,blk_mq_debugfs_register()
will fail because the dir aready exist.
4. BLKTRACESETUP, create two files(msg and dropped) inside the dir.
5. call __blk_release_queue() for q1, debugfs_remove_recursive() will
delete the files created in step 4.
6. LOOP_CTL_REMOVE, blk_release_queue() will add q2 to release queue.
And when __blk_release_queue() is called for q2, blk_trace_shutdown() will
try to release the two files created in step 4, wich are aready released
in step 5.

|thread1		  |kworker	             |thread2               |
| ----------------------- | ------------------------ | -------------------- |
|loop_control_ioctl       |                          |                      |
| loop_add                |                          |                      |
|  blk_mq_debugfs_register|                          |                      |
|   debugfs_create_dir    |                          |                      |
|loop_control_ioctl       |                          |                      |
| loop_remove		  |                          |                      |
|  blk_release_queue      |                          |                      |
|   schedule_work         |                          |                      |
|			  |			     |loop_control_ioctl    |
|			  |			     | loop_add             |
|			  |			     |  ...                 |
|			  |			     |blk_trace_ioctl       |
|			  |			     | __blk_trace_setup    |
|			  |			     |   debugfs_create_file|
|			  |__blk_release_queue       |                      |
|			  | blk_mq_debugfs_unregister|                      |
|			  |  debugfs_remove_recursive|                      |
|			  |			     |loop_control_ioctl    |
|			  |			     | loop_remove          |
|			  |			     |  ...                 |
|			  |__blk_release_queue       |                      |
|			  | blk_trace_shutdown       |                      |
|			  |  debugfs_remove          |                      |

commit dc9edc44de6c ("block: Fix a blk_exit_rl() regression") pushed the
final release of request_queue to a workqueue, so, when loop_add() is
called again(step 3), __blk_release_queue() might not been called yet,
which causes the problem.

Fix the problem by renaming 'q->debugfs_dir' in blk_unregister_queue().

[1] https://syzkaller.appspot.com/bug?extid=903b72a010ad6b7a40f2
References: CVE-2019-19770
Fixes: commit dc9edc44de6c ("block: Fix a blk_exit_rl() regression")
Reported-by: syzbot <syz...@syzkaller.appspotmail.com>
Signed-off-by: yu kuai <yukuai3@huawei.com>
---
 block/blk-sysfs.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index fca9b158f4a0..69d28b3f52d0 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -11,6 +11,7 @@
 #include <linux/blktrace_api.h>
 #include <linux/blk-mq.h>
 #include <linux/blk-cgroup.h>
+#include <linux/debugfs.h>
 
 #include "blk.h"
 #include "blk-mq.h"
@@ -1011,6 +1012,33 @@ int blk_register_queue(struct gendisk *disk)
 }
 EXPORT_SYMBOL_GPL(blk_register_queue);
 
+/*
+ * blk_prepare_release_queue - rename q->debugfs_dir
+ * @q: request_queue of which the dir to be renamed belong to.
+ *
+ * Because the final release of request_queue is in a workqueue, the
+ * cleanup might not been finished yet while the same device start to
+ * create. It's not correct if q->debugfs_dir still exist while trying
+ * to create a new one.
+ */
+static struct dentry *blk_prepare_release_queue(struct request_queue *q)
+{
+	struct dentry *new = NULL;
+	char name[DNAME_INLINE_LEN];
+	int i = 0;
+
+	if (IS_ERR_OR_NULL(q->debugfs_dir))
+		return q->debugfs_dir;
+
+	while (new == NULL) {
+		sprintf(name, "ready_to_remove_%d", i++);
+		new = debugfs_rename(blk_debugfs_root, q->debugfs_dir,
+				     blk_debugfs_root, name);
+	}
+
+	return new;
+}
+
 /**
  * blk_unregister_queue - counterpart of blk_register_queue()
  * @disk: Disk of which the request queue should be unregistered from sysfs.
@@ -1039,6 +1067,7 @@ void blk_unregister_queue(struct gendisk *disk)
 	mutex_unlock(&q->sysfs_lock);
 
 	mutex_lock(&q->sysfs_dir_lock);
+	q->debugfs_dir = blk_prepare_release_queue(q);
 	/*
 	 * Remove the sysfs attributes before unregistering the queue data
 	 * structures that can be modified through sysfs.
-- 
2.17.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] block: rename 'q->debugfs_dir' in blk_unregister_queue()
  2020-02-11  3:51 [PATCH] block: rename 'q->debugfs_dir' in blk_unregister_queue() yu kuai
@ 2020-02-12  3:27 ` Bart Van Assche
  2020-02-12  4:20   ` yukuai (C)
  2020-02-13  4:38 ` kbuild test robot
  2020-02-13 11:39 ` kbuild test robot
  2 siblings, 1 reply; 5+ messages in thread
From: Bart Van Assche @ 2020-02-12  3:27 UTC (permalink / raw)
  To: yu kuai, axboe, ming.lei
  Cc: yi.zhang, zhangxiaoxu5, luoshijie1, linux-block, linux-kernel

On 2020-02-10 19:51, yu kuai wrote:
> +static struct dentry *blk_prepare_release_queue(struct request_queue *q)
> +{
> +	struct dentry *new = NULL;
> +	char name[DNAME_INLINE_LEN];
> +	int i = 0;
> +
> +	if (IS_ERR_OR_NULL(q->debugfs_dir))
> +		return q->debugfs_dir;
> +
> +	while (new == NULL) {
> +		sprintf(name, "ready_to_remove_%d", i++);
> +		new = debugfs_rename(blk_debugfs_root, q->debugfs_dir,
> +				     blk_debugfs_root, name);
> +	}
> +
> +	return new;
> +}

What is the behavior of this loop if multiple block devices are being
removed concurrently? Does it perhaps change remove block device removal
from an O(1) into an O(n) operation?

Since this scenario may only matter to syzbot tests: has it been
considered to delay block device creation if the debugfs directory from
a previous incarnation of the block device still exists?

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] block: rename 'q->debugfs_dir' in blk_unregister_queue()
  2020-02-12  3:27 ` Bart Van Assche
@ 2020-02-12  4:20   ` yukuai (C)
  0 siblings, 0 replies; 5+ messages in thread
From: yukuai (C) @ 2020-02-12  4:20 UTC (permalink / raw)
  To: Bart Van Assche, axboe, ming.lei
  Cc: yi.zhang, zhangxiaoxu5, luoshijie1, linux-block, linux-kernel

On 2020/2/12 11:27, Bart Van Assche wrote:
> What is the behavior of this loop if multiple block devices are being
> removed concurrently? Does it perhaps change remove block device removal
> from an O(1) into an O(n) operation?
Yes, there may be performance overhead.(I thought it's minimal) However,
I can change the name of dir form "read_to_remove_%d" to
"read_to_remove_%s(dev_name)_%d" to fix that.
> 
> Since this scenario may only matter to syzbot tests: has it been
> considered to delay block device creation if the debugfs directory from
> a previous incarnation of the block device still exists?
> 
I think it's a bug device creation succeed when the debugfs directory
exist. Of course delay block device creation can fix the problem, but I
haven't come up with a good solution. And by renaming the dir, there is
no need to delay cration.

Thanks!
Yu Kuai


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] block: rename 'q->debugfs_dir' in blk_unregister_queue()
  2020-02-11  3:51 [PATCH] block: rename 'q->debugfs_dir' in blk_unregister_queue() yu kuai
  2020-02-12  3:27 ` Bart Van Assche
@ 2020-02-13  4:38 ` kbuild test robot
  2020-02-13 11:39 ` kbuild test robot
  2 siblings, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2020-02-13  4:38 UTC (permalink / raw)
  To: yu kuai
  Cc: kbuild-all, axboe, ming.lei, yukuai3, yi.zhang, zhangxiaoxu5,
	luoshijie1, linux-block, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 3201 bytes --]

Hi yu,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on block/for-next]
[also build test ERROR on v5.6-rc1 next-20200212]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/yu-kuai/block-rename-q-debugfs_dir-in-blk_unregister_queue/20200213-091022
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: um-x86_64_defconfig (attached as .config)
compiler: gcc-7 (Debian 7.5.0-4) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=um SUBARCH=x86_64

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   block/blk-sysfs.c: In function 'blk_prepare_release_queue':
>> block/blk-sysfs.c:1030:22: error: 'struct request_queue' has no member named 'debugfs_dir'
     if (IS_ERR_OR_NULL(q->debugfs_dir))
                         ^~
   block/blk-sysfs.c:1031:11: error: 'struct request_queue' has no member named 'debugfs_dir'
      return q->debugfs_dir;
              ^~
>> block/blk-sysfs.c:1035:24: error: 'blk_debugfs_root' undeclared (first use in this function); did you mean 'blk_mq_debugfs_attr'?
      new = debugfs_rename(blk_debugfs_root, q->debugfs_dir,
                           ^~~~~~~~~~~~~~~~
                           blk_mq_debugfs_attr
   block/blk-sysfs.c:1035:24: note: each undeclared identifier is reported only once for each function it appears in
   block/blk-sysfs.c:1035:43: error: 'struct request_queue' has no member named 'debugfs_dir'
      new = debugfs_rename(blk_debugfs_root, q->debugfs_dir,
                                              ^~
   block/blk-sysfs.c: In function 'blk_unregister_queue':
   block/blk-sysfs.c:1070:3: error: 'struct request_queue' has no member named 'debugfs_dir'
     q->debugfs_dir = blk_prepare_release_queue(q);
      ^~

vim +1030 block/blk-sysfs.c

  1014	
  1015	/*
  1016	 * blk_prepare_release_queue - rename q->debugfs_dir
  1017	 * @q: request_queue of which the dir to be renamed belong to.
  1018	 *
  1019	 * Because the final release of request_queue is in a workqueue, the
  1020	 * cleanup might not been finished yet while the same device start to
  1021	 * create. It's not correct if q->debugfs_dir still exist while trying
  1022	 * to create a new one.
  1023	 */
  1024	static struct dentry *blk_prepare_release_queue(struct request_queue *q)
  1025	{
  1026		struct dentry *new = NULL;
  1027		char name[DNAME_INLINE_LEN];
  1028		int i = 0;
  1029	
> 1030		if (IS_ERR_OR_NULL(q->debugfs_dir))
  1031			return q->debugfs_dir;
  1032	
  1033		while (new == NULL) {
  1034			sprintf(name, "ready_to_remove_%d", i++);
> 1035			new = debugfs_rename(blk_debugfs_root, q->debugfs_dir,
  1036					     blk_debugfs_root, name);
  1037		}
  1038	
  1039		return new;
  1040	}
  1041	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 8477 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] block: rename 'q->debugfs_dir' in blk_unregister_queue()
  2020-02-11  3:51 [PATCH] block: rename 'q->debugfs_dir' in blk_unregister_queue() yu kuai
  2020-02-12  3:27 ` Bart Van Assche
  2020-02-13  4:38 ` kbuild test robot
@ 2020-02-13 11:39 ` kbuild test robot
  2 siblings, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2020-02-13 11:39 UTC (permalink / raw)
  To: yu kuai
  Cc: kbuild-all, axboe, ming.lei, yukuai3, yi.zhang, zhangxiaoxu5,
	luoshijie1, linux-block, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 3276 bytes --]

Hi yu,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on block/for-next]
[also build test ERROR on v5.6-rc1 next-20200213]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/yu-kuai/block-rename-q-debugfs_dir-in-blk_unregister_queue/20200213-091022
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: mips-fuloong2e_defconfig (attached as .config)
compiler: mips64el-linux-gcc (GCC) 5.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=5.5.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   block/blk-sysfs.c: In function 'blk_prepare_release_queue':
   block/blk-sysfs.c:1030:22: error: 'struct request_queue' has no member named 'debugfs_dir'
     if (IS_ERR_OR_NULL(q->debugfs_dir))
                         ^
   block/blk-sysfs.c:1031:11: error: 'struct request_queue' has no member named 'debugfs_dir'
      return q->debugfs_dir;
              ^
>> block/blk-sysfs.c:1035:24: error: 'blk_debugfs_root' undeclared (first use in this function)
      new = debugfs_rename(blk_debugfs_root, q->debugfs_dir,
                           ^
   block/blk-sysfs.c:1035:24: note: each undeclared identifier is reported only once for each function it appears in
   block/blk-sysfs.c:1035:43: error: 'struct request_queue' has no member named 'debugfs_dir'
      new = debugfs_rename(blk_debugfs_root, q->debugfs_dir,
                                              ^
   block/blk-sysfs.c: In function 'blk_unregister_queue':
   block/blk-sysfs.c:1070:3: error: 'struct request_queue' has no member named 'debugfs_dir'
     q->debugfs_dir = blk_prepare_release_queue(q);
      ^

vim +/blk_debugfs_root +1035 block/blk-sysfs.c

  1014	
  1015	/*
  1016	 * blk_prepare_release_queue - rename q->debugfs_dir
  1017	 * @q: request_queue of which the dir to be renamed belong to.
  1018	 *
  1019	 * Because the final release of request_queue is in a workqueue, the
  1020	 * cleanup might not been finished yet while the same device start to
  1021	 * create. It's not correct if q->debugfs_dir still exist while trying
  1022	 * to create a new one.
  1023	 */
  1024	static struct dentry *blk_prepare_release_queue(struct request_queue *q)
  1025	{
  1026		struct dentry *new = NULL;
  1027		char name[DNAME_INLINE_LEN];
  1028		int i = 0;
  1029	
  1030		if (IS_ERR_OR_NULL(q->debugfs_dir))
  1031			return q->debugfs_dir;
  1032	
  1033		while (new == NULL) {
  1034			sprintf(name, "ready_to_remove_%d", i++);
> 1035			new = debugfs_rename(blk_debugfs_root, q->debugfs_dir,
  1036					     blk_debugfs_root, name);
  1037		}
  1038	
  1039		return new;
  1040	}
  1041	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 19355 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-11  3:51 [PATCH] block: rename 'q->debugfs_dir' in blk_unregister_queue() yu kuai
2020-02-12  3:27 ` Bart Van Assche
2020-02-12  4:20   ` yukuai (C)
2020-02-13  4:38 ` kbuild test robot
2020-02-13 11:39 ` kbuild test robot

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git