[PATCH] update sr to use kref and fix open/release race

[PATCH] update sr to use kref and fix open/release race

[James Bottomley]

The previous patch actually didn't null out the disk->private_data
pointer in it's release mechanism.  This does that and converts sr to
use the lightweight kref object instead of a kobject.

James

===== drivers/scsi/sr.c 1.104 vs edited =====
--- 1.104/drivers/scsi/sr.c	Thu Apr  8 08:39:12 2004
+++ edited/drivers/scsi/sr.c	Thu Apr  8 16:33:11 2004
@@ -86,6 +86,7 @@
 
 static unsigned long sr_index_bits[SR_DISKS / BITS_PER_LONG];
 static spinlock_t sr_index_lock = SPIN_LOCK_UNLOCKED;
+static DECLARE_MUTEX(sr_ref_sem);
 
 static int sr_open(struct cdrom_device_info *, int);
 static void sr_release(struct cdrom_device_info *);
@@ -119,20 +120,35 @@
 	.release = sr_kobject_release,
 };
 
+static inline struct scsi_cd *scsi_cd(struct gendisk *disk)
+{
+	return container_of(disk->private_data, struct scsi_cd, driver);
+}
+
 /*
  * The get and put routines for the struct scsi_cd.  Note this entity
  * has a scsi_device pointer and owns a reference to this.
  */
-static inline int scsi_cd_get(struct scsi_cd *cd)
+static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 {
+	struct scsi_cd *cd = NULL;
+
+	down(&sr_ref_sem);
+	if (disk->private_data == NULL)
+		goto out;
+	cd = scsi_cd(disk);
 	if (!kobject_get(&cd->kobj))
-		return -ENODEV;
-	return 0;
+		cd = NULL;
+ out:
+	up(&sr_ref_sem);
+	return cd;
 }
 
 static inline void scsi_cd_put(struct scsi_cd *cd)
 {
+	down(&sr_ref_sem);
 	kobject_put(&cd->kobj);
+	up(&sr_ref_sem);
 }
 
 /*
@@ -187,11 +203,6 @@
 	return retval;
 }
  
-static inline struct scsi_cd *scsi_cd(struct gendisk *disk)
-{
-	return container_of(disk->private_data, struct scsi_cd, driver);
-}
-
 /*
  * rw_intr is the interrupt routine for the device driver.
  *
@@ -440,8 +451,17 @@
 
 static int sr_block_open(struct inode *inode, struct file *file)
 {
+	struct gendisk *disk = inode->i_bdev->bd_disk;
 	struct scsi_cd *cd = scsi_cd(inode->i_bdev->bd_disk);
-	return cdrom_open(&cd->cdi, inode, file);
+	int ret = 0;
+
+	if(!(cd = scsi_cd_get(disk)))
+		return -ENXIO;
+
+	if((ret = cdrom_open(&cd->cdi, inode, file)) != 0)
+		scsi_cd_put(cd);
+
+	return ret;
 }
 
 static int sr_block_release(struct inode *inode, struct file *file)
@@ -496,10 +516,6 @@
 	struct scsi_device *sdev = cd->device;
 	int retval;
 
-	retval = scsi_cd_get(cd);
-	if (retval)
-		return retval;
-	
 	/*
 	 * If the device is in error recovery, wait until it is done.
 	 * If the device is offline, then disallow any access to it.

Re: [PATCH] update sr to use kref and fix open/release race

[James Bottomley]

And, er, with the correct patch this time.

James

===== sr.c 1.104 vs edited =====
--- 1.104/drivers/scsi/sr.c	Thu Apr  8 08:39:12 2004
+++ edited/sr.c	Fri Apr  9 09:26:07 2004
@@ -86,6 +86,7 @@
 
 static unsigned long sr_index_bits[SR_DISKS / BITS_PER_LONG];
 static spinlock_t sr_index_lock = SPIN_LOCK_UNLOCKED;
+static DECLARE_MUTEX(sr_ref_sem);
 
 static int sr_open(struct cdrom_device_info *, int);
 static void sr_release(struct cdrom_device_info *);
@@ -113,26 +114,37 @@
 	.generic_packet		= sr_packet,
 };
 
-static void sr_kobject_release(struct kobject *kobj);
+static void sr_kref_release(struct kref *kref);
 
-static struct kobj_type scsi_cdrom_kobj_type = {
-	.release = sr_kobject_release,
-};
+static inline struct scsi_cd *scsi_cd(struct gendisk *disk)
+{
+	return container_of(disk->private_data, struct scsi_cd, driver);
+}
 
 /*
  * The get and put routines for the struct scsi_cd.  Note this entity
  * has a scsi_device pointer and owns a reference to this.
  */
-static inline int scsi_cd_get(struct scsi_cd *cd)
+static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 {
-	if (!kobject_get(&cd->kobj))
-		return -ENODEV;
-	return 0;
+	struct scsi_cd *cd = NULL;
+
+	down(&sr_ref_sem);
+	if (disk->private_data == NULL)
+		goto out;
+	cd = scsi_cd(disk);
+	if (!kref_get(&cd->kref))
+		cd = NULL;
+ out:
+	up(&sr_ref_sem);
+	return cd;
 }
 
 static inline void scsi_cd_put(struct scsi_cd *cd)
 {
-	kobject_put(&cd->kobj);
+	down(&sr_ref_sem);
+	kref_put(&cd->kref);
+	up(&sr_ref_sem);
 }
 
 /*
@@ -187,11 +199,6 @@
 	return retval;
 }
  
-static inline struct scsi_cd *scsi_cd(struct gendisk *disk)
-{
-	return container_of(disk->private_data, struct scsi_cd, driver);
-}
-
 /*
  * rw_intr is the interrupt routine for the device driver.
  *
@@ -440,8 +447,17 @@
 
 static int sr_block_open(struct inode *inode, struct file *file)
 {
+	struct gendisk *disk = inode->i_bdev->bd_disk;
 	struct scsi_cd *cd = scsi_cd(inode->i_bdev->bd_disk);
-	return cdrom_open(&cd->cdi, inode, file);
+	int ret = 0;
+
+	if(!(cd = scsi_cd_get(disk)))
+		return -ENXIO;
+
+	if((ret = cdrom_open(&cd->cdi, inode, file)) != 0)
+		scsi_cd_put(cd);
+
+	return ret;
 }
 
 static int sr_block_release(struct inode *inode, struct file *file)
@@ -496,10 +512,6 @@
 	struct scsi_device *sdev = cd->device;
 	int retval;
 
-	retval = scsi_cd_get(cd);
-	if (retval)
-		return retval;
-	
 	/*
 	 * If the device is in error recovery, wait until it is done.
 	 * If the device is offline, then disallow any access to it.
@@ -551,8 +563,7 @@
 		goto fail_put_sdev;
 	memset(cd, 0, sizeof(*cd));
 
-	kobject_init(&cd->kobj);
-	cd->kobj.ktype = &scsi_cdrom_kobj_type;
+	kref_init(&cd->kref, sr_kref_release);
 
 	disk = alloc_disk(1);
 	if (!disk)
@@ -899,18 +910,21 @@
 	return cgc->stat;
 }
 
-static void sr_kobject_release(struct kobject *kobj)
+static void sr_kref_release(struct kref *kref)
 {
-	struct scsi_cd *cd = container_of(kobj, struct scsi_cd, kobj);
+	struct scsi_cd *cd = container_of(kref, struct scsi_cd, kref);
 	struct scsi_device *sdev = cd->device;
+	struct gendisk *disk = cd->disk;
 
 	spin_lock(&sr_index_lock);
-	clear_bit(cd->disk->first_minor, sr_index_bits);
+	clear_bit(disk->first_minor, sr_index_bits);
 	spin_unlock(&sr_index_lock);
 
 	unregister_cdrom(&cd->cdi);
 
-	put_disk(cd->disk);
+	disk->private_data = NULL;
+
+	put_disk(disk);
 
 	kfree(cd);
 
===== sr.h 1.11 vs edited =====
--- 1.11/drivers/scsi/sr.h	Wed Apr  7 09:21:27 2004
+++ edited/sr.h	Fri Apr  9 09:27:42 2004
@@ -19,6 +19,7 @@
 
 #include "scsi.h"
 #include <linux/genhd.h>
+#include <linux/kref.h>
 
 /* The CDROM is fairly slow, so we need a little extra time */
 /* In fact, it is very slow if it has to spin up first */
@@ -37,8 +38,8 @@
 	unsigned readcd_cdda:1;	/* reading audio data using READ_CD */
 	struct cdrom_device_info cdi;
 	/* We hold gendisk and scsi_device references on probe and use
-	 * the refs on this kobj to decide when to release them */
-	struct kobject kobj;
+	 * the refs on this kref to decide when to release them */
+	struct kref kref;
 	struct gendisk *disk;
 } Scsi_CD;