block: fix blktrace debugfs use after free
Subsystems (6)
v6 cover block: fix blktrace debugfs use after free
Here is v6 of the blktrace fixes which address the debugfs use after free. I've followed the strategy suggested by Christoph of open coding the solution in place, and extended it with the required work for partitions and scsi-generic. Jan's blktrace sparse fix ended up depending on one of my patch, "blktrace: break out of blktrace setup on concurrent calls", and so he has sent those for inclusion prior to my series. This series would have to be applied after those two patches from Jan are merged then.
Since the patch "blktrace: fix debugfs use after free" ends up being the only one modified lately, I've moved that patch to be the last one in the series now.
You can find these on my git tree branch 20200608-blktrace-fixes based on linux-next 20200608 [0].
Hopefully this is it.
[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20200608-blktrace-fixes
Luis Chamberlain (6):
block: add docs for gendisk / request_queue refcount helpers
block: clarify context for refcount increment helpers
block: revert back to synchronous request_queue removal
blktrace: annotate required lock on do_blk_trace_setup()
loop: be paranoid on exit and prevent new additions / removals
blktrace: fix debugfs use after free
block/blk-core.c | 27 ++++++++++-- block/blk-mq-debugfs.c | 5 --- block/blk-sysfs.c | 83 +++++++++++++++++++++++++++--------- block/blk.h | 2 - block/genhd.c | 73 ++++++++++++++++++++++++++++++- block/partitions/core.c | 3 ++ drivers/block/loop.c | 4 ++ drivers/scsi/sg.c | 3 ++ include/linux/blkdev.h | 6 +-- include/linux/blktrace_api.h | 1 - include/linux/genhd.h | 1 + kernel/trace/blktrace.c | 55 ++++++++++++++++++------ 12 files changed, 214 insertions(+), 49 deletions(-)
Signature · 1 line
2.26.2
PATCH v6 1/6 block: add docs for gendisk / request_queue refcount helpers
This adds documentation for the gendisk / request_queue refcount helpers.
--- block/blk-core.c | 13 +++++++++++++ block/genhd.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 1 deletion(-)
block/blk-core.c +13 -0
@@ -321,6 +321,13 @@ void blk_clear_pm_only(struct request_queue *q) }EXPORT_SYMBOL_GPL (blk_clear_pm_only); +/** +* blk_put_queue - decrement the request_queue refcount +* @q: the request_queue structure to decrement the refcount for +* +* Decrements the refcount of the request_queue kobject. When this reaches 0 +* we'll have blk_release_queue() called. +*/ void blk_put_queue (struct request_queue *q) {kobject_put (&q->kobj);
@@ -598,6 +605,12 @@ struct request_queue *blk_alloc_queue(make_request_fn make_request, int node_id) }EXPORT_SYMBOL (blk_alloc_queue); +/** +* blk_get_queue - increment the request_queue refcount +* @q: the request_queue structure to increment the refcount for +* +* Increment the refcount of the request_queue kobject. +*/ bool blk_get_queue (struct request_queue *q) {if (likely (!blk_queue_dying (q))) {
block/genhd.c +49 -1
80 lines not on this page · raw message
Signature · 1 line
2.26.2
PATCH v6 2/6 block: clarify context for refcount increment helpers
In reply to Luis ChamberlainLet us clarify the context under which the helpers to increment the refcount for the gendisk and request_queue can be called under. We make this explicit on the places where we may sleep with might_sleep().
We don't address the decrement context yet, as that needs some extra work and fixes, but will be addressed in the next patch.
--- block/blk-core.c | 2 ++ block/genhd.c | 6 ++++++ 2 files changed, 8 insertions(+)
block/blk-core.c +2 -0
@@ -610,6 +610,8 @@ EXPORT_SYMBOL(blk_alloc_queue);* @q: the request_queue structure to increment the refcount for * * Increment the refcount of the request_queue kobject. +* +* Context: Any context. */ bool blk_get_queue (struct request_queue *q) {
block/genhd.c +6 -0
@@ -985,11 +985,15 @@ static ssize_t disk_badblocks_store(struct device *dev,* * This function gets the structure containing partitioning * information for the given device @devt. +* +* Context: can sleep */ struct gendisk *get_gendisk (dev_t devt,int *partno) {struct gendisk *disk =NULL ; +might_sleep (); +if (MAJOR (devt) !=BLOCK_EXT_MAJOR ) {struct kobject *kobj;
@@ -1764,6 +1768,8 @@ EXPORT_SYMBOL(__alloc_disk_node);* * This increments the refcount for the struct gendisk, and the gendisk's * fops module owner. +* +* Context: Any context. */ struct kobject *get_disk_and_module (struct gendisk *disk) {
Signature · 1 line
2.26.2
PATCH v6 3/6 block: revert back to synchronous request_queue removal
In reply to Luis ChamberlainCommit dc9edc44de6c ("block: Fix a blk_exit_rl() regression") merged on v4.12 moved the work behind blk_release_queue() into a workqueue after a splat floated around which indicated some work on blk_release_queue() could sleep in blk_exit_rl(). This splat would be possible when a driver called blk_put_queue() or blk_cleanup_queue() (which calls blk_put_queue() as its final call) from an atomic context.
blk_put_queue() decrements the refcount for the request_queue kobject, and upon reaching 0 blk_release_queue() is called. Although blk_exit_rl() is now removed through commit db6d9952356 ("block: remove request_list code") on v5.0, we reserve the right to be able to sleep within blk_release_queue() context.
The last reference for the request_queue must not be called from atomic context. *When* the last reference to the request_queue reaches 0 varies, and so let's take the opportunity to document when that is expected to happen and also document the context of the related calls as best as possible so we can avoid future issues, and with the hopes that the synchronous request_queue removal sticks.
We revert back to synchronous request_queue removal because asynchronous removal creates a regression with expected userspace interaction with several drivers. An example is when removing the loopback driver, one uses ioctls from userspace to do so, but upon return and if successful, one expects the device to be removed. Likewise if one races to add another device the new one may not be added as it is still being removed. This was expected behavior before and it now fails as the device is still present and busy still. Moving to asynchronous request_queue removal could have broken many scripts which relied on the removal to have been completed if there was no error. Document this expectation as well so that this doesn't regress userspace again.
Using asynchronous request_queue removal however has helped us find other bugs. In the future we can test what could break with this arrangement by enabling CONFIG_DEBUG_KOBJECT_RELEASE.
While at it, update the docs with the context expectations for the request_queue / gendisk refcount decrement, and make these expectations explicit by using might_sleep().
--- block/blk-core.c | 8 ++++++++ block/blk-sysfs.c | 43 +++++++++++++++++++++--------------------- block/genhd.c | 17 +++++++++++++++++ include/linux/blkdev.h | 2 -- 4 files changed, 47 insertions(+), 23 deletions(-)
block/blk-core.c +8 -0
@@ -327,6 +327,9 @@ EXPORT_SYMBOL_GPL(blk_clear_pm_only);* * Decrements the refcount of the request_queue kobject. When this reaches 0 * we'll have blk_release_queue() called. +* +* Context: Any context, but the last reference must not be dropped from +* atomic context. */ void blk_put_queue (struct request_queue *q) {
@@ -359,9 +362,14 @@ EXPORT_SYMBOL_GPL(blk_set_queue_dying);* * Mark @q DYING, drain all pending requests, mark @q DEAD, destroy and * put it. All future requests will be failed immediately with -ENODEV. +* +* Context: can sleep */ void blk_cleanup_queue (struct request_queue *q) { +/* cannot be called from atomic context */ +might_sleep (); +WARN_ON_ONCE (blk_queue_registered (q));/* mark @q DYING, no new request or merges will be allowed afterwards */
block/blk-sysfs.c +22 -21
59 lines not on this page · raw message
block/genhd.c +17 -0
@@ -889,12 +889,19 @@ static void invalidate_partition(struct gendisk *disk, int partno)* The final removal of the struct gendisk happens when its refcount reaches 0 * with put_disk(), which should be called after del_gendisk(), if * __device_add_disk() was used. +* +* Drivers exist which depend on the release of the gendisk to be synchronous, +* it should not be deferred. +* +* Context: can sleep */ void del_gendisk (struct gendisk *disk) {struct disk_part_iter piter;struct hd_struct *part; +might_sleep (); +blk_integrity_del (disk);disk_del_events (disk);
@@ -1548,11 +1555,15 @@ int disk_expand_part_tbl(struct gendisk *disk, int partno)* drivers we also call blk_put_queue() for them, and we expect the * request_queue refcount to reach 0 at this point, and so the request_queue * will also be freed prior to the disk. +* +* Context: can sleep */ static void disk_release (struct device *dev) {struct gendisk *disk =dev_to_disk (dev); +might_sleep (); +blk_free_devt (dev->devt);disk_release_events (disk);kfree (disk->random);
@@ -1797,6 +1808,9 @@ EXPORT_SYMBOL(get_disk_and_module);* * This decrements the refcount for the struct gendisk. When this reaches 0 * we'll have disk_release() called. +* +* Context: Any context, but the last reference must not be dropped from +* atomic context. */ void put_disk (struct gendisk *disk) {
@@ -1811,6 +1825,9 @@ EXPORT_SYMBOL(put_disk);* * This is a counterpart of get_disk_and_module() and thus also of * get_gendisk(). +* +* Context: Any context, but the last reference must not be dropped from +* atomic context. */ void put_disk_and_module (struct gendisk *disk) {
include/linux/blkdev.h +0 -2
@@ -584,8 +584,6 @@ struct request_queue {size_t cmd_size; -struct work_struct release_work; -#define BLK_MAX_WRITE_HINTS 5u64 write_hints[BLK_MAX_WRITE_HINTS ]; };
Signature · 1 line
2.26.2
Re: [PATCH v6 3/6] block: revert back to synchronous request_queue removal
Quoted 2 lines · Luis Chamberlain
+ * Drivers exist which depend on the release of the
request_queueto be
+ * synchronous, it should not be deferred.
This sounds mysterious. Which drivers? Why do these depend on this function being synchronous? Anyway:
Re: [PATCH v6 3/6] block: revert back to synchronous request_queue removal
Quoted 6 lines · Bart Van Assche
Quoted 2 lines · Luis Chamberlain
+ * Drivers exist which depend on the release of the
request_queueto be
+ * synchronous, it should not be deferred.This sounds mysterious. Which drivers? Why do these depend on this function being synchronous?
Sorry that should be "Userspace can exist". I've fixed that.
Quoted 3 lines
Anyway:
Reviewed-by Bart Van Assche <[email protected]>
Luis
PATCH v6 4/6 blktrace: annotate required lock on do_blk_trace_setup()
In reply to Luis ChamberlainEnsure it is clear which lock is required on do_blk_trace_setup().
--- kernel/trace/blktrace.c | 2 ++ 1 file changed, 2 insertions(+)
kernel/trace/blktrace.c +2 -0
@@ -483,6 +483,8 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,struct dentry *dir =NULL ;int ret; +lockdep_assert_held (&q->blk_trace_mutex); +if (!buts->buf_size || !buts->buf_nr)return -EINVAL ;
Signature · 1 line
2.26.2
Re: [PATCH v6 4/6] blktrace: annotate required lock on do_blk_trace_setup()
Quoted 4 lines · Luis Chamberlain
Ensure it is clear which lock is required on
do_blk_trace_setup().Suggested-by Bart Van Assche <[email protected]>Signed-off-by Luis Chamberlain <[email protected]>
Looks good,
Re: [PATCH v6 4/6] blktrace: annotate required lock on do_blk_trace_setup()
In reply to Luis ChamberlainQuoted 1 line · Luis Chamberlain
Ensure it is clear which lock is required on
do_blk_trace_setup().
PATCH v6 5/6 loop: be paranoid on exit and prevent new additions / removals
In reply to Luis ChamberlainBe pedantic on removal as well and hold the mutex. This should prevent uses of addition while we exit.
--- drivers/block/loop.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/block/loop.c +4 -0
@@ -2402,6 +2402,8 @@ static void __exit loop_exit(void) range = max_loop ? max_loop << part_shift :1UL <<MINORBITS ; +mutex_lock (&loop_ctl_mutex); +idr_for_each (&loop_index_idr, &loop_exit_cb,NULL );idr_destroy (&loop_index_idr);
@@ -2409,6 +2411,8 @@ static void __exit loop_exit(void)unregister_blkdev (LOOP_MAJOR ,"loop" );misc_deregister (&loop_misc); + +mutex_unlock (&loop_ctl_mutex); }module_init (loop_init);
Signature · 1 line
2.26.2
PATCH v6 6/6 blktrace: fix debugfs use after free
In reply to Luis ChamberlainOn commit 6ac93117ab00 ("blktrace: use existing disk debugfs directory") merged on v4.12 Omar fixed the original blktrace code for request-based drivers (multiqueue). This however left in place a possible crash, if you happen to abuse blktrace while racing to remove / add a device.
We used to use asynchronous removal of the request_queue, and with that the issue was easier to reproduce. Now that we have reverted to synchronous removal of the request_queue, the issue is still possible to reproduce, its however just a bit more difficult.
We essentially run two instances of break-blktrace which add/remove a loop device, and setup a blktrace and just never tear the blktrace down. We do this twice in parallel. This is easily reproduced with the script run_0004.sh from break-blktrace [0].
We can end up with two types of panics each reflecting where we race, one a failed blktrace setup:
[ 252.426751] debugfs: Directory 'loop0' with parent 'block' already present!
[ 252.432265] BUG: kernel NULL pointer dereference, address: 00000000000000a0
[ 252.436592] #PF: supervisor write access in kernel mode
[ 252.439822] #PF: error_code(0x0002) - not-present page
[ 252.442967] PGD 0 P4D 0
[ 252.444656] Oops: 0002 [#1] SMP NOPTI
[ 252.446972] CPU: 10 PID: 1153 Comm: break-blktrace Tainted: G E 5.7.0-rc2-next-20200420+ #164
[ 252.452673] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1 04/01/2014
[ 252.456343] RIP: 0010:down_write+0x15/0x40
[ 252.458146] Code: eb ca e8 ae 22 8d ff cc cc cc cc cc cc cc cc cc cc cc cc
cc cc 0f 1f 44 00 00 55 48 89 fd e8 52 db ff ff 31 c0 ba 01 00
00 00 <f0> 48 0f b1 55 00 75 0f 48 8b 04 25 c0 8b 01 00 48 89
45 08 5d
[ 252.463638] RSP: 0018:ffffa626415abcc8 EFLAGS: 00010246
[ 252.464950] RAX: 0000000000000000 RBX: ffff958c25f0f5c0 RCX: ffffff8100000000
[ 252.466727] RDX: 0000000000000001 RSI: ffffff8100000000 RDI: 00000000000000a0
[ 252.468482] RBP: 00000000000000a0 R08: 0000000000000000 R09: 0000000000000001
[ 252.470014] R10: 0000000000000000 R11: ffff958d1f9227ff R12: 0000000000000000
[ 252.471473] R13: ffff958c25ea5380 R14: ffffffff8cce15f1 R15: 00000000000000a0
[ 252.473346] FS: 00007f2e69dee540(0000) GS:ffff958c2fc80000(0000) knlGS:0000000000000000
[ 252.475225] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 252.476267] CR2: 00000000000000a0 CR3: 0000000427d10004 CR4: 0000000000360ee0
[ 252.477526] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 252.478776] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 252.479866] Call Trace:
[ 252.480322] simple_recursive_removal+0x4e/0x2e0
[ 252.481078] ? debugfs_remove+0x60/0x60
[ 252.481725] ? relay_destroy_buf+0x77/0xb0
[ 252.482662] debugfs_remove+0x40/0x60
[ 252.483518] blk_remove_buf_file_callback+0x5/0x10
[ 252.484328] relay_close_buf+0x2e/0x60
[ 252.484930] relay_open+0x1ce/0x2c0
[ 252.485520] do_blk_trace_setup+0x14f/0x2b0
[ 252.486187] __blk_trace_setup+0x54/0xb0
[ 252.486803] blk_trace_ioctl+0x90/0x140
[ 252.487423] ? do_sys_openat2+0x1ab/0x2d0
[ 252.488053] blkdev_ioctl+0x4d/0x260
[ 252.488636] block_ioctl+0x39/0x40
[ 252.489139] ksys_ioctl+0x87/0xc0
[ 252.489675] __x64_sys_ioctl+0x16/0x20
[ 252.490380] do_syscall_64+0x52/0x180
[ 252.491032] entry_SYSCALL_64_after_hwframe+0x44/0xa9And the other on the device removal:
[ 128.528940] debugfs: Directory 'loop0' with parent 'block' already present!
[ 128.615325] BUG: kernel NULL pointer dereference, address: 00000000000000a0
[ 128.619537] #PF: supervisor write access in kernel mode
[ 128.622700] #PF: error_code(0x0002) - not-present page
[ 128.625842] PGD 0 P4D 0
[ 128.627585] Oops: 0002 [#1] SMP NOPTI
[ 128.629871] CPU: 12 PID: 544 Comm: break-blktrace Tainted: G E 5.7.0-rc2-next-20200420+ #164
[ 128.635595] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1 04/01/2014
[ 128.640471] RIP: 0010:down_write+0x15/0x40
[ 128.643041] Code: eb ca e8 ae 22 8d ff cc cc cc cc cc cc cc cc cc cc cc cc
cc cc 0f 1f 44 00 00 55 48 89 fd e8 52 db ff ff 31 c0 ba 01 00
00 00 <f0> 48 0f b1 55 00 75 0f 65 48 8b 04 25 c0 8b 01 00 48 89
45 08 5d
[ 128.650180] RSP: 0018:ffffa9c3c05ebd78 EFLAGS: 00010246
[ 128.651820] RAX: 0000000000000000 RBX: ffff8ae9a6370240 RCX: ffffff8100000000
[ 128.653942] RDX: 0000000000000001 RSI: ffffff8100000000 RDI: 00000000000000a0
[ 128.655720] RBP: 00000000000000a0 R08: 0000000000000002 R09: ffff8ae9afd2d3d0
[ 128.657400] R10: 0000000000000056 R11: 0000000000000000 R12: 0000000000000000
[ 128.659099] R13: 0000000000000000 R14: 0000000000000003 R15: 00000000000000a0
[ 128.660500] FS: 00007febfd995540(0000) GS:ffff8ae9afd00000(0000) knlGS:0000000000000000
[ 128.662204] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 128.663426] CR2: 00000000000000a0 CR3: 0000000420042003 CR4: 0000000000360ee0
[ 128.664776] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 128.666022] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 128.667282] Call Trace:
[ 128.667801] simple_recursive_removal+0x4e/0x2e0
[ 128.668663] ? debugfs_remove+0x60/0x60
[ 128.669368] debugfs_remove+0x40/0x60
[ 128.669985] blk_trace_free+0xd/0x50
[ 128.670593] __blk_trace_remove+0x27/0x40
[ 128.671274] blk_trace_shutdown+0x30/0x40
[ 128.671935] blk_release_queue+0x95/0xf0
[ 128.672589] kobject_put+0xa5/0x1b0
[ 128.673188] disk_release+0xa2/0xc0
[ 128.673786] device_release+0x28/0x80
[ 128.674376] kobject_put+0xa5/0x1b0
[ 128.674915] loop_remove+0x39/0x50 [loop]
[ 128.675511] loop_control_ioctl+0x113/0x130 [loop]
[ 128.676199] ksys_ioctl+0x87/0xc0
[ 128.676708] __x64_sys_ioctl+0x16/0x20
[ 128.677274] do_syscall_64+0x52/0x180
[ 128.677823] entry_SYSCALL_64_after_hwframe+0x44/0xa9The common theme here is:
debugfs: Directory 'loop0' with parent 'block' already present
This crash happens because of how blktrace uses the debugfs directory where it places its files. Upon init we always create the same directory which would be needed by blktrace but we only do this for make_request drivers (multiqueue) block drivers, but never for request-based block drivers. Furthermore, that directory is only created on init for the entire disk. This means that if you use blktrace on a partition, we'll always be creating a new directory regardless of whether or not you are doing blktrace on a make_request driver (multiqueue) or a request-based block drivers.
These directory creations are only associated with a path, and so when a debugfs_remove() is called it removes everything in its way. A device removal will remove all blktrace files, and so if a blktrace is still present a cleanup of blktrace files later will end up trying to remove dentries pointing to NULL.
We can fix the UAF by using a debugfs directory which moving forward will always be accessible if debugfs is enabled for both make_request drivers (multiqueue) and request-based block drivers, *and* for all partitions upon creation. This ensures that removal of the directories only happens on device removal and removes the race of the files underneath an active blktrace.
We special-case a solution for scsi-generic as well which got blktrace support added by Christof via commit 6da127ad0918 ("blktrace: Add blktrace ioctls to SCSI generic devices") so upstream since v2.6.25. scsi-generic drives use a character device, however behind the scenes we have a scsi device with a request_queue. How this is used varies by class of driver (TYPE_DISK, TYPE_TAPE, etc). We simply create its a scsi-generic dedicated debugfs_dir and have blktrace use that when this interface is used.
This goes tested with:
o nvme partitions
o ISCSI with tgt, and blktracing against scsi-generic with:
o block
o tape
o cdrom
o media changer
o blktests
This patch is part of the work which disputes the severity of CVE-2019-19770 which shows this issue is not a core debugfs issue, but a misuse of debugfs within blktace.
--- block/blk-core.c | 4 --- block/blk-mq-debugfs.c | 5 ---- block/blk-sysfs.c | 40 +++++++++++++++++++++++++++ block/blk.h | 2 -- block/partitions/core.c | 3 ++ drivers/scsi/sg.c | 3 ++ include/linux/blkdev.h | 4 ++- include/linux/blktrace_api.h | 1 - include/linux/genhd.h | 1 + kernel/trace/blktrace.c | 53 ++++++++++++++++++++++++++++-------- 10 files changed, 91 insertions(+), 25 deletions(-)
block/blk-core.c +0 -4
@@ -51,9 +51,7 @@#include "blk-pm.h" #include "blk-rq-qos.h" -#ifdef CONFIG_DEBUG_FS struct dentry *blk_debugfs_root; -#endif EXPORT_TRACEPOINT_SYMBOL_GPL (block_bio_remap);EXPORT_TRACEPOINT_SYMBOL_GPL (block_rq_remap);
@@ -1937,9 +1935,7 @@ int __init blk_dev_init(void) blk_requestq_cachep =kmem_cache_create ("request_queue" ,sizeof (struct request_queue ),0 ,SLAB_PANIC ,NULL ); -#ifdef CONFIG_DEBUG_FS blk_debugfs_root =debugfs_create_dir ("block" ,NULL ); -#endif return 0 ; }
block/blk-mq-debugfs.c +0 -5
@@ -824,9 +824,6 @@ void blk_mq_debugfs_register(struct request_queue *q)struct blk_mq_hw_ctx *hctx;int i; - q->debugfs_dir =debugfs_create_dir (kobject_name (q->kobj.parent), - blk_debugfs_root); -debugfs_create_files (q->debugfs_dir, q, blk_mq_debugfs_queue_attrs);/*
@@ -857,9 +854,7 @@ void blk_mq_debugfs_register(struct request_queue *q)void blk_mq_debugfs_unregister (struct request_queue *q) { -debugfs_remove_recursive (q->debugfs_dir); q->sched_debugfs_dir =NULL ; - q->debugfs_dir =NULL ; } static void blk_mq_debugfs_register_ctx(struct blk_mq_hw_ctx *hctx,
block/blk-sysfs.c +40 -0
@@ -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"
@@ -918,6 +919,9 @@ static void blk_release_queue(struct kobject *kobj)blk_trace_shutdown (q); +debugfs_remove_recursive (q->debugfs_dir); +if (IS_ENABLED (CONFIG_CHR_DEV_SG )) +debugfs_remove_recursive (q->sg_debugfs_dir);if (queue_is_mq (q))blk_mq_debugfs_unregister (q);
@@ -937,6 +941,20 @@ struct kobj_type blk_queue_ktype = { .release = blk_release_queue, }; +/** +* blk_sg_debugfs_init - initialize debugfs for scsi-generic +* @q: the associated queue +* @name: name of the scsi-generic device +* +* To be used by scsi-generic for allowing it to use blktrace. +*/ +void blk_sg_debugfs_init (struct request_queue *q,const char *name) +{ +if (IS_ENABLED (CONFIG_CHR_DEV_SG )) + q->sg_debugfs_dir =debugfs_create_dir (name, blk_debugfs_root); +} +EXPORT_SYMBOL_GPL (blk_sg_debugfs_init); +/** * blk_register_queue - register a block layer queue with sysfs * @disk: Disk of which the request queue should be registered with sysfs.
@@ -989,6 +1007,28 @@ int blk_register_queue(struct gendisk *disk) goto unlock; } +/* +* Blktrace needs a debugfs name even for queues that don't register +* a gendisk, so it lazily registers the debugfs directory. But that +* can get us into a situation where a SCSI device is found, with no +* driver for it (yet). Then blktrace is used on the device, creating +* the debugfs directory, and only after that a driver is loaded. In +* that case we might already have a debugfs directory registered here. +* Even worse we could be racing with blktrace to register it. +*/ +#ifdef CONFIG_BLK_DEV_IO_TRACE +mutex_lock (&q->blk_trace_mutex); +if (!q->debugfs_dir) { + q->debugfs_dir = +debugfs_create_dir (kobject_name (q->kobj.parent), + blk_debugfs_root); + } +mutex_unlock (&q->blk_trace_mutex); +#else + q->debugfs_dir =debugfs_create_dir (kobject_name (q->kobj.parent), + blk_debugfs_root); +#endif +if (queue_is_mq (q)) {__blk_mq_register_dev (dev, q);blk_mq_debugfs_register (q);
block/blk.h +0 -2
@@ -14,9 +14,7 @@/* Max future timer expiry for timeouts */ #define BLK_MAX_TIMEOUT (5 * HZ) -#ifdef CONFIG_DEBUG_FS extern struct dentry *blk_debugfs_root; -#endif struct blk_flush_queue {unsigned int flush_pending_idx:1 ;
block/partitions/core.c +3 -0
@@ -10,6 +10,7 @@#include <linux/vmalloc.h> #include <linux/blktrace_api.h> #include <linux/raid/detect.h> +#include <linux/debugfs.h> #include "check.h" static int (*check_part[])(struct parsed_partitions *) = {
@@ -322,6 +323,7 @@ void delete_partition(struct gendisk *disk, struct hd_struct *part)get_device (disk_to_dev (part_to_disk (part)));rcu_assign_pointer (ptbl->part[part->partno],NULL );kobject_put (part->holder_dir); +debugfs_remove_recursive (part->debugfs_dir);device_del (part_to_dev (part));/*
@@ -444,6 +446,7 @@ static struct hd_struct *add_partition(struct gendisk *disk, int partno,if (!p->holder_dir) goto out_del; + p->debugfs_dir =debugfs_create_dir (dev_name (pdev), blk_debugfs_root);dev_set_uevent_suppress (pdev,0 );if (flags &ADDPART_FLAG_WHOLEDISK ) { err =device_create_file (pdev, &dev_attr_whole_disk);
drivers/scsi/sg.c +3 -0
@@ -1117,6 +1117,9 @@ sg_ioctl_common(struct file *filp, Sg_device *sdp, Sg_fd *sfp,return put_user (max_sectors_bytes (sdp->device->request_queue), ip);case BLKTRACESETUP : +if (!sdp->device->request_queue->sg_debugfs_dir) +blk_sg_debugfs_init (sdp->device->request_queue, + sdp->disk->disk_name); return blk_trace_setup(sdp->device->request_queue, sdp->disk->disk_name, MKDEV(SCSI_GENERIC_MAJOR, sdp->index),
include/linux/blkdev.h +3 -1
@@ -574,8 +574,9 @@ struct request_queue {struct list_head tag_set_list;struct bio_set bio_split; -#ifdef CONFIG_BLK_DEBUG_FS struct dentry *debugfs_dir; +struct dentry *sg_debugfs_dir; +#ifdef CONFIG_BLK_DEBUG_FS struct dentry *sched_debugfs_dir;struct dentry *rqos_debugfs_dir;#endif
@@ -858,6 +859,7 @@ static inline void rq_flush_dcache_pages(struct request *rq)extern int blk_register_queue (struct gendisk *disk);extern void blk_unregister_queue (struct gendisk *disk); +extern void blk_sg_debugfs_init (struct request_queue *q,const char *name);extern blk_qc_t generic_make_request (struct bio *bio);extern blk_qc_t direct_make_request (struct bio *bio);extern void blk_rq_init (struct request_queue *q,struct request *rq);
include/linux/blktrace_api.h +0 -1
@@ -22,7 +22,6 @@ struct blk_trace {u64 end_lba;u32 pid;u32 dev; -struct dentry *dir;struct dentry *dropped_file;struct dentry *msg_file;struct list_head running_list;
include/linux/genhd.h +1 -0
@@ -76,6 +76,7 @@ struct hd_struct {int make_it_fail;#endif struct rcu_work rcu_work; +struct dentry *debugfs_dir; };/**
kernel/trace/blktrace.c +41 -12
83 lines not on this page · raw message
Signature · 1 line
2.26.2
Re: [PATCH v6 6/6] blktrace: fix debugfs use after free
All the complicated code to deal with delayed creation of the debugfs directory was only needed for sg. With sg handled speparately that can go away. Also I think the sg handling can be cleaned up by:
a) checking for a NULL block_devic instead of the major, especially as majors can be used independently for character and block devices
b) just keeping the old method of creating the entry on demand for the sg devices, as no one shares the debugfs directory in that case.
Untested patch on top of your series below:
block/blk-sysfs.c +0 -35
@@ -920,8 +920,6 @@ static void blk_release_queue(struct kobject *kobj)blk_trace_shutdown (q);debugfs_remove_recursive (q->debugfs_dir); -if (IS_ENABLED (CONFIG_CHR_DEV_SG )) -debugfs_remove_recursive (q->sg_debugfs_dir);if (queue_is_mq (q))blk_mq_debugfs_unregister (q);
@@ -941,20 +939,6 @@ struct kobj_type blk_queue_ktype = { .release = blk_release_queue, }; -/** -* blk_sg_debugfs_init - initialize debugfs for scsi-generic -* @q: the associated queue -* @name: name of the scsi-generic device -* -* To be used by scsi-generic for allowing it to use blktrace. -*/ -void blk_sg_debugfs_init (struct request_queue *q,const char *name) -{ -if (IS_ENABLED (CONFIG_CHR_DEV_SG )) - q->sg_debugfs_dir =debugfs_create_dir (name, blk_debugfs_root); -} -EXPORT_SYMBOL_GPL (blk_sg_debugfs_init); -/** * blk_register_queue - register a block layer queue with sysfs * @disk: Disk of which the request queue should be registered with sysfs.
@@ -1007,27 +991,8 @@ int blk_register_queue(struct gendisk *disk) goto unlock; } -/* -* Blktrace needs a debugfs name even for queues that don't register -* a gendisk, so it lazily registers the debugfs directory. But that -* can get us into a situation where a SCSI device is found, with no -* driver for it (yet). Then blktrace is used on the device, creating -* the debugfs directory, and only after that a driver is loaded. In -* that case we might already have a debugfs directory registered here. -* Even worse we could be racing with blktrace to register it. -*/ -#ifdef CONFIG_BLK_DEV_IO_TRACE -mutex_lock (&q->blk_trace_mutex); -if (!q->debugfs_dir) { - q->debugfs_dir = -debugfs_create_dir (kobject_name (q->kobj.parent), - blk_debugfs_root); - } -mutex_unlock (&q->blk_trace_mutex); -#else q->debugfs_dir =debugfs_create_dir (kobject_name (q->kobj.parent), blk_debugfs_root); -#endif if (queue_is_mq (q)) {__blk_mq_register_dev (dev, q);
drivers/scsi/sg.c +0 -3
@@ -1117,9 +1117,6 @@ sg_ioctl_common(struct file *filp, Sg_device *sdp, Sg_fd *sfp,return put_user (max_sectors_bytes (sdp->device->request_queue), ip);case BLKTRACESETUP : -if (!sdp->device->request_queue->sg_debugfs_dir) -blk_sg_debugfs_init (sdp->device->request_queue, - sdp->disk->disk_name); return blk_trace_setup(sdp->device->request_queue, sdp->disk->disk_name, MKDEV(SCSI_GENERIC_MAJOR, sdp->index),
include/linux/blkdev.h +0 -2
@@ -575,7 +575,6 @@ struct request_queue {struct bio_set bio_split;struct dentry *debugfs_dir; -struct dentry *sg_debugfs_dir;#ifdef CONFIG_BLK_DEBUG_FS struct dentry *sched_debugfs_dir;struct dentry *rqos_debugfs_dir;
@@ -859,7 +858,6 @@ static inline void rq_flush_dcache_pages(struct request *rq)extern int blk_register_queue (struct gendisk *disk);extern void blk_unregister_queue (struct gendisk *disk); -extern void blk_sg_debugfs_init (struct request_queue *q,const char *name);extern blk_qc_t generic_make_request (struct bio *bio);extern blk_qc_t direct_make_request (struct bio *bio);extern void blk_rq_init (struct request_queue *q,struct request *rq);
include/linux/blktrace_api.h +1 -0
@@ -22,6 +22,7 @@ struct blk_trace {u64 end_lba;u32 pid;u32 dev; +struct dentry *dir;struct dentry *dropped_file;struct dentry *msg_file;struct list_head running_list;
kernel/trace/blktrace.c +17 -29
69 lines not on this page · raw message
Re: [PATCH v6 6/6] blktrace: fix debugfs use after free
I like this, more below.
Quoted 20 lines · Christoph Hellwig
kernel/trace/blktrace.c +4
-8@@ -492,34 +493,23 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,*/ strreplace (buts->name,'/' ,'_' ); -/* -* We have to use a partition directory if a partition is being worked -* on. The same request_queue is shared between all partitions. -*/ -if (bdev && bdev != bdev->bd_contains) { - dir = bdev->bd_part->debugfs_dir; - }else if (IS_ENABLED (CONFIG_CHR_DEV_SG ) && -MAJOR (dev) ==SCSI_GENERIC_MAJOR ) { + bt =kzalloc (sizeof (*bt),GFP_KERNEL ); +if (!bt) +return -ENOMEM ; +
kernel/trace/blktrace.c static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev, · line 504
Quoted 10 lines
kernel/trace/blktrace.c +4
-5+if (unlikely (!bdev)) {/* -* scsi-generic exposes the request_queue through the /dev/sg* -* interface but since that uses a different path than whatever -* the respective scsi driver device name may expose and use -* for the request_queue debugfs_dir. We have a dedicated -* dentry for scsi-generic then. +* When tracing something that is not a block device (e.g. the +* /dev/sg nodes), create debugfs directory on demand. This +* directory will be remove when stopping the trace.
Is scsi-generic is the only unwanted ugly child blktrace has to deal with? For some reason I thought drivers/md/md.c was one but it seems like it is not. Do we have an easy way to search for these? I think this would just affect how we express the comment only.
kernel/trace/blktrace.c static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev, · line 507
Quoted 4 lines
kernel/trace/blktrace.c +2
-1*/ - dir = q->sg_debugfs_dir; + dir =debugfs_create_dir (buts->name, blk_debugfs_root); + bt->dir = dir;
The other chicken and egg problem to consider at least in the comments is that the debugfs directory for these types of devices *have* an exposed path, but the data structure is rather opaque to the device and even blktrace. Fortunately given the recent set of changes around the q->blk_trace and clarifications around its use we have made it clear now that so long as hold the q->blk_trace_mutex *and* check q->blk_trace we *should* not race against two separate creations of debugfs directories, so I think this is safe, so long as these indpendent drivers don't end up re-using the same path for some other things later in the future, and since we have control over what goes under debugfsroot block / I think we should be good.
But I think that the concern for race on names may still be worth explaining a bit here.
Luis
Re: [PATCH v6 6/6] blktrace: fix debugfs use after free
Quoted 4 lines · Luis Chamberlain
Is scsi-generic is the only unwanted ugly child blktrace has to deal with? For some reason I thought
drivers/md/md.cwas one but it seems like it is not. Do we have an easy way to search for these? I think this would just affect how we express the comment only.
grep for blk_trace_setup. For all blk devices that setup comes in through the block device ioctl path, and that relies on having a struct block_device and queue. sg on the other hand calls blk_trace_setup directly with a NULL bdev argument.
Quoted 17 lines
kernel/trace/blktrace.c static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev, · line 507
Quoted 4 lines
kernel/trace/blktrace.c +2
-1*/ - dir = q->sg_debugfs_dir; + dir =debugfs_create_dir (buts->name, blk_debugfs_root); + bt->dir = dir;The other chicken and egg problem to consider at least in the comments is that the debugfs directory for these types of devices *have* an exposed path, but the data structure is rather opaque to the device and even blktrace. Fortunately given the recent set of changes around the
q->blk_traceand clarifications around its use we have made it clear now that so long as hold theq->blk_trace_mutex*and* checkq->blk_tracewe *should* not race against two separate creations of debugfs directories, so I think this is safe, so long as these indpendent drivers don't end up re-using the same path for some other things later in the future, and since we have control over what goes under debugfsroot block / I think we should be good.
Quoted 2 lines
But I think that the concern for race on names may still be worth explaining a bit here.
Feel free to add more comments, but please try to keep them short and crisp. At the some point long comments really distract from what is going on.
Re: [PATCH v6 6/6] blktrace: fix debugfs use after free
Quoted 10 lines · Christoph Hellwig
Quoted 4 lines · Luis Chamberlain
Is scsi-generic is the only unwanted ugly child blktrace has to deal with? For some reason I thought
drivers/md/md.cwas one but it seems like it is not. Do we have an easy way to search for these? I think this would just affect how we express the comment only.grep for
blk_trace_setup. For all blk devices that setup comes in through the block device ioctl path, and that relies on having a structblock_deviceand queue. sg on the other hand callsblk_trace_setupdirectly with a NULL bdev argument.
Alright, then we should be good.
Quoted 17 lines
Quoted 17 lines
kernel/trace/blktrace.c static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev, · line 507
Quoted 4 lines
kernel/trace/blktrace.c +2
-1*/ - dir = q->sg_debugfs_dir; + dir =debugfs_create_dir (buts->name, blk_debugfs_root); + bt->dir = dir;The other chicken and egg problem to consider at least in the comments is that the debugfs directory for these types of devices *have* an exposed path, but the data structure is rather opaque to the device and even blktrace. Fortunately given the recent set of changes around the
q->blk_traceand clarifications around its use we have made it clear now that so long as hold theq->blk_trace_mutex*and* checkq->blk_tracewe *should* not race against two separate creations of debugfs directories, so I think this is safe, so long as these indpendent drivers don't end up re-using the same path for some other things later in the future, and since we have control over what goes under debugfsroot block / I think we should be good.
Quoted 6 lines
Quoted 2 lines
But I think that the concern for race on names may still be worth explaining a bit here.
Feel free to add more comments, but please try to keep them short and crisp. At the some point long comments really distract from what is going on.
Sure.
Come to think of it, given the above, I think we can also do way with the the partition stuff too, and rely on the buts->name too. I'll try this out, and test it.
Luis
Re: [PATCH v6 6/6] blktrace: fix debugfs use after free
Quoted 9 lines · Luis Chamberlain
Quoted 3 lines
Feel free to add more comments, but please try to keep them short and crisp. At the some point long comments really distract from what is going on.
Sure.
Come to think of it, given the above, I think we can also do way with the the partition stuff too, and rely on the
buts->nametoo. I'll try this out, and test it.
Yes, the sg path should work for partitions as well. That should not only simplify the code, but also the comments, we can do something like the full patch below (incorporating your original one). This doesn't include the error check on the creation - I think that check probably is a good idea for this case based on the comments in the old patch, but also a separate issue that should go into another patch on top.
block/blk-mq-debugfs.c +0 -5
@@ -824,9 +824,6 @@ void blk_mq_debugfs_register(struct request_queue *q)struct blk_mq_hw_ctx *hctx;int i; - q->debugfs_dir =debugfs_create_dir (kobject_name (q->kobj.parent), - blk_debugfs_root); -debugfs_create_files (q->debugfs_dir, q, blk_mq_debugfs_queue_attrs);/*
@@ -857,9 +854,7 @@ void blk_mq_debugfs_register(struct request_queue *q)void blk_mq_debugfs_unregister (struct request_queue *q) { -debugfs_remove_recursive (q->debugfs_dir); q->sched_debugfs_dir =NULL ; - q->debugfs_dir =NULL ; } static void blk_mq_debugfs_register_ctx(struct blk_mq_hw_ctx *hctx,
block/blk-sysfs.c +5 -0
@@ -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"
@@ -918,6 +919,7 @@ static void blk_release_queue(struct kobject *kobj)blk_trace_shutdown (q); +debugfs_remove_recursive (q->debugfs_dir);if (queue_is_mq (q))blk_mq_debugfs_unregister (q);
@@ -989,6 +991,9 @@ int blk_register_queue(struct gendisk *disk) goto unlock; } + q->debugfs_dir =debugfs_create_dir (kobject_name (q->kobj.parent), + blk_debugfs_root); +if (queue_is_mq (q)) {__blk_mq_register_dev (dev, q);blk_mq_debugfs_register (q);
block/blk.h +0 -2
@@ -14,9 +14,7 @@/* Max future timer expiry for timeouts */ #define BLK_MAX_TIMEOUT (5 * HZ) -#ifdef CONFIG_DEBUG_FS extern struct dentry *blk_debugfs_root; -#endif struct blk_flush_queue {unsigned int flush_pending_idx:1 ;
include/linux/blkdev.h +1 -1
@@ -574,8 +574,8 @@ struct request_queue {struct list_head tag_set_list;struct bio_set bio_split; -#ifdef CONFIG_BLK_DEBUG_FS struct dentry *debugfs_dir; +#ifdef CONFIG_BLK_DEBUG_FS struct dentry *sched_debugfs_dir;struct dentry *rqos_debugfs_dir;#endif
kernel/trace/blktrace.c +9 -6
@@ -509,10 +509,15 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,if (!bt->msg_data) goto err; - ret = -ENOENT ; - - dir =debugfs_lookup (buts->name, blk_debugfs_root); -if (!dir) +/* +* When tracing a whole block device, reuse the existing debugfs +* directory created by the block layer. For partitions or character +* devices (e.g. /dev/sg), create a new debugfs directory that will be +* removed once the trace ends. +*/ +if (bdev && bdev == bdev->bd_contains) + dir = q->debugfs_dir; +else bt->dir = dir =debugfs_create_dir (buts->name, blk_debugfs_root); bt->dev = dev;
@@ -553,8 +558,6 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev, ret =0 ; err: -if (dir && !bt->dir) -dput (dir);if (ret)blk_trace_free (bt);return ret;
Re: [PATCH v6 6/6] blktrace: fix debugfs use after free
Quoted 7 lines · Christoph Hellwig
Quoted 6 lines · Luis Chamberlain
Quoted 3 lines
Feel free to add more comments, but please try to keep them short and crisp. At the some point long comments really distract from what is going on.
Sure.
Quoted 7 lines
Quoted 3 lines
Come to think of it, given the above, I think we can also do way with the the partition stuff too, and rely on the
buts->nametoo. I'll try this out, and test it.Yes, the sg path should work for partitions as well. That should not only simplify the code, but also the comments, we can do something like the full patch below (incorporating your original one).
Indeed I ended up with something similar, will use this variation.
Quoted 4 lines
This doesn't
include the error check on the creation - I think that check probably is a good idea for this case based on the comments in the old patch, but also a separate issue that should go into another patch on top.
Makes sense.
Luis
Re: [PATCH v6 6/6] blktrace: fix debugfs use after free
So, upon updating the commit log, and moving the empty directory check into another patch, I realized we might be able to simplify this now even further still. Patch below. The key of the issue was that the use after free happens when a recursive removal happens, and then later a specific dentry removal happens. This happened for make_request block drivers when using the whole disk, but since we *don't* have any other users of the directory for the others cases, this in theory shuld not happen for them either.
I'll try to shoot some bullets at this.
kernel/trace/blktrace.c +10 -6
@@ -524,10 +524,16 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,if (!bt->msg_data) goto err; - ret = -ENOENT ; - - dir =debugfs_lookup (buts->name, blk_debugfs_root); -if (!dir) +/* +* When tracing whole make_request drivers (multiqueue) block devices, +* reuse the existing debugfs directory created by the block layer on +* init. For request-based block devices, all partitions block devices, +* and scsi-generic block devices we create a temporary new debugfs +* directory that will be removed once the trace ends. +*/ +if (queue_is_mq (q)) + dir = q->debugfs_dir; +else bt->dir = dir =debugfs_create_dir (buts->name, blk_debugfs_root); bt->dev = dev;
@@ -565,8 +571,6 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev, ret =0 ; err: -if (dir && !bt->dir) -dput (dir);if (ret)blk_trace_free (bt);return ret;
Re: [PATCH v6 6/6] blktrace: fix debugfs use after free
Quoted 10 lines · Luis Chamberlain
kernel/trace/blktrace.c +0
-2@@ -524,10 +524,16 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,if (!bt->msg_data) goto err; - ret = -ENOENT ; -
kernel/trace/blktrace.c static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev, · line 534
Quoted 10 lines
kernel/trace/blktrace.c +8
-2- dir =debugfs_lookup (buts->name, blk_debugfs_root); -if (!dir) +/* +* When tracing whole make_request drivers (multiqueue) block devices, +* reuse the existing debugfs directory created by the block layer on +* init. For request-based block devices, all partitions block devices, +* and scsi-generic block devices we create a temporary new debugfs +* directory that will be removed once the trace ends. +*/ +if (queue_is_mq (q))
And this should be instead:
if (queue_is_mq(q) && bdev && bdev == bdev->bd_contains)
Quoted 14 lines
kernel/trace/blktrace.c +2
-2+ dir = q->debugfs_dir; + else bt->dir = dir = debugfs_create_dir(buts->name, blk_debugfs_root); bt->dev = dev;@@ -565,8 +571,6 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev, ret =0 ; err: -if (dir && !bt->dir) -dput (dir);if (ret)blk_trace_free (bt);return ret;
Re: [PATCH v6 6/6] blktrace: fix debugfs use after free
Quoted 24 lines · Luis Chamberlain
Quoted 10 lines · Luis Chamberlain
kernel/trace/blktrace.c +0
-2@@ -524,10 +524,16 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,if (!bt->msg_data) goto err; - ret = -ENOENT ; -kernel/trace/blktrace.c static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev, · line 534
Quoted 10 lines
kernel/trace/blktrace.c +8
-2- dir =debugfs_lookup (buts->name, blk_debugfs_root); -if (!dir) +/* +* When tracing whole make_request drivers (multiqueue) block devices, +* reuse the existing debugfs directory created by the block layer on +* init. For request-based block devices, all partitions block devices, +* and scsi-generic block devices we create a temporary new debugfs +* directory that will be removed once the trace ends. +*/ +if (queue_is_mq (q))And this should be instead:
Quoted 1 line
if (
queue_is_mq(q)&& bdev && bdev ==bdev->bd_contains)
Yes. But I think keeping the queue_is_mq out and always creating the debugfs dir is an improvement as we'll sooner or later grow more debugfs files and than the whole set of problmes reappears. But I'd be fine with doing the above in the first patch that is tiny and backportable, and then have another patch that always creates the debugfs directory.
Re: [PATCH v6 6/6] blktrace: fix debugfs use after free
In reply to Luis Chamberlainblock/blk-sysfs.c int blk_register_queue(struct gendisk *disk) · line 1018
Quoted 9 lines · Luis Chamberlain
block/blk-sysfs.c +9
-0+/* +* Blktrace needs a debugfs name even for queues that don't register +* a gendisk, so it lazily registers the debugfs directory. But that +* can get us into a situation where a SCSI device is found, with no +* driver for it (yet). Then blktrace is used on the device, creating +* the debugfs directory, and only after that a driver is loaded. In +* that case we might already have a debugfs directory registered here. +* Even worse we could be racing with blktrace to register it. +*/
There are LLD and ULD drivers in the SCSI subsystem. Please mention the driver type explicitly. I assume that you are referring to SCSI ULDs since only SCSI ULD drivers call device_add_disk()?
Could the above comment be made shorter by only mentioning that blktrace may have been set up before or concurrently with device_add_disk() and that device_add_disk() calls blk_register_queue()?
drivers/scsi/sg.c sg_ioctl_common(struct file *filp, Sg_device *sdp, Sg_fd *sfp, · line 1122
Quoted 4 lines
drivers/scsi/sg.c +3
-0case BLKTRACESETUP : +if (!sdp->device->request_queue->sg_debugfs_dir) +blk_sg_debugfs_init (sdp->device->request_queue, + sdp->disk->disk_name);
How about moving the sg_debugfs_dir check into blk_sg_debugfs_init()?
Thanks,
Bart.
Re: [PATCH v6 6/6] blktrace: fix debugfs use after free
Quoted 14 lines · Bart Van Assche
block/blk-sysfs.c int blk_register_queue(struct gendisk *disk) · line 1018
Quoted 9 lines · Luis Chamberlain
block/blk-sysfs.c +9
-0+/* +* Blktrace needs a debugfs name even for queues that don't register +* a gendisk, so it lazily registers the debugfs directory. But that +* can get us into a situation where a SCSI device is found, with no +* driver for it (yet). Then blktrace is used on the device, creating +* the debugfs directory, and only after that a driver is loaded. In +* that case we might already have a debugfs directory registered here. +* Even worse we could be racing with blktrace to register it. +*/ There are LLD and ULD drivers in the SCSI subsystem. Please mention the driver type explicitly. I assume that you are referring to SCSI ULDs since only SCSI ULD drivers call
device_add_disk()?
I've simplified this and so this is no longer a valid comment.
Quoted 6 lines
drivers/scsi/sg.c sg_ioctl_common(struct file *filp, Sg_device *sdp, Sg_fd *sfp, · line 1122
Quoted 4 lines
drivers/scsi/sg.c +3
-0case BLKTRACESETUP : +if (!sdp->device->request_queue->sg_debugfs_dir) +blk_sg_debugfs_init (sdp->device->request_queue, + sdp->disk->disk_name);How about moving the
sg_debugfs_dircheck intoblk_sg_debugfs_init()?
I found a way to not have to do any of this, the fix will be short and sweet now.
Luis