[S390] dasd: fix race between open and offline

The dasd_open function uses the private_data pointer of the gendisk to
find the dasd_block structure that matches the gendisk. When a DASD
device is set offline, we set the private_data pointer of the gendisk
to NULL and later remove the dasd_block structure, but there is still
a small race window, in which dasd_open could first read a pointer
from the private_data field and then try to use it, after the structure
has already been freed.
To close this race window, we will store a pointer to the dasd_devmap
structure of the base device in the private_data field. The devmap
entries are not deleted, and we already have proper locking and
reference counting in place, so that we can safely get from a devmap
pointer to the dasd_device and dasd_block structures of the device.

Signed-off-by: Stefan Weinhuber <wein@de.ibm.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 4d2df2f76ea0daa718215c179e88fe2b45249516..475e603fc584f8547e6171eaf8cdb325ec085d1f 100644 (file)
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -2314,15 +2314,14 @@ static void dasd_flush_request_queue(struct dasd_block *block)
 
 static int dasd_open(struct block_device *bdev, fmode_t mode)
 {
-       struct dasd_block *block = bdev->bd_disk->private_data;
        struct dasd_device *base;
        int rc;
 
-       if (!block)
+       base = dasd_device_from_gendisk(bdev->bd_disk);
+       if (!base)
                return -ENODEV;
 
-       base = block->base;
-       atomic_inc(&block->open_count);
+       atomic_inc(&base->block->open_count);
        if (test_bit(DASD_FLAG_OFFLINE, &base->flags)) {
                rc = -ENODEV;
                goto unlock;
@@ -2355,21 +2354,28 @@ static int dasd_open(struct block_device *bdev, fmode_t mode)
                goto out;
        }
 
+       dasd_put_device(base);
        return 0;
 
 out:
        module_put(base->discipline->owner);
 unlock:
-       atomic_dec(&block->open_count);
+       atomic_dec(&base->block->open_count);
+       dasd_put_device(base);
        return rc;
 }
 
 static int dasd_release(struct gendisk *disk, fmode_t mode)
 {
-       struct dasd_block *block = disk->private_data;
+       struct dasd_device *base;
 
-       atomic_dec(&block->open_count);
-       module_put(block->base->discipline->owner);
+       base = dasd_device_from_gendisk(disk);
+       if (!base)
+               return -ENODEV;
+
+       atomic_dec(&base->block->open_count);
+       module_put(base->discipline->owner);
+       dasd_put_device(base);
        return 0;
 }
 
@@ -2378,20 +2384,20 @@ static int dasd_release(struct gendisk *disk, fmode_t mode)
  */
 static int dasd_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 {
-       struct dasd_block *block;
        struct dasd_device *base;
 
-       block = bdev->bd_disk->private_data;
-       if (!block)
+       base = dasd_device_from_gendisk(bdev->bd_disk);
+       if (!base)
                return -ENODEV;
-       base = block->base;
 
        if (!base->discipline ||
-           !base->discipline->fill_geometry)
+           !base->discipline->fill_geometry) {
+               dasd_put_device(base);
                return -EINVAL;
-
-       base->discipline->fill_geometry(block, geo);
-       geo->start = get_start_sect(bdev) >> block->s2b_shift;
+       }
+       base->discipline->fill_geometry(base->block, geo);
+       geo->start = get_start_sect(bdev) >> base->block->s2b_shift;
+       dasd_put_device(base);
        return 0;
 }
 
@@ -2528,7 +2534,6 @@ void dasd_generic_remove(struct ccw_device *cdev)
        dasd_set_target_state(device, DASD_STATE_NEW);
        /* dasd_delete_device destroys the device reference. */
        block = device->block;
-       device->block = NULL;
        dasd_delete_device(device);
        /*
         * life cycle of block is bound to device, so delete it after
@@ -2650,7 +2655,6 @@ int dasd_generic_set_offline(struct ccw_device *cdev)
        dasd_set_target_state(device, DASD_STATE_NEW);
        /* dasd_delete_device destroys the device reference. */
        block = device->block;
-       device->block = NULL;
        dasd_delete_device(device);
        /*
         * life cycle of block is bound to device, so delete it after

diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
index 42e1bf35f6893d53e3374eba20673b1d27d095f7..d71511c7850a8560014ceee03c468e4da24c361a 100644 (file)
--- a/drivers/s390/block/dasd_devmap.c
+++ b/drivers/s390/block/dasd_devmap.c
@@ -674,6 +674,36 @@ dasd_device_from_cdev(struct ccw_device *cdev)
        return device;
 }
 
+void dasd_add_link_to_gendisk(struct gendisk *gdp, struct dasd_device *device)
+{
+       struct dasd_devmap *devmap;
+
+       devmap = dasd_find_busid(dev_name(&device->cdev->dev));
+       if (IS_ERR(devmap))
+               return;
+       spin_lock(&dasd_devmap_lock);
+       gdp->private_data = devmap;
+       spin_unlock(&dasd_devmap_lock);
+}
+
+struct dasd_device *dasd_device_from_gendisk(struct gendisk *gdp)
+{
+       struct dasd_device *device;
+       struct dasd_devmap *devmap;
+
+       if (!gdp->private_data)
+               return NULL;
+       device = NULL;
+       spin_lock(&dasd_devmap_lock);
+       devmap = gdp->private_data;
+       if (devmap && devmap->device) {
+               device = devmap->device;
+               dasd_get_device(device);
+       }
+       spin_unlock(&dasd_devmap_lock);
+       return device;
+}
+
 /*
  * SECTION: files in sysfs
  */

diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
index 5505bc07e1e7cccccb84549943d89b772533d199..19a1ff03d65ef25c5320585c04064916827a41bb 100644 (file)
--- a/drivers/s390/block/dasd_genhd.c
+++ b/drivers/s390/block/dasd_genhd.c
@@ -73,7 +73,7 @@ int dasd_gendisk_alloc(struct dasd_block *block)
        if (base->features & DASD_FEATURE_READONLY ||
            test_bit(DASD_FLAG_DEVICE_RO, &base->flags))
                set_disk_ro(gdp, 1);
-       gdp->private_data = block;
+       dasd_add_link_to_gendisk(gdp, base);
        gdp->queue = block->request_queue;
        block->gdp = gdp;
        set_capacity(block->gdp, 0);

diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index df9f6999411def84414938e65df42d699b33bfcc..d1e4f2c1264c54225a70307b0830343a5e5c5f0c 100644 (file)
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -686,6 +686,9 @@ struct dasd_device *dasd_device_from_cdev(struct ccw_device *);
 struct dasd_device *dasd_device_from_cdev_locked(struct ccw_device *);
 struct dasd_device *dasd_device_from_devindex(int);
 
+void dasd_add_link_to_gendisk(struct gendisk *, struct dasd_device *);
+struct dasd_device *dasd_device_from_gendisk(struct gendisk *);
+
 int dasd_parse(void);
 int dasd_busid_known(const char *);
 

diff --git a/drivers/s390/block/dasd_ioctl.c b/drivers/s390/block/dasd_ioctl.c
index 26075e95b1bad786db8ff7b0e8504521fe8f3d68..72261e4c516de89e836a2afb3d646d24b947ecfd 100644 (file)
--- a/drivers/s390/block/dasd_ioctl.c
+++ b/drivers/s390/block/dasd_ioctl.c
@@ -42,16 +42,22 @@ dasd_ioctl_api_version(void __user *argp)
 static int
 dasd_ioctl_enable(struct block_device *bdev)
 {
-       struct dasd_block *block = bdev->bd_disk->private_data;
+       struct dasd_device *base;
 
        if (!capable(CAP_SYS_ADMIN))
                return -EACCES;
 
-       dasd_enable_device(block->base);
+       base = dasd_device_from_gendisk(bdev->bd_disk);
+       if (!base)
+               return -ENODEV;
+
+       dasd_enable_device(base);
        /* Formatting the dasd device can change the capacity. */
        mutex_lock(&bdev->bd_mutex);
-       i_size_write(bdev->bd_inode, (loff_t)get_capacity(block->gdp) << 9);
+       i_size_write(bdev->bd_inode,
+                    (loff_t)get_capacity(base->block->gdp) << 9);
        mutex_unlock(&bdev->bd_mutex);
+       dasd_put_device(base);
        return 0;
 }
 
@@ -62,11 +68,14 @@ dasd_ioctl_enable(struct block_device *bdev)
 static int
 dasd_ioctl_disable(struct block_device *bdev)
 {
-       struct dasd_block *block = bdev->bd_disk->private_data;
+       struct dasd_device *base;
 
        if (!capable(CAP_SYS_ADMIN))
                return -EACCES;
 
+       base = dasd_device_from_gendisk(bdev->bd_disk);
+       if (!base)
+               return -ENODEV;
        /*
         * Man this is sick. We don't do a real disable but only downgrade
         * the device to DASD_STATE_BASIC. The reason is that dasdfmt uses
@@ -75,7 +84,7 @@ dasd_ioctl_disable(struct block_device *bdev)
         * using the BIODASDFMT ioctl. Therefore the correct state for the
         * device is DASD_STATE_BASIC that allows to do basic i/o.
         */
-       dasd_set_target_state(block->base, DASD_STATE_BASIC);
+       dasd_set_target_state(base, DASD_STATE_BASIC);
        /*
         * Set i_size to zero, since read, write, etc. check against this
         * value.
@@ -83,6 +92,7 @@ dasd_ioctl_disable(struct block_device *bdev)
        mutex_lock(&bdev->bd_mutex);
        i_size_write(bdev->bd_inode, 0);
        mutex_unlock(&bdev->bd_mutex);
+       dasd_put_device(base);
        return 0;
 }
 
@@ -191,26 +201,36 @@ static int dasd_format(struct dasd_block *block, struct format_data_t *fdata)
 static int
 dasd_ioctl_format(struct block_device *bdev, void __user *argp)
 {
-       struct dasd_block *block = bdev->bd_disk->private_data;
+       struct dasd_device *base;
        struct format_data_t fdata;
+       int rc;
 
        if (!capable(CAP_SYS_ADMIN))
                return -EACCES;
        if (!argp)
                return -EINVAL;
-
-       if (block->base->features & DASD_FEATURE_READONLY ||
-           test_bit(DASD_FLAG_DEVICE_RO, &block->base->flags))
+       base = dasd_device_from_gendisk(bdev->bd_disk);
+       if (!base)
+               return -ENODEV;
+       if (base->features & DASD_FEATURE_READONLY ||
+           test_bit(DASD_FLAG_DEVICE_RO, &base->flags)) {
+               dasd_put_device(base);
                return -EROFS;
-       if (copy_from_user(&fdata, argp, sizeof(struct format_data_t)))
+       }
+       if (copy_from_user(&fdata, argp, sizeof(struct format_data_t))) {
+               dasd_put_device(base);
                return -EFAULT;
+       }
        if (bdev != bdev->bd_contains) {
                pr_warning("%s: The specified DASD is a partition and cannot "
                           "be formatted\n",
-                          dev_name(&block->base->cdev->dev));
+                          dev_name(&base->cdev->dev));
+               dasd_put_device(base);
                return -EINVAL;
        }
-       return dasd_format(block, &fdata);
+       rc = dasd_format(base->block, &fdata);
+       dasd_put_device(base);
+       return rc;
 }
 
 #ifdef CONFIG_DASD_PROFILE
@@ -340,8 +360,8 @@ static int dasd_ioctl_information(struct dasd_block *block,
 static int
 dasd_ioctl_set_ro(struct block_device *bdev, void __user *argp)
 {
-       struct dasd_block *block =  bdev->bd_disk->private_data;
-       int intval;
+       struct dasd_device *base;
+       int intval, rc;
 
        if (!capable(CAP_SYS_ADMIN))
                return -EACCES;
@@ -350,10 +370,17 @@ dasd_ioctl_set_ro(struct block_device *bdev, void __user *argp)
                return -EINVAL;
        if (get_user(intval, (int __user *)argp))
                return -EFAULT;
-       if (!intval && test_bit(DASD_FLAG_DEVICE_RO, &block->base->flags))
+       base = dasd_device_from_gendisk(bdev->bd_disk);
+       if (!base)
+               return -ENODEV;
+       if (!intval && test_bit(DASD_FLAG_DEVICE_RO, &base->flags)) {
+               dasd_put_device(base);
                return -EROFS;
+       }
        set_disk_ro(bdev->bd_disk, intval);
-       return dasd_set_feature(block->base->cdev, DASD_FEATURE_READONLY, intval);
+       rc = dasd_set_feature(base->cdev, DASD_FEATURE_READONLY, intval);
+       dasd_put_device(base);
+       return rc;
 }
 
 static int dasd_ioctl_readall_cmb(struct dasd_block *block, unsigned int cmd,
@@ -372,59 +399,78 @@ static int dasd_ioctl_readall_cmb(struct dasd_block *block, unsigned int cmd,
 int dasd_ioctl(struct block_device *bdev, fmode_t mode,
               unsigned int cmd, unsigned long arg)
 {
-       struct dasd_block *block = bdev->bd_disk->private_data;
+       struct dasd_block *block;
+       struct dasd_device *base;
        void __user *argp;
+       int rc;
 
        if (is_compat_task())
                argp = compat_ptr(arg);
        else
                argp = (void __user *)arg;
 
-       if (!block)
-                return -ENODEV;
-
        if ((_IOC_DIR(cmd) != _IOC_NONE) && !arg) {
                PRINT_DEBUG("empty data ptr");
                return -EINVAL;
        }
 
+       base = dasd_device_from_gendisk(bdev->bd_disk);
+       if (!base)
+               return -ENODEV;
+       block = base->block;
+       rc = 0;
        switch (cmd) {
        case BIODASDDISABLE:
-               return dasd_ioctl_disable(bdev);
+               rc = dasd_ioctl_disable(bdev);
+               break;
        case BIODASDENABLE:
-               return dasd_ioctl_enable(bdev);
+               rc = dasd_ioctl_enable(bdev);
+               break;
        case BIODASDQUIESCE:
-               return dasd_ioctl_quiesce(block);
+               rc = dasd_ioctl_quiesce(block);
+               break;
        case BIODASDRESUME:
-               return dasd_ioctl_resume(block);
+               rc = dasd_ioctl_resume(block);
+               break;
        case BIODASDFMT:
-               return dasd_ioctl_format(bdev, argp);
+               rc = dasd_ioctl_format(bdev, argp);
+               break;
        case BIODASDINFO:
-               return dasd_ioctl_information(block, cmd, argp);
+               rc = dasd_ioctl_information(block, cmd, argp);
+               break;
        case BIODASDINFO2:
-               return dasd_ioctl_information(block, cmd, argp);
+               rc = dasd_ioctl_information(block, cmd, argp);
+               break;
        case BIODASDPRRD:
-               return dasd_ioctl_read_profile(block, argp);
+               rc = dasd_ioctl_read_profile(block, argp);
+               break;
        case BIODASDPRRST:
-               return dasd_ioctl_reset_profile(block);
+               rc = dasd_ioctl_reset_profile(block);
+               break;
        case BLKROSET:
-               return dasd_ioctl_set_ro(bdev, argp);
+               rc = dasd_ioctl_set_ro(bdev, argp);
+               break;
        case DASDAPIVER:
-               return dasd_ioctl_api_version(argp);
+               rc = dasd_ioctl_api_version(argp);
+               break;
        case BIODASDCMFENABLE:
-               return enable_cmf(block->base->cdev);
+               rc = enable_cmf(base->cdev);
+               break;
        case BIODASDCMFDISABLE:
-               return disable_cmf(block->base->cdev);
+               rc = disable_cmf(base->cdev);
+               break;
        case BIODASDREADALLCMB:
-               return dasd_ioctl_readall_cmb(block, cmd, argp);
+               rc = dasd_ioctl_readall_cmb(block, cmd, argp);
+               break;
        default:
                /* if the discipline has an ioctl method try it. */
-               if (block->base->discipline->ioctl) {
-                       int rval = block->base->discipline->ioctl(block, cmd, argp);
-                       if (rval != -ENOIOCTLCMD)
-                               return rval;
-               }
-
-               return -EINVAL;
+               if (base->discipline->ioctl) {
+                       rc = base->discipline->ioctl(block, cmd, argp);
+                       if (rc == -ENOIOCTLCMD)
+                               rc = -EINVAL;
+               } else
+                       rc = -EINVAL;
        }
+       dasd_put_device(base);
+       return rc;
 }