Re: [PATCH] scsi: fix race condition when removing target

[James Bottomley <jejb@linux.vnet.ibm.com>]

On Thu, 2017-11-30 at 16:08 +0000, Bart Van Assche wrote:
> On Thu, 2017-11-30 at 09:18 +0800, Jason Yan wrote:
> > 
> > Hi Bart, I chose the approach in my patch because it has been used
> > in scsi_device_get() for years and been proved safe. I think using
> > kobject_get_unless_zero() is safe here and can fix this issue too.
> > And this approach is beneficial to all users.
> 
> Hello Jason,
> 
> A possible approach is that we start with your patch and defer any
> get_device() changes until after your patch has been applied.

It's possible, but not quite good enough: the same race can be produced
with any of our sdev lists that are deleted in the release callback,
because there could be a released device on any one of them.  The only
way to mediate it properly is to get a reference in the iterator using
kobject_get_unless_zero().

It's a bit like a huge can of worms, there's another problem every time
I look.  However, this is something like the mechanism that could work
(and if get_device() ever gets fixed, we can put it in place of
kobject_get_unless_zero()).

James

---

diff --git a/drivers/scsi/53c700.c b/drivers/scsi/53c700.c
index 6be77b3aa8a5..c3246f26c02c 100644
--- a/drivers/scsi/53c700.c
+++ b/drivers/scsi/53c700.c
@@ -1169,6 +1169,7 @@ process_script_interrupt(__u32 dsps, __u32 dsp, struct scsi_cmnd *SCp,
 
 			
 		}
+		put_device(&SDp->sdev_gendev);
 	} else if(dsps == A_RESELECTED_DURING_SELECTION) {
 
 		/* This section is full of debugging code because I've
diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index c3fc34b9964d..7736f3fb2501 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -1198,6 +1198,7 @@ static int esp_reconnect(struct esp *esp)
 		goto do_reset;
 	}
 	lp = dev->hostdata;
+	put_device(&dev->sdev_gendev);
 
 	ent = lp->non_tagged_cmd;
 	if (!ent) {
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index a7e4fba724b7..c96c11716152 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -677,11 +677,10 @@ struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *starget,
 {
 	struct scsi_device *sdev;
 
-	list_for_each_entry(sdev, &starget->devices, same_target_siblings) {
-		if (sdev->sdev_state == SDEV_DEL)
-			continue;
-		if (sdev->lun ==lun)
+	__sdev_for_each_get(sdev, &starget->devices, same_target_siblings) {
+		if (sdev->sdev_state != SDEV_DEL && sdev->lun ==lun)
 			return sdev;
+		put_device(&sdev->sdev_gendev);
 	}
 
 	return NULL;
@@ -700,15 +699,16 @@ EXPORT_SYMBOL(__scsi_device_lookup_by_target);
 struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
 						 u64 lun)
 {
-	struct scsi_device *sdev;
+  struct scsi_device *sdev, *sdev_copy;
 	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
 	unsigned long flags;
 
 	spin_lock_irqsave(shost->host_lock, flags);
-	sdev = __scsi_device_lookup_by_target(starget, lun);
+	sdev_copy = sdev = __scsi_device_lookup_by_target(starget, lun);
+	spin_unlock_irqrestore(shost->host_lock, flags);
 	if (sdev && scsi_device_get(sdev))
 		sdev = NULL;
-	spin_unlock_irqrestore(shost->host_lock, flags);
+	put_device(&sdev_copy->sdev_gendev);
 
 	return sdev;
 }
@@ -735,12 +735,12 @@ struct scsi_device *__scsi_device_lookup(struct Scsi_Host *shost,
 {
 	struct scsi_device *sdev;
 
-	list_for_each_entry(sdev, &shost->__devices, siblings) {
-		if (sdev->sdev_state == SDEV_DEL)
-			continue;
-		if (sdev->channel == channel && sdev->id == id &&
-				sdev->lun ==lun)
+	__sdev_for_each_get(sdev, &shost->__devices, siblings) {
+		if (sdev->sdev_state != SDEV_DEL &&
+		    sdev->channel == channel && sdev->id == id &&
+		    sdev->lun ==lun)
 			return sdev;
+		put_device(&sdev->sdev_gendev);
 	}
 
 	return NULL;
@@ -761,14 +761,15 @@ EXPORT_SYMBOL(__scsi_device_lookup);
 struct scsi_device *scsi_device_lookup(struct Scsi_Host *shost,
 		uint channel, uint id, u64 lun)
 {
-	struct scsi_device *sdev;
+  struct scsi_device *sdev, *sdev_copy;
 	unsigned long flags;
 
 	spin_lock_irqsave(shost->host_lock, flags);
-	sdev = __scsi_device_lookup(shost, channel, id, lun);
+	sdev_copy = sdev = __scsi_device_lookup(shost, channel, id, lun);
+	spin_unlock_irqrestore(shost->host_lock, flags);
 	if (sdev && scsi_device_get(sdev))
 		sdev = NULL;
-	spin_unlock_irqrestore(shost->host_lock, flags);
+	put_device(&sdev_copy->sdev_gendev);
 
 	return sdev;
 }
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 40124648a07b..cddd5a93e962 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1870,11 +1870,14 @@ void scsi_forget_host(struct Scsi_Host *shost)
 
  restart:
 	spin_lock_irqsave(shost->host_lock, flags);
-	list_for_each_entry(sdev, &shost->__devices, siblings) {
-		if (sdev->sdev_state == SDEV_DEL)
+	__sdev_for_each_get(sdev, &shost->__devices, siblings) {
+		if (sdev->sdev_state == SDEV_DEL) {
+			put_device(&sdev->sdev_gendev);
 			continue;
+		}
 		spin_unlock_irqrestore(shost->host_lock, flags);
 		__scsi_remove_device(sdev);
+		put_device(&sdev->sdev_gendev);
 		goto restart;
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index f796bd61f3f0..380404ec49cd 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1375,17 +1375,7 @@ static void __scsi_remove_target(struct scsi_target *starget)
 
 	spin_lock_irqsave(shost->host_lock, flags);
  restart:
-	list_for_each_entry(sdev, &shost->__devices, siblings) {
-		/*
-		 * We cannot call scsi_device_get() here, as
-		 * we might've been called from rmmod() causing
-		 * scsi_device_get() to fail the module_is_live()
-		 * check.
-		 */
-		if (sdev->channel != starget->channel ||
-		    sdev->id != starget->id ||
-		    !get_device(&sdev->sdev_gendev))
-			continue;
+	__sdev_for_each_get(sdev, &starget->devices, same_target_siblings) {
 		spin_unlock_irqrestore(shost->host_lock, flags);
 		scsi_remove_device(sdev);
 		put_device(&sdev->sdev_gendev);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 571ddb49b926..2e4d48d8cd68 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -380,6 +380,23 @@ extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
 #define __shost_for_each_device(sdev, shost) \
 	list_for_each_entry((sdev), &((shost)->__devices), siblings)
 
+/**
+ * __sdev_list_for_each_get - get a reference to each element
+ * @sdev: the scsi device to use in the body
+ * @head: the head of the list
+ * @list: the element (sdev->list) containing list members
+ *
+ * Iterator that only executes the body if it can obtain a reference
+ * to the element.  This closes a race where the device release can
+ * have been called, but the element is still on the lists.
+ *
+ * The lock protecting the list (the host lock) must be held before
+ * calling this iterator
+ */
+#define __sdev_for_each_get(sdev, head, list)				\
+	list_for_each_entry(sdev, head, list)				\
+		if (kobject_get_unless_zero(&sdev->sdev_gendev.kobj))
+
 extern int scsi_change_queue_depth(struct scsi_device *, int);
 extern int scsi_track_queue_full(struct scsi_device *, int);