| Message ID | 20200211035137.19454-1-yukuai3@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: rename 'q->debugfs_dir' in blk_unregister_queue() | expand |
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.
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
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
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
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.
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(+)