[PATCH][RFC] qla2xxx: Proposed protection of fcports field of struct scsi_qla_host

[PATCH][RFC] qla2xxx: Proposed protection of fcports field of struct scsi_qla_host

[Vladislav Bolkhovitin]

[-- Attachment #1: Type: text/plain, Size: 10029 bytes --]

This patch is proposal for protection of fcports field of struct 
scsi_qla_host, which is currently unprotected.

The problem.

At the moment, in qla2xxx driver list fcports, which is part of struct 
scsi_qla_host, is unprotected by any way. Basically, only one function 
can add to this list new entries: qla2x00_configure_local_loop() 
(another one, qla2x00_configure_fabric(), can be called only from 
qla2x00_configure_local_loop()), no entries never deleted from this list 
(except on shutdown) and many functions can go through this list using 
list_for_each_entry().

The problem is that all traversing over fcports functions can be called 
not only from the DPC thread, where qla2x00_configure_local_loop() adds 
new entries, but also from many other threads. For example, 
qla2x00_get_starget_node_name() can be called by any thread via sysfs.

As it is known, simultaneous traversing over a list together with adding 
a new member to it is racy and can lead to a crash.

The proposed solution.

This patch proposes a fix for this issue by protecting fcports list 
using RCU list access functions. It is possible, because adding to the 
list is done from the only DPC thread, so it's selfprotected.

But the main purpose of this patch is to bring attention to this issue 
as well as to possibility of existence of similar issues in other places 
of qla2xxx driver. Particularly, this patch doesn't address the 
following 2 issues:

  - It wasn't investigated if there are possible bad consequences if 
some function, traveling over fcports list, missed just added to it new entry.

  - It wasn't investigated if it is possible if new entries can be added 
to fcports list by qla2x00_configure_local_loop() called not from DPC 
thread. According to the attached call tree made by KScope, 
qla2x00_configure_local_loop() also can be called from 
eh_host_reset_handler(). But I'm not sure, if there isn't any protection 
somewhere against that.

Also I wasn't deeply investigated if other fields of struct 
scsi_qla_host similarly incorrectly unprotected.

(BTW, IMHO the whole idea of never deleting entries from fcports list 
isn't quite good. Sure, it is impossible to have thousands of dead 
entries, but with virtual ports having hundreds of them is quite 
possible. I'm not sure if it's good to loose that memory.)

This patch is against 2.6.26

Signed-off-by: Vladislav Bolkhovitin <vst@vlnb.net>

diff -upr linux-2.6.26/drivers/scsi/qla2xxx/qla_attr.c linux-2.6.26/drivers/scsi/qla2xxx/qla_attr.c
--- linux-2.6.26/drivers/scsi/qla2xxx/qla_attr.c	2008-07-14 01:51:29.000000000 +0400
+++ linux-2.6.26/drivers/scsi/qla2xxx/qla_attr.c	2008-07-24 10:08:12.000000000 +0400
@@ -921,7 +921,7 @@ qla2x00_get_starget_node_name(struct scs
 	fc_port_t *fcport;
 	u64 node_name = 0;
 
-	list_for_each_entry(fcport, &ha->fcports, list) {
+	list_for_each_entry_rcu(fcport, &ha->fcports, list) {
 		if (fcport->rport &&
 		    starget->id == fcport->rport->scsi_target_id) {
 			node_name = wwn_to_u64(fcport->node_name);
@@ -940,7 +940,7 @@ qla2x00_get_starget_port_name(struct scs
 	fc_port_t *fcport;
 	u64 port_name = 0;
 
-	list_for_each_entry(fcport, &ha->fcports, list) {
+	list_for_each_entry_rcu(fcport, &ha->fcports, list) {
 		if (fcport->rport &&
 		    starget->id == fcport->rport->scsi_target_id) {
 			port_name = wwn_to_u64(fcport->port_name);
@@ -959,7 +959,7 @@ qla2x00_get_starget_port_id(struct scsi_
 	fc_port_t *fcport;
 	uint32_t port_id = ~0U;
 
-	list_for_each_entry(fcport, &ha->fcports, list) {
+	list_for_each_entry_rcu(fcport, &ha->fcports, list) {
 		if (fcport->rport &&
 		    starget->id == fcport->rport->scsi_target_id) {
 			port_id = fcport->d_id.b.domain << 16 |
diff -upr linux-2.6.26/drivers/scsi/qla2xxx/qla_def.h linux-2.6.26/drivers/scsi/qla2xxx/qla_def.h
--- linux-2.6.26/drivers/scsi/qla2xxx/qla_def.h	2008-07-14 01:51:29.000000000 +0400
+++ linux-2.6.26/drivers/scsi/qla2xxx/qla_def.h	2008-07-24 10:07:39.000000000 +0400
@@ -2403,7 +2403,14 @@ typedef struct scsi_qla_host {
 
 	struct list_head	work_list;
 
-	/* Fibre Channel Device List. */
+	/*
+	 * Fibre Channel Device List.
+	 *
+	 * This list can be accessed from many contexts, including concurrently.
+	 * So, although anything never deleted from this list, it needs the
+	 * corresponding protection anyway. Let's do it by making access to
+	 * it using RCU functions.
+	 */
 	struct list_head	fcports;
 
 	/* RSCN queue. */
diff -upr linux-2.6.26/drivers/scsi/qla2xxx/qla_init.c linux-2.6.26/drivers/scsi/qla2xxx/qla_init.c
--- linux-2.6.26/drivers/scsi/qla2xxx/qla_init.c	2008-07-14 01:51:29.000000000 +0400
+++ linux-2.6.26/drivers/scsi/qla2xxx/qla_init.c	2008-07-24 10:11:56.000000000 +0400
@@ -2071,7 +2071,7 @@ qla2x00_configure_local_loop(scsi_qla_ho
 	/*
 	 * Mark local devices that were present with FCF_DEVICE_LOST for now.
 	 */
-	list_for_each_entry(fcport, &pha->fcports, list) {
+	list_for_each_entry_rcu(fcport, &pha->fcports, list) {
 		if (fcport->vp_idx != ha->vp_idx)
 			continue;
 
@@ -2136,7 +2136,7 @@ qla2x00_configure_local_loop(scsi_qla_ho
 		/* Check for matching device in port list. */
 		found = 0;
 		fcport = NULL;
-		list_for_each_entry(fcport, &pha->fcports, list) {
+		list_for_each_entry_rcu(fcport, &pha->fcports, list) {
 			if (fcport->vp_idx != ha->vp_idx)
 				continue;
 
@@ -2165,7 +2165,7 @@ qla2x00_configure_local_loop(scsi_qla_ho
 				list_add_tail(&new_fcport->vp_fcport,
 				    &ha->vp_fcports);
 			}
-			list_add_tail(&new_fcport->list, &pha->fcports);
+			list_add_tail_rcu(&new_fcport->list, &pha->fcports);
 
 			/* Allocate a new replacement fcport. */
 			fcport = new_fcport;
@@ -2405,7 +2405,7 @@ qla2x00_configure_fabric(scsi_qla_host_t
 		 * Logout all previous fabric devices marked lost, except
 		 * tape devices.
 		 */
-		list_for_each_entry(fcport, &pha->fcports, list) {
+		list_for_each_entry_rcu(fcport, &pha->fcports, list) {
 			if (fcport->vp_idx !=ha->vp_idx)
 				continue;
 
@@ -2439,7 +2439,7 @@ qla2x00_configure_fabric(scsi_qla_host_t
 		 * Scan through our port list and login entries that need to be
 		 * logged in.
 		 */
-		list_for_each_entry(fcport, &pha->fcports, list) {
+		list_for_each_entry_rcu(fcport, &pha->fcports, list) {
 			if (fcport->vp_idx != ha->vp_idx)
 				continue;
 
@@ -2494,10 +2494,13 @@ qla2x00_configure_fabric(scsi_qla_host_t
 				fcport->vp_idx = ha->vp_idx;
 				list_add_tail(&fcport->vp_fcport,
 				    &ha->vp_fcports);
-				list_move_tail(&fcport->list,
+				list_del(&fcport->list);
+				list_add_tail_rcu(&fcport->list,
 				    &ha->parent->fcports);
-			} else
-				list_move_tail(&fcport->list, &ha->fcports);
+			} else {
+				list_del(&fcport->list);
+				list_add_tail_rcu(&fcport->list, &ha->fcports);
+			}
 		}
 	} while (0);
 
@@ -2680,7 +2683,7 @@ qla2x00_find_all_fabric_devs(scsi_qla_ho
 
 		/* Locate matching device in database. */
 		found = 0;
-		list_for_each_entry(fcport, &pha->fcports, list) {
+		list_for_each_entry_rcu(fcport, &pha->fcports, list) {
 			if (new_fcport->vp_idx != fcport->vp_idx)
 				continue;
 			if (memcmp(new_fcport->port_name, fcport->port_name,
@@ -2810,7 +2813,7 @@ qla2x00_find_new_loop_id(scsi_qla_host_t
 		/* Check for loop ID being already in use. */
 		found = 0;
 		fcport = NULL;
-		list_for_each_entry(fcport, &pha->fcports, list) {
+		list_for_each_entry_rcu(fcport, &pha->fcports, list) {
 			if (fcport->loop_id == dev->loop_id && fcport != dev) {
 				/* ID possibly in use */
 				found++;
@@ -2924,7 +2927,7 @@ qla2x00_device_resync(scsi_qla_host_t *h
 
 		rval = QLA_SUCCESS;
 
-		list_for_each_entry(fcport, &pha->fcports, list) {
+		list_for_each_entry_rcu(fcport, &pha->fcports, list) {
 			if (fcport->vp_idx != ha->vp_idx)
 				continue;
 
@@ -3219,7 +3222,7 @@ qla2x00_update_fcports(scsi_qla_host_t *
 	fc_port_t *fcport;
 
 	/* Go with deferred removal of rport references. */
-	list_for_each_entry(fcport, &ha->fcports, list)
+	list_for_each_entry_rcu(fcport, &ha->fcports, list)
 		if (fcport->drport)
 			qla2x00_rport_del(fcport);
 }
diff -upr linux-2.6.26/drivers/scsi/qla2xxx/qla_mid.c linux-2.6.26/drivers/scsi/qla2xxx/qla_mid.c
--- linux-2.6.26/drivers/scsi/qla2xxx/qla_mid.c	2008-07-14 01:51:29.000000000 +0400
+++ linux-2.6.26/drivers/scsi/qla2xxx/qla_mid.c	2008-07-24 10:12:02.000000000 +0400
@@ -95,7 +95,7 @@ qla2x00_mark_vp_devices_dead(scsi_qla_ho
 	fc_port_t *fcport;
 	scsi_qla_host_t *pha = to_qla_parent(vha);
 
-	list_for_each_entry(fcport, &pha->fcports, list) {
+	list_for_each_entry_rcu(fcport, &pha->fcports, list) {
 		if (fcport->vp_idx != vha->vp_idx)
 			continue;
 
diff -upr linux-2.6.26/drivers/scsi/qla2xxx/qla_os.c linux-2.6.26/drivers/scsi/qla2xxx/qla_os.c
--- linux-2.6.26/drivers/scsi/qla2xxx/qla_os.c	2008-07-14 01:51:29.000000000 +0400
+++ linux-2.6.26/drivers/scsi/qla2xxx/qla_os.c	2008-07-24 10:13:45.000000000 +0400
@@ -1009,7 +1009,7 @@ qla2x00_loop_reset(scsi_qla_host_t *ha)
 	}
 
 	if (ha->flags.enable_target_reset) {
-		list_for_each_entry(fcport, &ha->fcports, list) {
+		list_for_each_entry_rcu(fcport, &ha->fcports, list) {
 			if (fcport->port_type != FCT_TARGET)
 				continue;
 
@@ -1915,7 +1915,7 @@ qla2x00_mark_all_devices_lost(scsi_qla_h
 	fc_port_t *fcport;
 	scsi_qla_host_t *pha = to_qla_parent(ha);
 
-	list_for_each_entry(fcport, &pha->fcports, list) {
+	list_for_each_entry_rcu(fcport, &pha->fcports, list) {
 		if (ha->vp_idx != 0 && ha->vp_idx != fcport->vp_idx)
 			continue;
 		/*
@@ -2348,7 +2348,7 @@ qla2x00_do_dpc(void *data)
 			    ha->host_no));
 
 			next_loopid = 0;
-			list_for_each_entry(fcport, &ha->fcports, list) {
+			list_for_each_entry_rcu(fcport, &ha->fcports, list) {
 				/*
 				 * If the port is not ONLINE then try to login
 				 * to it if we haven't run out of retries.
@@ -2524,7 +2524,7 @@ qla2x00_timer(scsi_qla_host_t *ha)
 	 * the port it marked DEAD.
 	 */
 	t = 0;
-	list_for_each_entry(fcport, &ha->fcports, list) {
+	list_for_each_entry_rcu(fcport, &ha->fcports, list) {
 		if (fcport->port_type != FCT_TARGET)
 			continue;
 


[-- Attachment #2: Screenshot-Calling Functions Tree.png --]
[-- Type: image/png, Size: 34695 bytes --]