On Thu, 2005-11-03 at 10:54 -0500, Alan Stern wrote:
> When the well-known open/remove race was fixed in sd.c, a closely-related
> race between rescan and remove was ignored. Probably because rescan gets
> called from an unusual pathway (via sysfs). It's easy for this race to
> trigger an oops, especially if you unplug a USB storage device while a
> user program continually writes to the "rescan" attribute file.
>
> This patch (as597) fixes the race by making the rescan routine acquire the
> appropriate references. It also cleans up the code scsi_disk_get
> somewhat. This resolves Bugzilla entry #5237.
Thanks for investigating this ... I really hate debugging these obscure
object reference races.
I'm afraid that your fix isn't quite right: The problem occurs because
in most cases scsi_remove_device will call sd_remove (via the driver
model call backs from device_del). The kref_put in sd_remove will
(almost always) trigger scsi_disk_release which frees sdkp. So, I'm
afraid you can't rely on sdkp->device being correct.
The fix for this one is a bit nasty because it's a reversal in the
logic. The usual logic is that the user does something, so we have a
refcounted gendisk object that we tie to scsi_disk and then scsi_device.
In this case, we actually get a scsi_device and we have to try to work
out how to get a valid refcounted scsi_disk object out of it.
Unfortunately, this problem doesn't just affect sd_rescan, it affects
the flush functions too where they try to do the same thing.
Could you take a look at the attached? I just threw it together on a
'plane, so it's completely untested. It tries to implement the secure
get of struct scsi_disk by tying the nulling out of drvdata to the
sd_ref_sem, so we can now either get a reference to the scsi_disk or
return NULL. I also fixed all the other relevant driver data users to
follow this model.
Thanks,
James
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -177,24 +177,38 @@ static inline struct scsi_disk *scsi_dis
return container_of(disk->private_data, struct scsi_disk, driver);
}
-static struct scsi_disk *scsi_disk_get(struct gendisk *disk)
+static struct scsi_disk *__scsi_disk_get(struct gendisk *disk)
{
struct scsi_disk *sdkp = NULL;
- down(&sd_ref_sem);
if (disk->private_data == NULL)
goto out;
sdkp = scsi_disk(disk);
+ if (scsi_device_get(sdkp->device)) {
+ sdkp = NULL;
+ goto out;
+ }
kref_get(&sdkp->kref);
- if (scsi_device_get(sdkp->device))
- goto out_put;
+ out:
+ return sdkp;
+}
+
+static struct scsi_disk *scsi_disk_get(struct gendisk *disk)
+{
+ struct scsi_disk *sdkp;
+ down(&sd_ref_sem);
+ sdkp = __scsi_disk_get(disk);
up(&sd_ref_sem);
return sdkp;
+}
- out_put:
- kref_put(&sdkp->kref, scsi_disk_release);
- sdkp = NULL;
- out:
+static struct scsi_disk *scsi_disk_get_from_dev(struct device *dev)
+{
+ struct scsi_disk *sdkp;
+ down(&sd_ref_sem);
+ sdkp = dev_get_drvdata(dev);
+ if (sdkp)
+ sdkp = __scsi_disk_get(sdkp->disk);
up(&sd_ref_sem);
return sdkp;
}
@@ -716,16 +730,20 @@ static int sd_sync_cache(struct scsi_dev
static int sd_issue_flush(struct device *dev, sector_t *error_sector)
{
+ int ret = 0;
struct scsi_device *sdp = to_scsi_device(dev);
- struct scsi_disk *sdkp = dev_get_drvdata(dev);
+ struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
if (!sdkp)
return -ENODEV;
if (!sdkp->WCE)
- return 0;
+ goto out;
- return sd_sync_cache(sdp);
+ ret = sd_sync_cache(sdp);
+ out:
+ scsi_disk_put(sdkp);
+ return ret;
}
static void sd_end_flush(request_queue_t *q, struct request *flush_rq)
@@ -754,13 +772,14 @@ static void sd_end_flush(request_queue_t
static int sd_prepare_flush(request_queue_t *q, struct request *rq)
{
struct scsi_device *sdev = q->queuedata;
- struct scsi_disk *sdkp = dev_get_drvdata(&sdev->sdev_gendev);
+ struct scsi_disk *sdkp = scsi_disk_get_from_dev(&sdev->sdev_gendev);
- if (sdkp->WCE) {
+ if (sdkp && sdkp->WCE) {
memset(rq->cmd, 0, sizeof(rq->cmd));
rq->flags |= REQ_BLOCK_PC | REQ_SOFTBARRIER;
rq->timeout = SD_TIMEOUT;
rq->cmd[0] = SYNCHRONIZE_CACHE;
+ scsi_disk_put(sdkp);
return 1;
}
@@ -769,8 +788,11 @@ static int sd_prepare_flush(request_queu
static void sd_rescan(struct device *dev)
{
- struct scsi_disk *sdkp = dev_get_drvdata(dev);
- sd_revalidate_disk(sdkp->disk);
+ struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
+ if (sdkp) {
+ sd_revalidate_disk(sdkp->disk);
+ scsi_disk_put(sdkp);
+ }
}
@@ -1641,6 +1663,7 @@ static int sd_remove(struct device *dev)
del_gendisk(sdkp->disk);
sd_shutdown(dev);
down(&sd_ref_sem);
+ dev_set_drvdata(dev, NULL);
kref_put(&sdkp->kref, scsi_disk_release);
up(&sd_ref_sem);
@@ -1680,18 +1703,20 @@ static void scsi_disk_release(struct kre
static void sd_shutdown(struct device *dev)
{
struct scsi_device *sdp = to_scsi_device(dev);
- struct scsi_disk *sdkp = dev_get_drvdata(dev);
+ struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
if (!sdkp)
return; /* this can happen */
if (!sdkp->WCE)
- return;
+ goto out;
printk(KERN_NOTICE "Synchronizing SCSI cache for disk %s: \n",
sdkp->disk->disk_name);
sd_sync_cache(sdp);
-}
+ out:
+ scsi_disk_put(sdkp);
+}
/**
* init_sd - entry point for this driver (both when built in or when
-
: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
