scsi_aen: fix event locking problems

PATCH In review James Bottomley 2 participants 3 messages Oct 28, 2007 SCSI

Share & tools

James Bottomley

author

2007-10-28 19:38 UTC #0 lore raw

PATCH 1/1 scsi_aen: fix event locking problems

There was a fundamental flaw in the initial code, in that it had no locking around scsi_device_event_notify(). This is a problem because execute_in_process_context() also has no locking (it expects the caller to do the work). This means that two events firing simultaneously could potentially corrupt each other. This might be unlikely for media change events, but it will become more likely as we use this call as the engine for other SCSI AENs.

The best fix looks to be simply not to use sdev->ew for the source of the execute_work queue. Further, since kzalloc() always has to give us at least 32 bytes and we're doing an allocation anyway, it's free to shove the execute_work structure into scsi_device_event_info and eliminate all of the other locking and the work queue list.

I also separated the aen headers out into scsi_aen.h and cleaned up the uevent processing (there's no real need to define the strings anywhere other than in scsi_aen_uevent_notifier() ... especially as I envisage there will be more complex strings for some SCSI events.

Finally, I exported and made available scsi_aen_chain for the things that will attach to it.

James

include/scsi/scsi_device.h +0 ~~-13~~

Index: BUILD-2.6/include/scsi/scsi_device.h
===================================================================
@@ -46,16 +46,6 @@ enum scsi_device_state {
 				 * to the scsi lld. */
 };
 
-/* must match scsi_device_event_strings in scsi_lib.c */
-enum scsi_device_event {
-	SDEV_MEDIA_CHANGE = 1,	/* media has changed */
-};
-
-struct scsi_device_event_info {
-	enum scsi_device_event event;
-	struct list_head link;
-};
-
 struct scsi_device {
 	struct Scsi_Host *host;
 	struct request_queue *request_queue;

@@ -154,7 +144,6 @@ struct scsi_device {
 	struct execute_work	ew; /* used to get process context on put */
 
 	enum scsi_device_state sdev_state;
-	struct list_head	sdev_event_list;
 	unsigned long		sdev_data[0];
 } __attribute__((aligned(sizeof(unsigned long))));
 #define	to_scsi_device(d)	\

@@ -286,8 +275,6 @@ extern int scsi_test_unit_ready(struct s
 				int retries);
 extern int scsi_device_set_state(struct scsi_device *sdev,
 				 enum scsi_device_state state);
-extern int scsi_device_event_notify(struct scsi_device *sdev,
-				enum scsi_device_event event);
 extern int scsi_device_quiesce(struct scsi_device *sdev);
 extern void scsi_device_resume(struct scsi_device *sdev);
 extern void scsi_target_quiesce(struct scsi_target *);

drivers/scsi/scsi_aen.c +26 ~~-56~~

133 lines not on this page · raw message

include/scsi/scsi_aen.h new file +24 -0

Index: BUILD-2.6/include/scsi/scsi_aen.h
===================================================================
@@ -0,0 +1,24 @@
+#ifndef _SCSI_SCSI_AEN_H
+#define _SCSI_SCSI_AEN_H
+
+#include <linux/workqueue.h>
+
+struct scsi_device;
+
+/* must match the event environment in scsi_aen.c:scsi_aen_uevent_notifier */
+enum scsi_device_event {
+	SDEV_MEDIA_CHANGE = 1,	/* media has changed */
+};
+
+struct scsi_device_event_info {
+	enum scsi_device_event event;
+	struct scsi_device *sdev;
+	struct execute_work ew;
+};
+
+extern struct blocking_notifier_head scsi_aen_chain;
+
+extern int scsi_device_event_notify(struct scsi_device *sdev,
+				    enum scsi_device_event event);
+
+#endif

drivers/scsi/scsi_scan.c +0 -1

Index: BUILD-2.6/drivers/scsi/scsi_scan.c
===================================================================
@@ -254,7 +254,6 @@ static struct scsi_device *scsi_alloc_sd
 	INIT_LIST_HEAD(&sdev->same_target_siblings);
 	INIT_LIST_HEAD(&sdev->cmd_list);
 	INIT_LIST_HEAD(&sdev->starved_entry);
-	INIT_LIST_HEAD(&sdev->sdev_event_list);
 	spin_lock_init(&sdev->list_lock);
 
 	sdev->sdev_gendev.parent = get_device(&starget->dev);

drivers/scsi/sd.c +1 -0

Index: BUILD-2.6/drivers/scsi/sd.c
===================================================================
@@ -50,6 +50,7 @@
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>
+#include <scsi/scsi_aen.h>
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_dbg.h>
 #include <scsi/scsi_device.h>

drivers/scsi/sr.c +1 -0

Index: BUILD-2.6/drivers/scsi/sr.c
===================================================================
@@ -47,6 +47,7 @@
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>
+#include <scsi/scsi_aen.h>
 #include <scsi/scsi_dbg.h>
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_driver.h>

Jeff Garzik

2007-10-28 20:52 UTC #1 lore raw

Quoted 14 lines · James Bottomley

There was a fundamental flaw in the initial code, in that it had no locking around scsi_device_event_notify(). This is a problem because execute_in_process_context() also has no locking (it expects the caller to do the work). This means that two events firing simultaneously could potentially corrupt each other. This might be unlikely for media change events, but it will become more likely as we use this call as the engine for other SCSI AENs.
The best fix looks to be simply not to use sdev->ew for the source of the execute_work queue. Further, since kzalloc() always has to give us at least 32 bytes and we're doing an allocation anyway, it's free to shove the execute_work structure into scsi_device_event_info and eliminate all of the other locking and the work queue list.

Quoted 7 lines

I also separated the aen headers out into scsi_aen.h and cleaned up the uevent processing (there's no real need to define the strings anywhere other than in scsi_aen_uevent_notifier() ... especially as I envisage there will be more complex strings for some SCSI events.
Finally, I exported and made available scsi_aen_chain for the things that will attach to it.

You missed an updated version (which was CC'd to you).

Jeff

James Bottomley

author

2007-10-28 22:01 UTC #2 lore raw

Quoted 1 line · Jeff Garzik

You missed an updated version (which was CC'd to you).

No ... I was coming to yours next.

James