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

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

[Hannes Reinecke]

Hi all,

here's a set of fixes for a long-standing issue in the 'ch' driver
where we would crash if one of the referenced devices was removed
from underneath us.

As usual, comments and reviews are welcome.

Changes to v1:
- Reworked after reviews from Bart

Hannes Reinecke (3):
  ch: fixup refcounting imbalance for SCSI devices
  ch: synchronize ch_probe() and ch_open()
  ch: remove ch_mutex()

 drivers/scsi/ch.c | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

-- 
2.16.4

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

[Hannes Reinecke]

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 <hare@suse.de>
---
 drivers/scsi/ch.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

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;
 }
-- 
2.16.4

[PATCH 2/3] ch: synchronize ch_probe() and ch_open()

[Hannes Reinecke]

The 'ch' device node is created before the configuration is
being read in, which leads to a race window when ch_open() is called
before that.
To avoid any races we should be taking the device mutex during
ch_readconfig() and ch_init_elem(), and also during ch_open().
That ensures ch_probe is finished before ch_open() completes.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/ch.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 974afb4bd5fe..9cbfb00ab950 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -606,7 +606,10 @@ ch_open(struct inode *inode, struct file *file)
 		mutex_unlock(&ch_mutex);
 		return -ENXIO;
 	}
+	/* Synchronize with ch_probe() */
+	mutex_lock(&ch->lock);
 	file->private_data = ch;
+	mutex_unlock(&ch->lock);
 	mutex_unlock(&ch_mutex);
 	return 0;
 }
@@ -949,6 +952,9 @@ static int ch_probe(struct device *dev)
 		goto remove_idr;
 	}
 
+	mutex_init(&ch->lock);
+	kref_init(&ch->ref);
+	ch->device = sd;
 	class_dev = device_create(ch_sysfs_class, dev,
 				  MKDEV(SCSI_CHANGER_MAJOR, ch->minor), ch,
 				  "s%s", ch->name);
@@ -959,15 +965,16 @@ static int ch_probe(struct device *dev)
 		goto put_device;
 	}
 
-	mutex_init(&ch->lock);
-	kref_init(&ch->ref);
-	ch->device = sd;
+	mutex_lock(&ch->lock);
 	ret = ch_readconfig(ch);
-	if (ret)
+	if (ret) {
+		mutex_unlock(&ch->lock);
 		goto destroy_dev;
+	}
 	if (init)
 		ch_init_elem(ch);
 
+	mutex_unlock(&ch->lock);
 	dev_set_drvdata(dev, ch);
 	sdev_printk(KERN_INFO, sd, "Attached scsi changer %s\n", ch->name);
 
-- 
2.16.4

[PATCH 3/3] ch: remove ch_mutex()

[Hannes Reinecke]

ch_mutex() was introduced with a mechanical conversion, but as we
now have correct locking we can remove it altogether.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/ch.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 9cbfb00ab950..4ea3b61f275f 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -44,7 +44,6 @@ MODULE_LICENSE("GPL");
 MODULE_ALIAS_CHARDEV_MAJOR(SCSI_CHANGER_MAJOR);
 MODULE_ALIAS_SCSI_DEVICE(TYPE_MEDIUM_CHANGER);
 
-static DEFINE_MUTEX(ch_mutex);
 static int init = 1;
 module_param(init, int, 0444);
 MODULE_PARM_DESC(init, \
@@ -591,26 +590,22 @@ ch_open(struct inode *inode, struct file *file)
 	scsi_changer *ch;
 	int minor = iminor(inode);
 
-	mutex_lock(&ch_mutex);
 	spin_lock(&ch_index_lock);
 	ch = idr_find(&ch_index_idr, minor);
 
 	if (NULL == ch || !kref_get_unless_zero(&ch->ref)) {
 		spin_unlock(&ch_index_lock);
-		mutex_unlock(&ch_mutex);
 		return -ENXIO;
 	}
 	spin_unlock(&ch_index_lock);
 	if (scsi_device_get(ch->device)) {
 		kref_put(&ch->ref, ch_destroy);
-		mutex_unlock(&ch_mutex);
 		return -ENXIO;
 	}
 	/* Synchronize with ch_probe() */
 	mutex_lock(&ch->lock);
 	file->private_data = ch;
 	mutex_unlock(&ch->lock);
-	mutex_unlock(&ch_mutex);
 	return 0;
 }
 
-- 
2.16.4

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

[Bart Van Assche]

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 <bvanassche@acm.org>

Re: [PATCH 2/3] ch: synchronize ch_probe() and ch_open()

[Bart Van Assche]

On 2020-02-13 07:32, Hannes Reinecke wrote:
> The 'ch' device node is created before the configuration is
> being read in, which leads to a race window when ch_open() is called
> before that.
> To avoid any races we should be taking the device mutex during
> ch_readconfig() and ch_init_elem(), and also during ch_open().
> That ensures ch_probe is finished before ch_open() completes.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 3/3] ch: remove ch_mutex()

[Bart Van Assche]

On 2020-02-13 07:32, Hannes Reinecke wrote:
> ch_mutex() was introduced with a mechanical conversion, but as we
> now have correct locking we can remove it altogether.
 Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCHv2 0/3] ch: fixup refcounting imbalance for SCSI devices

[Martin K. Petersen]

Hannes,

> here's a set of fixes for a long-standing issue in the 'ch' driver
> where we would crash if one of the referenced devices was removed from
> underneath us.

Applied to 5.7/scsi-queue. Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering