two leaks in scsi_alloc_sdev failure paths

PATCH Dave Jones 2 participants 3 messages Mar 9, 2006 SCSI

Share & tools

Dave Jones

author

2006-03-09 03:36 UTC #0 lore raw

If the scsi_alloc_queue or the slave_alloc calls in scsi_alloc_device fail, we forget to release the locally allocated sdev on the failure path.

Coverity #609

Signed-off-by Dave Jones <[email protected]>

drivers/scsi/scsi_scan.c renamed from drivers/scsi/scsi_scan.c~ +3 -1

@@ -252,7 +252,7 @@ static struct scsi_device *scsi_alloc_sd
 		/* release fn is set up in scsi_sysfs_device_initialise, so
 		 * have to free and put manually here */
 		put_device(&starget->dev);
-		goto out;
+		goto out_free;
 	}
 
 	sdev->request_queue->queuedata = sdev;

@@ -278,6 +278,8 @@ static struct scsi_device *scsi_alloc_sd
 out_device_destroy:
 	transport_destroy_device(&sdev->sdev_gendev);
 	put_device(&sdev->sdev_gendev);
+out_free:
+	kfree(sdev);
 out:
 	if (display_failure_msg)
 		printk(ALLOC_FAILURE_MSG, __FUNCTION__);

Signature · 1 line

http://www.codemonkey.org.uk

Brian King

2006-03-09 15:07 UTC #1 lore raw

Quoted 2 lines · Dave Jones

If the scsi_alloc_queue or the slave_alloc calls in scsi_alloc_device fail, we forget to release the locally allocated sdev on the failure path.

Actually, I think the slave_alloc failure path works today, and this patch breaks it. Today, in the slave_alloc failure path, the release function called as a result of the put_device in out_device_destroy should end up freeing the sdev. Your patch will result in a double free in this path.

drivers/scsi/scsi_scan.c static struct scsi_device *scsi_alloc_sd · line 255

Quoted 8 lines

drivers/scsi/scsi_scan.c renamed from drivers/scsi/scsi_scan.c~ +1 -1

@@ -252,7 +252,7 @@ static struct scsi_device *scsi_alloc_sd
 		/* release fn is set up in scsi_sysfs_device_initialise, so
 		 * have to free and put manually here */
 		put_device(&starget->dev);
-		goto out;
+		goto out_free;

Rather than this change, I think just adding a kfree(sdev) before the goto out would accomplish what you want.

Brian

Signature · 3 lines

Brian King
eServer Storage I/O
IBM Linux Technology Center

Dave Jones

author

2006-03-09 15:21 UTC #2 lore raw

Quoted 9 lines · Brian King

Quoted 2 lines · Dave Jones
If the scsi_alloc_queue or the slave_alloc calls in scsi_alloc_device fail, we forget to release the locally allocated sdev on the failure path.
Actually, I think the slave_alloc failure path works today, and this patch breaks it. Today, in the slave_alloc failure path, the release function called
as a result of the put_device in out_device_destroy should end up freeing the sdev.

Correct, thanks.

Quoted 2 lines

Rather than this change, I think just adding a kfree(sdev) before the goto out would accomplish what you want.

looks a lot simpler.

Thanks.

Dave

If the scsi_alloc_queue or the slave_alloc calls in scsi_alloc_device fail, we forget to release the locally allocated sdev on the failure path.

Coverity #609

Signed-off-by Dave Jones <[email protected]>

drivers/scsi/scsi_scan.c renamed from drivers/scsi/scsi_scan.c~ +1 -0

@@ -252,6 +252,7 @@ static struct scsi_device *scsi_alloc_sd
 		/* release fn is set up in scsi_sysfs_device_initialise, so
 		 * have to free and put manually here */
 		put_device(&starget->dev);
+		kfree(sdev);
 		goto out;
 	}

Signature · 1 line

http://www.codemonkey.org.uk