[PATCH] fix sd open/remove race

[PATCH] fix sd open/remove race

[James Bottomley]

There's a race where open and remove may be executing simultaneously and
cause various oopses depending on who wins.

This fixes the race by ensuring that acquire/release of the scsi_disk
refcounted object are atomic with respect to each other.

James

===== drivers/scsi/sd.c 1.144 vs edited =====
--- 1.144/drivers/scsi/sd.c	Tue Mar 16 18:57:15 2004
+++ edited/drivers/scsi/sd.c	Tue Apr  6 22:20:38 2004
@@ -100,6 +100,7 @@
 
 static unsigned long sd_index_bits[SD_DISKS / BITS_PER_LONG];
 static spinlock_t sd_index_lock = SPIN_LOCK_UNLOCKED;
+DECLARE_MUTEX(sd_ref_sem);
 
 static int sd_revalidate_disk(struct gendisk *disk);
 static void sd_rw_intr(struct scsi_cmnd * SCpnt);
@@ -168,24 +169,26 @@
 	return container_of(disk->private_data, struct scsi_disk, driver);
 }
 
-static int scsi_disk_get(struct scsi_disk *sdkp)
+static struct scsi_disk *scsi_disk_get(struct gendisk *disk)
 {
-	if (!kobject_get(&sdkp->kobj))
-		goto out;
-	if (scsi_device_get(sdkp->device))
-		goto out_put_kobj;
-	return 0;
+	struct scsi_disk *sdkp = NULL;
 
-out_put_kobj:
-	kobject_put(&sdkp->kobj);
-out:
-	return -ENXIO;
+	down(&sd_ref_sem);
+	if (disk->private_data == NULL)
+		goto out;
+	sdkp = scsi_disk(disk);
+	if (!kobject_get(&sdkp->kobj))
+		sdkp = NULL;
+ out:
+	up(&sd_ref_sem);
+	return sdkp;
 }
 
 static void scsi_disk_put(struct scsi_disk *sdkp)
 {
-	scsi_device_put(sdkp->device);
+	down(&sd_ref_sem);
 	kobject_put(&sdkp->kobj);
+	up(&sd_ref_sem);
 }
 
 /**
@@ -406,15 +409,15 @@
 static int sd_open(struct inode *inode, struct file *filp)
 {
 	struct gendisk *disk = inode->i_bdev->bd_disk;
-	struct scsi_disk *sdkp = scsi_disk(disk);
+	struct scsi_disk *sdkp;
 	struct scsi_device *sdev;
 	int retval;
 
-	SCSI_LOG_HLQUEUE(3, printk("sd_open: disk=%s\n", disk->disk_name));
+	if (!(sdkp = scsi_disk_get(disk)))
+		return -ENXIO;
 
-	retval = scsi_disk_get(sdkp);
-	if (retval)
-		return retval;
+
+	SCSI_LOG_HLQUEUE(3, printk("sd_open: disk=%s\n", disk->disk_name));
 
 	sdev = sdkp->device;
 
@@ -1338,13 +1341,16 @@
 	if ((sdp->type != TYPE_DISK) && (sdp->type != TYPE_MOD))
 		goto out;
 
+	if ((error = scsi_device_get(sdp)) != 0)
+		goto out;
+
 	SCSI_LOG_HLQUEUE(3, printk("sd_attach: scsi device: <%d,%d,%d,%d>\n", 
 			 sdp->host->host_no, sdp->channel, sdp->id, sdp->lun));
 
 	error = -ENOMEM;
 	sdkp = kmalloc(sizeof(*sdkp), GFP_KERNEL);
 	if (!sdkp)
-		goto out;
+		goto out_put_sdev;
 
 	memset (sdkp, 0, sizeof(*sdkp));
 	kobject_init(&sdkp->kobj);
@@ -1421,6 +1427,8 @@
 	put_disk(gd);
 out_free:
 	kfree(sdkp);
+out_put_sdev:
+	scsi_device_put(sdp);
 out:
 	return error;
 }
@@ -1454,14 +1462,20 @@
 static void scsi_disk_release(struct kobject *kobj)
 {
 	struct scsi_disk *sdkp = to_scsi_disk(kobj);
+	struct scsi_device *sdev = sdkp->device;
+	struct gendisk *disk = sdkp->disk;
 	
-	put_disk(sdkp->disk);
-
 	spin_lock(&sd_index_lock);
 	clear_bit(sdkp->index, sd_index_bits);
 	spin_unlock(&sd_index_lock);
 
+	disk->private_data = NULL;
+
+	put_disk(disk);
+
 	kfree(sdkp);
+
+	scsi_device_put(sdev);
 }
 
 /*

Re: [PATCH] fix sd open/remove race

[Alan Stern]

On Thu, 8 Apr 2004, Patrick Mansfield wrote:

> James posted this sd.c patch to fix the open/remove race:
> 
> http://marc.theaimsgroup.com/?l=linux-scsi&m=108130838613268&w=2

The patch is almost but not quite correct.  It acquires the new semaphore 
unnecessarily during scsi_disk_put, it fails to acquire it when erasing 
the gendisk private_data, and it needs to erase the private_data during 
sd_remove (not scsi_disk_release).  It also contains a syntax error.

This patch, applied on top of James's, should fix things up.  It compiles 
okay, but I haven't tried running it.

Alan Stern


--- 2.6/drivers/scsi/sd.c.orig	Fri Apr  9 10:01:22 2004
+++ 2.6/drivers/scsi/sd.c	Fri Apr  9 10:01:13 2004
@@ -162,7 +162,7 @@
 
 /* reverse mapping dev -> (sd_nr, part) not currently needed */
 
-#define to_scsi_disk(obj) container_of(obj,struct scsi_disk,kobj);
+#define to_scsi_disk(obj) container_of(obj,struct scsi_disk,kobj)
 
 static inline struct scsi_disk *scsi_disk(struct gendisk *disk)
 {
@@ -186,9 +186,7 @@
 
 static void scsi_disk_put(struct scsi_disk *sdkp)
 {
-	down(&sd_ref_sem);
 	kobject_put(&sdkp->kobj);
-	up(&sd_ref_sem);
 }
 
 /**
@@ -1442,6 +1440,10 @@
 {
 	struct scsi_disk *sdkp = dev_get_drvdata(dev);
 
+	down(&sd_ref_sem);
+	sdkp->disk->private_data = NULL;
+	up(&sd_ref_sem);
+
 	del_gendisk(sdkp->disk);
 	sd_shutdown(dev);
 	kobject_put(&sdkp->kobj);
@@ -1457,15 +1459,12 @@
 {
 	struct scsi_disk *sdkp = to_scsi_disk(kobj);
 	struct scsi_device *sdev = sdkp->device;
-	struct gendisk *disk = sdkp->disk;
 	
 	spin_lock(&sd_index_lock);
 	clear_bit(sdkp->index, sd_index_bits);
 	spin_unlock(&sd_index_lock);
 
-	disk->private_data = NULL;
-
-	put_disk(disk);
+	put_disk(sdkp->disk);
 
 	kfree(sdkp);
 

Re: [PATCH] fix sd open/remove race

[James Bottomley]

On Fri, 2004-04-09 at 09:11, Alan Stern wrote:
> The patch is almost but not quite correct.  It acquires the new semaphore 
> unnecessarily during scsi_disk_put, it fails to acquire it when erasing 
> the gendisk private_data, and it needs to erase the private_data during 
> sd_remove (not scsi_disk_release).  It also contains a syntax error.

Heh, the additional semicolon I'll remove.

However, the rest of your patch is wrong.

The reason is that what you're trying to synchronise is the naming
lookup (the gendisk open).  You have to allow currently open devices to
continue to operate.  NULL'ing out the disk->private_data before
del_gendisk would mean it was null for sd_release, so every attempt to
close the device after removal would oops.  That's why the NULL must be
done in the reference release routine, not in the disconnection
indication routine.  The race you're trying to prevent is the 0->1
reference after disconnection.

The original patch, By the way, did hold the sd_ref_sem around the NULL
of disk->private_data because the scsi_disk_release is triggered from
kobject_put on last put (and thus is under the semaphore).

James

Re: [PATCH] fix sd open/remove race

[Alan Stern]

On 9 Apr 2004, James Bottomley wrote:

> Heh, the additional semicolon I'll remove.

I'm glad somethings in life aren't controversial! :-)

> However, the rest of your patch is wrong.
> 
> The reason is that what you're trying to synchronise is the naming
> lookup (the gendisk open).  You have to allow currently open devices to
> continue to operate.  NULL'ing out the disk->private_data before
> del_gendisk would mean it was null for sd_release, so every attempt to
> close the device after removal would oops.  That's why the NULL must be
> done in the reference release routine, not in the disconnection
> indication routine.  The race you're trying to prevent is the 0->1
> reference after disconnection.

You're right.  However, you still want calls to sd_open to start failing
as soon as the disconnection notice is received.  Since the
disk-unavailable state can't be indicated by clearing disk->private_data,
it has to be indicated in some other way.  How to do it is up to you, but
whatever way you choose, the indicator should be cleared in sd_remove (and
that change must be synchronized by acquiring the new semaphore).

> The original patch, By the way, did hold the sd_ref_sem around the NULL
> of disk->private_data because the scsi_disk_release is triggered from
> kobject_put on last put (and thus is under the semaphore).

Ah, yes.  However there's no reason to hold the semaphore _every_ time you 
do a scsi_disk_put.  And once you use something else as an indicator, 
there won't be any need to clear disk->private_data.

Alan Stern

Re: [PATCH] fix sd open/remove race

[James Bottomley]

On Fri, 2004-04-09 at 10:24, Alan Stern wrote:
> You're right.  However, you still want calls to sd_open to start failing
> as soon as the disconnection notice is received.  Since the
> disk-unavailable state can't be indicated by clearing disk->private_data,
> it has to be indicated in some other way.  How to do it is up to you, but
> whatever way you choose, the indicator should be cleared in sd_remove (and
> that change must be synchronized by acquiring the new semaphore).

That's what the original bug2400 thread was all about.  I think opening
with an already degraded device (one that will EIO to everything) is
just as good as refusing the open.  That's why the only race is the
reference race.  It's better to refuse the open, but the effects of
either are the same.

> Ah, yes.  However there's no reason to hold the semaphore _every_ time you 
> do a scsi_disk_put.  And once you use something else as an indicator, 
> there won't be any need to clear disk->private_data.

Actually, there is a (very obscure) three thread SMP race that this
prevents.  Thread 1 -> sd_remove; Thread 2 -> sd_close (for last close)
and Thread 3 -> sd_open

Thread 2 might end up doing the last put and if it were not protected it
would still race with sd_open.

James

Re: [PATCH] fix sd open/remove race

[Alan Stern]

On 9 Apr 2004, James Bottomley wrote:

> On Fri, 2004-04-09 at 10:24, Alan Stern wrote:
> > You're right.  However, you still want calls to sd_open to start failing
> > as soon as the disconnection notice is received.  Since the
> > disk-unavailable state can't be indicated by clearing disk->private_data,
> > it has to be indicated in some other way.  How to do it is up to you, but
> > whatever way you choose, the indicator should be cleared in sd_remove (and
> > that change must be synchronized by acquiring the new semaphore).
> 
> That's what the original bug2400 thread was all about.  I think opening
> with an already degraded device (one that will EIO to everything) is
> just as good as refusing the open.  That's why the only race is the
> reference race.  It's better to refuse the open, but the effects of
> either are the same.

Okay, yes, that will work either way.

> > Ah, yes.  However there's no reason to hold the semaphore _every_ time you 
> > do a scsi_disk_put.  And once you use something else as an indicator, 
> > there won't be any need to clear disk->private_data.
> 
> Actually, there is a (very obscure) three thread SMP race that this
> prevents.  Thread 1 -> sd_remove; Thread 2 -> sd_close (for last close)
> and Thread 3 -> sd_open
> 
> Thread 2 might end up doing the last put and if it were not protected it
> would still race with sd_open.

Hmm.  That wouldn't matter if you cleared the indicator in sd_remove.  
Then the race would only be sd_remove against sd_open.  Also, you do have
to admit, your way of doing it is more obscure.  You should at least add
comments explaining this 3-way race and the fact that the semaphore is
already acquired during scsi_disk_release.

Alan Stern

Re: [PATCH] fix sd open/remove race

[James Bottomley]

On Fri, 2004-04-09 at 10:55, Alan Stern wrote:
> Hmm.  That wouldn't matter if you cleared the indicator in sd_remove.  
> Then the race would only be sd_remove against sd_open.  Also, you do have
> to admit, your way of doing it is more obscure.  You should at least add
> comments explaining this 3-way race and the fact that the semaphore is
> already acquired during scsi_disk_release.

But, as I pointed out before, you can't clear the indicator in sd_remove
because, fundamentally, that's not where the race is.  The fundamental
race is the get/put one making sure no-one can get a reference to a
freed object.

sd_remove is just the disconnection indication.  It triggers a put of
the object, causing garbage collection if it's the last put, but that's
all it does.  To mediate the race, the synchronisation has to be around
object acquisition/deallocation.

I can add comments, but the placement of the semaphores is natural to
mediate the object acquisition/deallocation race.  The three thread
illustration was just the first thing that came into my head for
explaining what problems could be caused by not doing natural semaphore
placement.

James

Re: [PATCH] fix sd open/remove race

[Alan Stern]

On 9 Apr 2004, James Bottomley wrote:

> But, as I pointed out before, you can't clear the indicator in sd_remove
> because, fundamentally, that's not where the race is.  The fundamental
> race is the get/put one making sure no-one can get a reference to a
> freed object.
> 
> sd_remove is just the disconnection indication.  It triggers a put of
> the object, causing garbage collection if it's the last put, but that's
> all it does.  To mediate the race, the synchronisation has to be around
> object acquisition/deallocation.

I understand you now.  It's really a matter of what one is trying to 
protect by the synchronization:

	You want to prevent anyone from acquiring a reference to the
	object after it has been deallocated.  Hence you use the
	semaphore around the get and the put.

	I want to prevent anyone from acquiring a reference to the
	object after the driver has decided that no more references
	should be given out, i.e., after sd_remove has been called.
	Hence I use the semaphore around the get and in the remove.

Both approaches are valid.


There's a somewhat similar problem that arises in the relation between the
SCSI core and the low-level drivers.  Here the data structure in question
is the host template.  (Yes, LLDs don't normally deallocate their host
templates, but if they are built as modules they can be unloaded from
memory, which has the same effect.)  After calling scsi_remove_host() the
LLD has no way to know when the SCSI core is finished using the template.

Mike Anderson has suggested that scsi_remove_host could replace the hostt
pointer with a pointer to a dummy template that will fail every operation.
There may still be difficulties involving the procfs interface, though.

Alan Stern

Re: [PATCH] fix sd open/remove race

[James Bottomley]

On Fri, 2004-04-09 at 13:29, Alan Stern wrote:
> 	I want to prevent anyone from acquiring a reference to the
> 	object after the driver has decided that no more references
> 	should be given out, i.e., after sd_remove has been called.
> 	Hence I use the semaphore around the get and in the remove.

Well, but I think you're going to spend a lot of effort trying to do
this.  I'm not convinced it's worth it.

Think of the CD ROM unplug that started all this.  Supposing the user
opens the CD, waits a minute, disconnects it then tries to use the open
fd.  Both of our schemes are forced to keep the object around giving EIO
until the user gets bored and closes it.

since we have to support the open degraded object anyway, why worry
about mediating open/disconnect races the only result of which would be
to refuse the open if it occurs within the correct window with the
disconnect?

> There's a somewhat similar problem that arises in the relation between the
> SCSI core and the low-level drivers.  Here the data structure in question
> is the host template.  (Yes, LLDs don't normally deallocate their host
> templates, but if they are built as modules they can be unloaded from
> memory, which has the same effect.)  After calling scsi_remove_host() the
> LLD has no way to know when the SCSI core is finished using the template.
> 
> Mike Anderson has suggested that scsi_remove_host could replace the hostt
> pointer with a pointer to a dummy template that will fail every operation.
> There may still be difficulties involving the procfs interface, though.

Yes, well, the host needs a proper lifecycle state model to begin with. 
I'm afraid I've been concentrating on getting the device sorted out
first.

James

Re: [PATCH] fix sd open/remove race

[Alan Stern]

On 9 Apr 2004, James Bottomley wrote:

> On Fri, 2004-04-09 at 13:29, Alan Stern wrote:
> > 	I want to prevent anyone from acquiring a reference to the
> > 	object after the driver has decided that no more references
> > 	should be given out, i.e., after sd_remove has been called.
> > 	Hence I use the semaphore around the get and in the remove.
> 
> Well, but I think you're going to spend a lot of effort trying to do
> this.  I'm not convinced it's worth it.

If it was possible to set disk->private_data to NULL within sd_remove, I'd 
argue that it's no more effort than what you've done.  As it is though, 
I'm forced to agree.

Alan Stern