[1/3] ch: fixup refcounting imbalance for SCSI devices

Commit Message

Hannes Reinecke Feb. 13, 2020, 3:32 p.m. UTC

The SCSI device is required to be present during ch_probe()
and ch_open(). But the SCSI device itself is only checked during
ch_open(), so it's anyones guess if it had been present during
ch_probe(). And consequently we can't reliably detach it during
ch_release(), as ch_remove() might have been called first.
So initialize the changer device during ch_probe(), and
take a reference to the SCSI device during both ch_probe()
and ch_open().

Signed-off-by: Hannes Reinecke <[e-mail]>
---
 drivers/scsi/ch.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

Comments

Bart Van Assche Feb. 20, 2020, 4:49 a.m. UTC | #1

On 2020-02-13 07:32, Hannes Reinecke wrote:
> The SCSI device is required to be present during ch_probe()
> and ch_open(). But the SCSI device itself is only checked during
> ch_open(), so it's anyones guess if it had been present during
> ch_probe(). And consequently we can't reliably detach it during
> ch_release(), as ch_remove() might have been called first.
> So initialize the changer device during ch_probe(), and
> take a reference to the SCSI device during both ch_probe()
> and ch_open().

Reviewed-by: Bart Van Assche <[e-mail]>

Patch

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index ed5f4a6ae270..974afb4bd5fe 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -569,6 +569,7 @@  static void ch_destroy(struct kref *ref)
 {
 	scsi_changer *ch = container_of(ref, scsi_changer, ref);
 
+	ch->device = NULL;
 	kfree(ch->dt);
 	kfree(ch);
 }
@@ -594,14 +595,17 @@  ch_open(struct inode *inode, struct file *file)
 	spin_lock(&ch_index_lock);
 	ch = idr_find(&ch_index_idr, minor);
 
-	if (NULL == ch || scsi_device_get(ch->device)) {
+	if (NULL == ch || !kref_get_unless_zero(&ch->ref)) {
 		spin_unlock(&ch_index_lock);
 		mutex_unlock(&ch_mutex);
 		return -ENXIO;
 	}
-	kref_get(&ch->ref);
 	spin_unlock(&ch_index_lock);
-
+	if (scsi_device_get(ch->device)) {
+		kref_put(&ch->ref, ch_destroy);
+		mutex_unlock(&ch_mutex);
+		return -ENXIO;
+	}
 	file->private_data = ch;
 	mutex_unlock(&ch_mutex);
 	return 0;
@@ -938,6 +942,12 @@  static int ch_probe(struct device *dev)
 
 	ch->minor = ret;
 	sprintf(ch->name,"ch%d",ch->minor);
+	ret = scsi_device_get(sd);
+	if (ret) {
+		sdev_printk(KERN_WARNING, sd, "ch%d: failed to get device\n",
+			    ch->minor);
+		goto remove_idr;
+	}
 
 	class_dev = device_create(ch_sysfs_class, dev,
 				  MKDEV(SCSI_CHANGER_MAJOR, ch->minor), ch,
@@ -946,7 +956,7 @@  static int ch_probe(struct device *dev)
 		sdev_printk(KERN_WARNING, sd, "ch%d: device_create failed\n",
 			    ch->minor);
 		ret = PTR_ERR(class_dev);
-		goto remove_idr;
+		goto put_device;
 	}
 
 	mutex_init(&ch->lock);
@@ -964,6 +974,8 @@  static int ch_probe(struct device *dev)
 	return 0;
 destroy_dev:
 	device_destroy(ch_sysfs_class, MKDEV(SCSI_CHANGER_MAJOR, ch->minor));
+put_device:
+	scsi_device_put(sd);
 remove_idr:
 	idr_remove(&ch_index_idr, ch->minor);
 free_ch:
@@ -977,9 +989,11 @@  static int ch_remove(struct device *dev)
 
 	spin_lock(&ch_index_lock);
 	idr_remove(&ch_index_idr, ch->minor);
+	dev_set_drvdata(dev, NULL);
 	spin_unlock(&ch_index_lock);
 
 	device_destroy(ch_sysfs_class, MKDEV(SCSI_CHANGER_MAJOR,ch->minor));
+	scsi_device_put(ch->device);
 	kref_put(&ch->ref, ch_destroy);
 	return 0;
 }