| From 9f53d2fe815b4011ff930a7b6db98385d45faa68 Mon Sep 17 00:00:00 2001 |
| From: Stanislaw Gruszka <sgruszka@redhat.com> |
| Date: Fri, 2 Mar 2012 10:43:28 +0100 |
| Subject: block: fix __blkdev_get and add_disk race condition |
| |
| From: Stanislaw Gruszka <sgruszka@redhat.com> |
| |
| commit 9f53d2fe815b4011ff930a7b6db98385d45faa68 upstream. |
| |
| The following situation might occur: |
| |
| __blkdev_get: add_disk: |
| |
| register_disk() |
| get_gendisk() |
| |
| disk_block_events() |
| disk->ev == NULL |
| |
| disk_add_events() |
| |
| __disk_unblock_events() |
| disk->ev != NULL |
| --ev->block |
| |
| Then we unblock events, when they are suppose to be blocked. This can |
| trigger events related block/genhd.c warnings, but also can crash in |
| sd_check_events() or other places. |
| |
| I'm able to reproduce crashes with the following scripts (with |
| connected usb dongle as sdb disk). |
| |
| <snip> |
| DEV=/dev/sdb |
| ENABLE=/sys/bus/usb/devices/1-2/bConfigurationValue |
| |
| function stop_me() |
| { |
| for i in `jobs -p` ; do kill $i 2> /dev/null ; done |
| exit |
| } |
| |
| trap stop_me SIGHUP SIGINT SIGTERM |
| |
| for ((i = 0; i < 10; i++)) ; do |
| while true; do fdisk -l $DEV 2>&1 > /dev/null ; done & |
| done |
| |
| while true ; do |
| echo 1 > $ENABLE |
| sleep 1 |
| echo 0 > $ENABLE |
| done |
| </snip> |
| |
| I use the script to verify patch fixing oops in sd_revalidate_disk |
| http://marc.info/?l=linux-scsi&m=132935572512352&w=2 |
| Without Jun'ichi Nomura patch titled "Fix NULL pointer dereference in |
| sd_revalidate_disk" or this one, script easily crash kernel within |
| a few seconds. With both patches applied I do not observe crash. |
| Unfortunately after some time (dozen of minutes), script will hung in: |
| |
| [ 1563.906432] [<c08354f5>] schedule_timeout_uninterruptible+0x15/0x20 |
| [ 1563.906437] [<c04532d5>] msleep+0x15/0x20 |
| [ 1563.906443] [<c05d60b2>] blk_drain_queue+0x32/0xd0 |
| [ 1563.906447] [<c05d6e00>] blk_cleanup_queue+0xd0/0x170 |
| [ 1563.906454] [<c06d278f>] scsi_free_queue+0x3f/0x60 |
| [ 1563.906459] [<c06d7e6e>] __scsi_remove_device+0x6e/0xb0 |
| [ 1563.906463] [<c06d4aff>] scsi_forget_host+0x4f/0x60 |
| [ 1563.906468] [<c06cd84a>] scsi_remove_host+0x5a/0xf0 |
| [ 1563.906482] [<f7f030fb>] quiesce_and_remove_host+0x5b/0xa0 [usb_storage] |
| [ 1563.906490] [<f7f03203>] usb_stor_disconnect+0x13/0x20 [usb_storage] |
| |
| Anyway I think this patch is some step forward. |
| |
| As drawback, I do not teardown on sysfs file create error, because I do |
| not know how to nullify disk->ev (since it can be used). However add_disk |
| error handling practically does not exist too, and things will work |
| without this sysfs file, except events will not be exported to user |
| space. |
| |
| Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> |
| Acked-by: Tejun Heo <tj@kernel.org> |
| Signed-off-by: Jens Axboe <axboe@kernel.dk> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| block/genhd.c | 32 +++++++++++++++++++------------- |
| 1 file changed, 19 insertions(+), 13 deletions(-) |
| |
| --- a/block/genhd.c |
| +++ b/block/genhd.c |
| @@ -36,6 +36,7 @@ static DEFINE_IDR(ext_devt_idr); |
| |
| static struct device_type disk_type; |
| |
| +static void disk_alloc_events(struct gendisk *disk); |
| static void disk_add_events(struct gendisk *disk); |
| static void disk_del_events(struct gendisk *disk); |
| static void disk_release_events(struct gendisk *disk); |
| @@ -602,6 +603,8 @@ void add_disk(struct gendisk *disk) |
| disk->major = MAJOR(devt); |
| disk->first_minor = MINOR(devt); |
| |
| + disk_alloc_events(disk); |
| + |
| /* Register BDI before referencing it from bdev */ |
| bdi = &disk->queue->backing_dev_info; |
| bdi_register_dev(bdi, disk_devt(disk)); |
| @@ -1734,9 +1737,9 @@ module_param_cb(events_dfl_poll_msecs, & |
| &disk_events_dfl_poll_msecs, 0644); |
| |
| /* |
| - * disk_{add|del|release}_events - initialize and destroy disk_events. |
| + * disk_{alloc|add|del|release}_events - initialize and destroy disk_events. |
| */ |
| -static void disk_add_events(struct gendisk *disk) |
| +static void disk_alloc_events(struct gendisk *disk) |
| { |
| struct disk_events *ev; |
| |
| @@ -1749,16 +1752,6 @@ static void disk_add_events(struct gendi |
| return; |
| } |
| |
| - if (sysfs_create_files(&disk_to_dev(disk)->kobj, |
| - disk_events_attrs) < 0) { |
| - pr_warn("%s: failed to create sysfs files for events\n", |
| - disk->disk_name); |
| - kfree(ev); |
| - return; |
| - } |
| - |
| - disk->ev = ev; |
| - |
| INIT_LIST_HEAD(&ev->node); |
| ev->disk = disk; |
| spin_lock_init(&ev->lock); |
| @@ -1767,8 +1760,21 @@ static void disk_add_events(struct gendi |
| ev->poll_msecs = -1; |
| INIT_DELAYED_WORK(&ev->dwork, disk_events_workfn); |
| |
| + disk->ev = ev; |
| +} |
| + |
| +static void disk_add_events(struct gendisk *disk) |
| +{ |
| + if (!disk->ev) |
| + return; |
| + |
| + /* FIXME: error handling */ |
| + if (sysfs_create_files(&disk_to_dev(disk)->kobj, disk_events_attrs) < 0) |
| + pr_warn("%s: failed to create sysfs files for events\n", |
| + disk->disk_name); |
| + |
| mutex_lock(&disk_events_mutex); |
| - list_add_tail(&ev->node, &disk_events); |
| + list_add_tail(&disk->ev->node, &disk_events); |
| mutex_unlock(&disk_events_mutex); |
| |
| /* |