[SCSI] fix async scan add/remove race resulting in an oops

Async scanning introduced a very wide window where the SCSI device is up and running but has not yet been added to sysfs. We delay the adding until all scans have completed to retain the same ordering as sync scanning. This delay in visibility causes an oops if a device is removed before we make it visible because the SCSI removal routines have an inbuilt assumption that if a device is in SDEV_RUNNING state, it must be visible (which is not necessarily true in the async scanning case). Fix this by introducing an additional is_visible flag which we can use to condition the tear down so we do the right thing for running but not yet made visible. Reported-by: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> Signed-off-by: James Bottomley <James.Bottomley@suse.de>

[SCSI] fix async scan add/remove race resulting in an oops
Async scanning introduced a very wide window where the SCSI device is up and running but has not yet been added to sysfs. We delay the adding until all scans have completed to retain the same ordering as sync scanning. This delay in visibility causes an oops if a device is removed before we make it visible because the SCSI removal routines have an inbuilt assumption that if a device is in SDEV_RUNNING state, it must be visible (which is not necessarily true in the async scanning case). Fix this by introducing an additional is_visible flag which we can use to condition the tear down so we do the right thing for running but not yet made visible. Reported-by: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> Signed-off-by: James Bottomley <James.Bottomley@suse.de>
James Bottomley
1 parent 3bf3583b6a
Showing 3 changed files with 32 additions and 50 deletions Side-by-side Diff
drivers/scsi/scsi_scan.c
drivers/scsi/scsi_sysfs.c
include/scsi/scsi_device.h
@@ -952,16 +952,6 @@
 	return SCSI_SCAN_LUN_PRESENT;
 }
  
-static inline void scsi_destroy_sdev(struct scsi_device *sdev)
-{
-	scsi_device_set_state(sdev, SDEV_DEL);
-	if (sdev->host->hostt->slave_destroy)
-		sdev->host->hostt->slave_destroy(sdev);
-	transport_destroy_device(&sdev->sdev_gendev);
-	put_device(&sdev->sdev_dev);
-	put_device(&sdev->sdev_gendev);
-}
-
 #ifdef CONFIG_SCSI_LOGGING
 /** 
  * scsi_inq_str - print INQUIRY data from min to max index, strip trailing whitespace
@@ -1139,7 +1129,7 @@
 			}
 		}
 	} else
-		scsi_destroy_sdev(sdev);
+		__scsi_remove_device(sdev);
  out:
 	return res;
 }
@@ -1500,7 +1490,7 @@
 		/*
 		 * the sdev we used didn't appear in the report luns scan
 		 */
-		scsi_destroy_sdev(sdev);
+		__scsi_remove_device(sdev);
 	return ret;
 }
  
@@ -1710,7 +1700,7 @@
 	shost_for_each_device(sdev, shost) {
 		if (!scsi_host_scan_allowed(shost) ||
 		    scsi_sysfs_add_sdev(sdev) != 0)
-			scsi_destroy_sdev(sdev);
+			__scsi_remove_device(sdev);
 	}
 }
  
@@ -1943,7 +1933,7 @@
 {
 	BUG_ON(sdev->id != sdev->host->this_id);
  
-	scsi_destroy_sdev(sdev);
+	__scsi_remove_device(sdev);
 }
 EXPORT_SYMBOL(scsi_free_host_dev);
@@ -854,82 +854,73 @@
 	transport_configure_device(&starget->dev);
 	error = device_add(&sdev->sdev_gendev);
 	if (error) {
-		put_device(sdev->sdev_gendev.parent);
 		printk(KERN_INFO "error 1\n");
-		return error;
+		goto out_remove;
 	}
 	error = device_add(&sdev->sdev_dev);
 	if (error) {
 		printk(KERN_INFO "error 2\n");
-		goto clean_device;
+		device_del(&sdev->sdev_gendev);
+		goto out_remove;
 	}
+	transport_add_device(&sdev->sdev_gendev);
+	sdev->is_visible = 1;
  
 	/* create queue files, which may be writable, depending on the host */
 	if (sdev->host->hostt->change_queue_depth)
 		error = device_create_file(&sdev->sdev_gendev, &sdev_attr_queue_depth_rw);
 	else
 		error = device_create_file(&sdev->sdev_gendev, &dev_attr_queue_depth);
-	if (error) {
-		__scsi_remove_device(sdev);
-		goto out;
-	}
+	if (error)
+		goto out_remove;
+
 	if (sdev->host->hostt->change_queue_type)
 		error = device_create_file(&sdev->sdev_gendev, &sdev_attr_queue_type_rw);
 	else
 		error = device_create_file(&sdev->sdev_gendev, &dev_attr_queue_type);
-	if (error) {
-		__scsi_remove_device(sdev);
-		goto out;
-	}
+	if (error)
+		goto out_remove;
  
 	error = bsg_register_queue(rq, &sdev->sdev_gendev, NULL, NULL);
  
 	if (error)
+		/* we're treating error on bsg register as non-fatal,
+		 * so pretend nothing went wrong */
 		sdev_printk(KERN_INFO, sdev,
 			    "Failed to register bsg queue, errno=%d\n", error);
  
-	/* we're treating error on bsg register as non-fatal, so pretend
-	 * nothing went wrong */
-	error = 0;
-
 	/* add additional host specific attributes */
 	if (sdev->host->hostt->sdev_attrs) {
 		for (i = 0; sdev->host->hostt->sdev_attrs[i]; i++) {
 			error = device_create_file(&sdev->sdev_gendev,
 					sdev->host->hostt->sdev_attrs[i]);
-			if (error) {
-				__scsi_remove_device(sdev);
-				goto out;
-			}
+			if (error)
+				goto out_remove;
 		}
 	}
  
-	transport_add_device(&sdev->sdev_gendev);
- out:
-	return error;
+	return 0;
  
- clean_device:
-	scsi_device_set_state(sdev, SDEV_CANCEL);
-
-	device_del(&sdev->sdev_gendev);
-	transport_destroy_device(&sdev->sdev_gendev);
-	put_device(&sdev->sdev_dev);
-	put_device(&sdev->sdev_gendev);
-
+ out_remove:
+	__scsi_remove_device(sdev);
 	return error;
+
 }
  
 void __scsi_remove_device(struct scsi_device *sdev)
 {
 	struct device *dev = &sdev->sdev_gendev;
  
-	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
-		return;
+	if (sdev->is_visible) {
+		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
+			return;
  
-	bsg_unregister_queue(sdev->request_queue);
-	device_unregister(&sdev->sdev_dev);
-	transport_remove_device(dev);
-	device_del(dev);
+		bsg_unregister_queue(sdev->request_queue);
+		device_unregister(&sdev->sdev_dev);
+		transport_remove_device(dev);
+		device_del(dev);
+	} else
+		put_device(&sdev->sdev_dev);
 	scsi_device_set_state(sdev, SDEV_DEL);
 	if (sdev->host->hostt->slave_destroy)
 		sdev->host->hostt->slave_destroy(sdev);
@@ -145,6 +145,7 @@
 	unsigned retry_hwerror:1;	/* Retry HARDWARE_ERROR */
 	unsigned last_sector_bug:1;	/* do not use multisector accesses on
 					   SD_LAST_BUGGY_SECTORS */
+	unsigned is_visible:1;	/* is the device visible in sysfs */
  
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
...	...	@@ -952,16 +952,6 @@
952	952	return SCSI_SCAN_LUN_PRESENT;
953	953	}
954	954
955		-static inline void scsi_destroy_sdev(struct scsi_device *sdev)
956		-{
957		- scsi_device_set_state(sdev, SDEV_DEL);
958		- if (sdev->host->hostt->slave_destroy)
959		- sdev->host->hostt->slave_destroy(sdev);
960		- transport_destroy_device(&sdev->sdev_gendev);
961		- put_device(&sdev->sdev_dev);
962		- put_device(&sdev->sdev_gendev);
963		-}
964		-
965	955	#ifdef CONFIG_SCSI_LOGGING
966	956	/**
967	957	* scsi_inq_str - print INQUIRY data from min to max index, strip trailing whitespace
...	...	@@ -1139,7 +1129,7 @@
1139	1129	}
1140	1130	}
1141	1131	} else
1142		- scsi_destroy_sdev(sdev);
	1132	+ __scsi_remove_device(sdev);
1143	1133	out:
1144	1134	return res;
1145	1135	}
...	...	@@ -1500,7 +1490,7 @@
1500	1490	/*
1501	1491	* the sdev we used didn't appear in the report luns scan
1502	1492	*/
1503		- scsi_destroy_sdev(sdev);
	1493	+ __scsi_remove_device(sdev);
1504	1494	return ret;
1505	1495	}
1506	1496
...	...	@@ -1710,7 +1700,7 @@
1710	1700	shost_for_each_device(sdev, shost) {
1711	1701	if (!scsi_host_scan_allowed(shost) \|\|
1712	1702	scsi_sysfs_add_sdev(sdev) != 0)
1713		- scsi_destroy_sdev(sdev);
	1703	+ __scsi_remove_device(sdev);
1714	1704	}
1715	1705	}
1716	1706
...	...	@@ -1943,7 +1933,7 @@
1943	1933	{
1944	1934	BUG_ON(sdev->id != sdev->host->this_id);
1945	1935
1946		- scsi_destroy_sdev(sdev);
	1936	+ __scsi_remove_device(sdev);
1947	1937	}
1948	1938	EXPORT_SYMBOL(scsi_free_host_dev);
...	...	@@ -854,82 +854,73 @@
854	854	transport_configure_device(&starget->dev);
855	855	error = device_add(&sdev->sdev_gendev);
856	856	if (error) {
857		- put_device(sdev->sdev_gendev.parent);
858	857	printk(KERN_INFO "error 1\n");
859		- return error;
	858	+ goto out_remove;
860	859	}
861	860	error = device_add(&sdev->sdev_dev);
862	861	if (error) {
863	862	printk(KERN_INFO "error 2\n");
864		- goto clean_device;
	863	+ device_del(&sdev->sdev_gendev);
	864	+ goto out_remove;
865	865	}
	866	+ transport_add_device(&sdev->sdev_gendev);
	867	+ sdev->is_visible = 1;
866	868
867	869	/* create queue files, which may be writable, depending on the host */
868	870	if (sdev->host->hostt->change_queue_depth)
869	871	error = device_create_file(&sdev->sdev_gendev, &sdev_attr_queue_depth_rw);
870	872	else
871	873	error = device_create_file(&sdev->sdev_gendev, &dev_attr_queue_depth);
872		- if (error) {
873		- __scsi_remove_device(sdev);
874		- goto out;
875		- }
	874	+ if (error)
	875	+ goto out_remove;
	876	+
876	877	if (sdev->host->hostt->change_queue_type)
877	878	error = device_create_file(&sdev->sdev_gendev, &sdev_attr_queue_type_rw);
878	879	else
879	880	error = device_create_file(&sdev->sdev_gendev, &dev_attr_queue_type);
880		- if (error) {
881		- __scsi_remove_device(sdev);
882		- goto out;
883		- }
	881	+ if (error)
	882	+ goto out_remove;
884	883
885	884	error = bsg_register_queue(rq, &sdev->sdev_gendev, NULL, NULL);
886	885
887	886	if (error)
	887	+ /* we're treating error on bsg register as non-fatal,
	888	+ * so pretend nothing went wrong */
888	889	sdev_printk(KERN_INFO, sdev,
889	890	"Failed to register bsg queue, errno=%d\n", error);
890	891
891		- /* we're treating error on bsg register as non-fatal, so pretend
892		- * nothing went wrong */
893		- error = 0;
894		-
895	892	/* add additional host specific attributes */
896	893	if (sdev->host->hostt->sdev_attrs) {
897	894	for (i = 0; sdev->host->hostt->sdev_attrs[i]; i++) {
898	895	error = device_create_file(&sdev->sdev_gendev,
899	896	sdev->host->hostt->sdev_attrs[i]);
900		- if (error) {
901		- __scsi_remove_device(sdev);
902		- goto out;
903		- }
	897	+ if (error)
	898	+ goto out_remove;
904	899	}
905	900	}
906	901
907		- transport_add_device(&sdev->sdev_gendev);
908		- out:
909		- return error;
	902	+ return 0;
910	903
911		- clean_device:
912		- scsi_device_set_state(sdev, SDEV_CANCEL);
913		-
914		- device_del(&sdev->sdev_gendev);
915		- transport_destroy_device(&sdev->sdev_gendev);
916		- put_device(&sdev->sdev_dev);
917		- put_device(&sdev->sdev_gendev);
918		-
	904	+ out_remove:
	905	+ __scsi_remove_device(sdev);
919	906	return error;
	907	+
920	908	}
921	909
922	910	void __scsi_remove_device(struct scsi_device *sdev)
923	911	{
924	912	struct device *dev = &sdev->sdev_gendev;
925	913
926		- if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
927		- return;
	914	+ if (sdev->is_visible) {
	915	+ if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
	916	+ return;
928	917
929		- bsg_unregister_queue(sdev->request_queue);
930		- device_unregister(&sdev->sdev_dev);
931		- transport_remove_device(dev);
932		- device_del(dev);
	918	+ bsg_unregister_queue(sdev->request_queue);
	919	+ device_unregister(&sdev->sdev_dev);
	920	+ transport_remove_device(dev);
	921	+ device_del(dev);
	922	+ } else
	923	+ put_device(&sdev->sdev_dev);
933	924	scsi_device_set_state(sdev, SDEV_DEL);
934	925	if (sdev->host->hostt->slave_destroy)
935	926	sdev->host->hostt->slave_destroy(sdev);
...	...	@@ -145,6 +145,7 @@
145	145	unsigned retry_hwerror:1; /* Retry HARDWARE_ERROR */
146	146	unsigned last_sector_bug:1; /* do not use multisector accesses on
147	147	SD_LAST_BUGGY_SECTORS */
	148	+ unsigned is_visible:1; /* is the device visible in sysfs */
148	149
149	150	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
150	151	struct list_head event_list; /* asserted events */