[PATCH] ch: add refcounting

[PATCH] ch: add refcounting

[Hannes Reinecke]

The 'scsi_changer' structure needs refcounting, as the device
might be removed while the fd is still open.

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

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 2a32374..adb5426 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -102,6 +102,7 @@ do {									\
 static struct class * ch_sysfs_class;
 
 typedef struct {
+	struct kref         ref;
 	struct list_head    list;
 	int                 minor;
 	char                name[8];
@@ -565,13 +566,23 @@ static int ch_gstatus(scsi_changer *ch, int type, unsigned char __user *dest)
 
 /* ------------------------------------------------------------------------ */
 
+static void ch_destroy(struct kref *ref)
+{
+	scsi_changer *ch = container_of(ref, scsi_changer, ref);
+
+	kfree(ch->dt);
+	kfree(ch);
+}
+
 static int
 ch_release(struct inode *inode, struct file *file)
 {
 	scsi_changer *ch = file->private_data;
 
 	scsi_device_put(ch->device);
+	ch->device = NULL;
 	file->private_data = NULL;
+	kref_put(&ch->ref, ch_destroy);
 	return 0;
 }
 
@@ -590,6 +601,7 @@ ch_open(struct inode *inode, struct file *file)
 		mutex_unlock(&ch_mutex);
 		return -ENXIO;
 	}
+	kref_get(&ch->ref);
 	spin_unlock(&ch_index_lock);
 
 	file->private_data = ch;
@@ -931,6 +943,7 @@ static int ch_probe(struct device *dev)
 	}
 
 	mutex_init(&ch->lock);
+	kref_init(&ch->ref);
 	ch->device = sd;
 	ch_readconfig(ch);
 	if (init)
@@ -956,8 +969,7 @@ static int ch_remove(struct device *dev)
 	spin_unlock(&ch_index_lock);
 
 	device_destroy(ch_sysfs_class, MKDEV(SCSI_CHANGER_MAJOR,ch->minor));
-	kfree(ch->dt);
-	kfree(ch);
+	kref_put(&ch->ref, ch_destroy);
 	return 0;
 }
 
-- 
1.7.12.4

Re: [PATCH] ch: add refcounting

[Christoph Hellwig]

>  static int
>  ch_release(struct inode *inode, struct file *file)
>  {
>  	scsi_changer *ch = file->private_data;
>  
>  	scsi_device_put(ch->device);
> +	ch->device = NULL;
>  	file->private_data = NULL;
> +	kref_put(&ch->ref, ch_destroy);

Any reason you need to put the scsi_device here already?  Defering
this would give you much eaiser life time rules, and no need to
deal with a NULL ch->device ever.

Re: [PATCH] ch: add refcounting

[Hannes Reinecke]

On 04/10/2014 01:51 PM, Christoph Hellwig wrote:
>>  static int
>>  ch_release(struct inode *inode, struct file *file)
>>  {
>>  	scsi_changer *ch = file->private_data;
>>  
>>  	scsi_device_put(ch->device);
>> +	ch->device = NULL;
>>  	file->private_data = NULL;
>> +	kref_put(&ch->ref, ch_destroy);
> 
> Any reason you need to put the scsi_device here already?  Defering
> this would give you much eaiser life time rules, and no need to
> deal with a NULL ch->device ever.
> 
Sure. But this would require a far more in-depth analysis of the
lifetime of the ch object, and most likely a far more intrusive
patch. You're welcome to do so :-)

This patch is just a minimal fix; I didn't dare to change too much
of the internals.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ch: add refcounting

[Christoph Hellwig]

On Thu, Apr 10, 2014 at 04:12:57PM +0200, Hannes Reinecke wrote:
> On 04/10/2014 01:51 PM, Christoph Hellwig wrote:
> >>  static int
> >>  ch_release(struct inode *inode, struct file *file)
> >>  {
> >>  	scsi_changer *ch = file->private_data;
> >>  
> >>  	scsi_device_put(ch->device);
> >> +	ch->device = NULL;
> >>  	file->private_data = NULL;
> >> +	kref_put(&ch->ref, ch_destroy);
> > 
> > Any reason you need to put the scsi_device here already?  Defering
> > this would give you much eaiser life time rules, and no need to
> > deal with a NULL ch->device ever.
> > 
> Sure. But this would require a far more in-depth analysis of the
> lifetime of the ch object, and most likely a far more intrusive
> patch. You're welcome to do so :-)
> 
> This patch is just a minimal fix; I didn't dare to change too much
> of the internals.

Oh well, let's go with your simpler version for now.

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] ch: add refcounting

[Martin K. Petersen]

Hannes,

> struct scsi_changer needs refcounting as the device might be removed
> while the fd is still open.

Applied to 4.14/scsi-queue.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH] ch: add refcounting

[Hannes Reinecke]

struct scsi_changer needs refcounting as the device
might be removed while the fd is still open.

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

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index dad959f..b14583a 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -105,6 +105,7 @@
 static struct class * ch_sysfs_class;
 
 typedef struct {
+	struct kref         ref;
 	struct list_head    list;
 	int                 minor;
 	char                name[8];
@@ -563,13 +564,23 @@ static int ch_gstatus(scsi_changer *ch, int type, unsigned char __user *dest)
 
 /* ------------------------------------------------------------------------ */
 
+static void ch_destroy(struct kref *ref)
+{
+	scsi_changer *ch = container_of(ref, scsi_changer, ref);
+
+	kfree(ch->dt);
+	kfree(ch);
+}
+
 static int
 ch_release(struct inode *inode, struct file *file)
 {
 	scsi_changer *ch = file->private_data;
 
 	scsi_device_put(ch->device);
+	ch->device = NULL;
 	file->private_data = NULL;
+	kref_put(&ch->ref, ch_destroy);
 	return 0;
 }
 
@@ -588,6 +599,7 @@ static int ch_gstatus(scsi_changer *ch, int type, unsigned char __user *dest)
 		mutex_unlock(&ch_mutex);
 		return -ENXIO;
 	}
+	kref_get(&ch->ref);
 	spin_unlock(&ch_index_lock);
 
 	file->private_data = ch;
@@ -935,8 +947,11 @@ static int ch_probe(struct device *dev)
 	}
 
 	mutex_init(&ch->lock);
+	kref_init(&ch->ref);
 	ch->device = sd;
-	ch_readconfig(ch);
+	ret = ch_readconfig(ch);
+	if (ret)
+		goto destroy_dev;
 	if (init)
 		ch_init_elem(ch);
 
@@ -944,6 +959,8 @@ static int ch_probe(struct device *dev)
 	sdev_printk(KERN_INFO, sd, "Attached scsi changer %s\n", ch->name);
 
 	return 0;
+destroy_dev:
+	device_destroy(ch_sysfs_class, MKDEV(SCSI_CHANGER_MAJOR,ch->minor));
 remove_idr:
 	idr_remove(&ch_index_idr, ch->minor);
 free_ch:
@@ -960,8 +977,7 @@ static int ch_remove(struct device *dev)
 	spin_unlock(&ch_index_lock);
 
 	device_destroy(ch_sysfs_class, MKDEV(SCSI_CHANGER_MAJOR,ch->minor));
-	kfree(ch->dt);
-	kfree(ch);
+	kref_put(&ch->ref, ch_destroy);
 	return 0;
 }
 
-- 
1.8.5.6