On 10/01/2013 12:15 AM, Josh Durgin wrote:
> get_user() and set_disk_ro() may allocate memory, leading to a
> potential deadlock if theye are called while a spin lock is held.
>
> Move the acquisition and release of rbd_dev->lock from rbd_ioctl()
> into rbd_ioctl_set_ro(), so it can occur between get_user() and
> set_disk_ro().
This fix looks good. I have a couple small comments to consider.
Reviewed-by: Alex Elder <elder@xxxxxxxxxx>
> Signed-off-by: Josh Durgin <josh.durgin@xxxxxxxxxxx>
> ---
> drivers/block/rbd.c | 36 +++++++++++++++++++++++-------------
> 1 files changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 34bcdb7..b3b1b57 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -510,23 +510,42 @@ static void rbd_release(struct gendisk *disk, fmode_t mode)
>
> static int rbd_ioctl_set_ro(struct rbd_device *rbd_dev, unsigned long arg)
> {
> + int ret = 0;
> int val;
> bool ro;
> + bool ro_changed = false;
>
> + /* get_user() may sleep, so call it before taking rbd_dev->lock */
> if (get_user(val, (int __user *)(arg)))
> return -EFAULT;
>
> + spin_lock_irq(&rbd_dev->lock);
> + /* prevent others open this device */
> + if (rbd_dev->open_count > 1) {
> + ret = -EBUSY;
> + goto out;
> + }
I like to do as little as possible inside spinlock protection.
> +
> ro = val ? true : false;
This assignment can be made outside the lock.
> /* Snapshot doesn't allow to write*/
> - if (rbd_dev->spec->snap_id != CEPH_NOSNAP && !ro)
> - return -EROFS;
> + if (rbd_dev->spec->snap_id != CEPH_NOSNAP && !ro) {
> + ret = -EROFS;
> + goto out;
> + }
This check can be too.
>
> if (rbd_dev->mapping.read_only != ro) {
> rbd_dev->mapping.read_only = ro;
> - set_disk_ro(rbd_dev->disk, ro ? 1 : 0);
> + ro_changed = true;
> }
>
> - return 0;
> +out:
> + spin_unlock_irq(&rbd_dev->lock);
> + /* set_disk_ro() may sleep, so call it after releasing rbd_dev->lock */
> + if (ret == 0 && ro_changed)
> + set_disk_ro(rbd_dev->disk, ro ? 1 : 0);
> +
I like white space a lot, but this is one too many.
> +
> + return ret;
> }
>
> static int rbd_ioctl(struct block_device *bdev, fmode_t mode,
> @@ -535,13 +554,6 @@ static int rbd_ioctl(struct block_device *bdev, fmode_t mode,
> struct rbd_device *rbd_dev = bdev->bd_disk->private_data;
> int ret = 0;
>
> - spin_lock_irq(&rbd_dev->lock);
> - /* prevent others open this device */
> - if (rbd_dev->open_count > 1) {
> - ret = -EBUSY;
> - goto out;
> - }
> -
> switch (cmd) {
> case BLKROSET:
> ret = rbd_ioctl_set_ro(rbd_dev, arg);
> @@ -550,8 +562,6 @@ static int rbd_ioctl(struct block_device *bdev, fmode_t mode,
> ret = -ENOTTY;
> }
>
> -out:
> - spin_unlock_irq(&rbd_dev->lock);
> return ret;
> }
>
>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
