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