[3/4] scsi: add missing get_device() return value checks

Commit Message

Hannes Reinecke Dec. 12, 2017, 8:57 a.m. UTC

We need to validate that get_device() succeeded, otherwise
we'll end up working with invalid devices.

Signed-off-by: Hannes Reinecke <[e-mail]>
---
 drivers/scsi/scsi_scan.c | 14 ++++++++++++--
 drivers/scsi/sd.c        |  8 +++++---
 2 files changed, 17 insertions(+), 5 deletions(-)

Comments

Bart Van Assche Dec. 14, 2017, 10:03 p.m. UTC | #1

On Tue, 2017-12-12 at 09:57 +0100, Hannes Reinecke wrote:
>  	sdev->sdev_gendev.parent = get_device(&starget->dev);

> +	if (!sdev->sdev_gendev.parent) {

> +		kfree(sdev);

> +		return NULL;

> +	}


Are you sure that get_device() can return NULL?

Thanks,

Bart.

Hannes Reinecke Dec. 15, 2017, 12:08 p.m. UTC | #2

On 12/14/2017 11:03 PM, Bart Van Assche wrote:
> On Tue, 2017-12-12 at 09:57 +0100, Hannes Reinecke wrote:
>>  	sdev->sdev_gendev.parent = get_device(&starget->dev);
>> +	if (!sdev->sdev_gendev.parent) {
>> +		kfree(sdev);
>> +		return NULL;
>> +	}
> 
> Are you sure that get_device() can return NULL?
> 
If it can't return NULL, why bother having a return value at all?
And this is not the only place where get_device() is checked for a NULL
return value.

We _really_ need to come up with a consistent view here.
If get_device() can never return NULL I would strongly argue for making
it a void function.
If it _can_ return NULL we need to check for it.

Either way I don't mind; but please stay consistent.

Cheers,

Hannes

Patch

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index be5e919..18edfd7 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -245,6 +245,10 @@  static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	INIT_WORK(&sdev->requeue_work, scsi_requeue_run_queue);
 
 	sdev->sdev_gendev.parent = get_device(&starget->dev);
+	if (!sdev->sdev_gendev.parent) {
+		kfree(sdev);
+		return NULL;
+	}
 	sdev->sdev_target = starget;
 
 	/* usually NULL and set by ->slave_alloc instead */
@@ -366,7 +370,8 @@  static struct scsi_target *__scsi_find_target(struct device *parent,
 		}
 	}
 	if (found_starget)
-		get_device(&found_starget->dev);
+		if (!get_device(&found_starget->dev))
+			found_starget = NULL;
 
 	return found_starget;
 }
@@ -436,6 +441,10 @@  static struct scsi_target *scsi_alloc_target(struct device *parent,
 	device_initialize(dev);
 	kref_init(&starget->reap_ref);
 	dev->parent = get_device(parent);
+	if (!dev->parent) {
+		kfree(starget);
+		return NULL;
+	}
 	dev_set_name(dev, "target%d:%d:%d", shost->host_no, channel, id);
 	dev->bus = &scsi_bus_type;
 	dev->type = &scsi_target_type;
@@ -469,7 +478,8 @@  static struct scsi_target *scsi_alloc_target(struct device *parent,
 			return NULL;
 		}
 	}
-	get_device(dev);
+	/* No good way to recover here; keep fingers crossed */
+	WARN_ON(!get_device(dev));
 
 	return starget;
 
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 228b0b62..abbab17 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -619,10 +619,12 @@  static struct scsi_disk *scsi_disk_get(struct gendisk *disk)
 
 	if (disk->private_data) {
 		sdkp = scsi_disk(disk);
-		if (scsi_device_get(sdkp->device) == 0)
-			get_device(&sdkp->dev);
-		else
+		if (scsi_device_get(sdkp->device))
+			sdkp = NULL;
+		else if (!get_device(&sdkp->dev)) {
+			scsi_device_put(sdkp->device);
 			sdkp = NULL;
+		}
 	}
 	mutex_unlock(&sd_ref_mutex);
 	return sdkp;