* [PATCH 1/3] ch: fixup refcounting imbalance for SCSI devices
2020-02-13 15:32 [PATCHv2 0/3] ch: fixup refcounting imbalance for SCSI devices Hannes Reinecke
@ 2020-02-13 15:32 ` Hannes Reinecke
2020-02-20 4:49 ` Bart Van Assche
2020-02-13 15:32 ` [PATCH 2/3] ch: synchronize ch_probe() and ch_open() Hannes Reinecke
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Hannes Reinecke @ 2020-02-13 15:32 UTC (permalink / raw)
To: Martin K. Petersen
Cc: Christoph Hellwig, Bart van Assche, James Bottomley, linux-scsi,
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] ch: fixup refcounting imbalance for SCSI devices
2020-02-13 15:32 ` [PATCH 1/3] " Hannes Reinecke
@ 2020-02-20 4:49 ` Bart Van Assche
0 siblings, 0 replies; 8+ messages in thread
From: Bart Van Assche @ 2020-02-20 4:49 UTC (permalink / raw)
To: Hannes Reinecke, Martin K. Petersen
Cc: Christoph Hellwig, James Bottomley, linux-scsi
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>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/3] ch: synchronize ch_probe() and ch_open()
2020-02-13 15:32 [PATCHv2 0/3] ch: fixup refcounting imbalance for SCSI devices Hannes Reinecke
2020-02-13 15:32 ` [PATCH 1/3] " Hannes Reinecke
@ 2020-02-13 15:32 ` Hannes Reinecke
2020-02-20 4:51 ` Bart Van Assche
2020-02-13 15:32 ` [PATCH 3/3] ch: remove ch_mutex() Hannes Reinecke
2020-02-24 19:54 ` [PATCHv2 0/3] ch: fixup refcounting imbalance for SCSI devices Martin K. Petersen
3 siblings, 1 reply; 8+ messages in thread
From: Hannes Reinecke @ 2020-02-13 15:32 UTC (permalink / raw)
To: Martin K. Petersen
Cc: Christoph Hellwig, Bart van Assche, James Bottomley, linux-scsi,
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] ch: synchronize ch_probe() and ch_open()
2020-02-13 15:32 ` [PATCH 2/3] ch: synchronize ch_probe() and ch_open() Hannes Reinecke
@ 2020-02-20 4:51 ` Bart Van Assche
0 siblings, 0 replies; 8+ messages in thread
From: Bart Van Assche @ 2020-02-20 4:51 UTC (permalink / raw)
To: Hannes Reinecke, Martin K. Petersen
Cc: Christoph Hellwig, James Bottomley, linux-scsi
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>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] ch: remove ch_mutex()
2020-02-13 15:32 [PATCHv2 0/3] ch: fixup refcounting imbalance for SCSI devices Hannes Reinecke
2020-02-13 15:32 ` [PATCH 1/3] " Hannes Reinecke
2020-02-13 15:32 ` [PATCH 2/3] ch: synchronize ch_probe() and ch_open() Hannes Reinecke
@ 2020-02-13 15:32 ` Hannes Reinecke
2020-02-20 4:51 ` Bart Van Assche
2020-02-24 19:54 ` [PATCHv2 0/3] ch: fixup refcounting imbalance for SCSI devices Martin K. Petersen
3 siblings, 1 reply; 8+ messages in thread
From: Hannes Reinecke @ 2020-02-13 15:32 UTC (permalink / raw)
To: Martin K. Petersen
Cc: Christoph Hellwig, Bart van Assche, James Bottomley, linux-scsi,
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCHv2 0/3] ch: fixup refcounting imbalance for SCSI devices
2020-02-13 15:32 [PATCHv2 0/3] ch: fixup refcounting imbalance for SCSI devices Hannes Reinecke
` (2 preceding siblings ...)
2020-02-13 15:32 ` [PATCH 3/3] ch: remove ch_mutex() Hannes Reinecke
@ 2020-02-24 19:54 ` Martin K. Petersen
3 siblings, 0 replies; 8+ messages in thread
From: Martin K. Petersen @ 2020-02-24 19:54 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Martin K. Petersen, Christoph Hellwig, Bart van Assche,
James Bottomley, linux-scsi
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
^ permalink raw reply [flat|nested] 8+ messages in thread