SCSI: Synchronization issue while deleting vport

PATCH In review Gal Rosen 3 participants 7 messages Jan 21, 2010 – Mar 8, 2010 SCSI

lore

Share & tools

lore

Gal Rosen

author

2010-01-21 08:16 UTC #0 lore raw

PATCH 1/1 SCSI: Synchronization issue while deleting vport

Hi,

Attach proposed patch for the PROBLEM: Synchronization issue in scsi_transport_fc.

Gal Rosen.

From ed24557e2f98f9617d083853fd2cb40f93e1c486 Mon Sep 17 00:00:00 2001
From: Gal Rosen <[email protected]>
Date: Thu, 21 Jan 2010 09:53:42 +0200
Subject: Synchronization issue while deleting vport

The issue occur while deleting 60 virtual ports through the sys interface /sys/class/fc_vports/vport-X/vport_delete. It happen while in a mistake each request sent twice for the same vport. This interface is asynchronous, entering the delete request into a work queue, allowing more than one request to enter to the delete work queue. The result is a NULL pointer. The first request already delete the vport, while the second request got a pointer to the vport before the device destroyed. Re-create vport later cause system freeze.

Solution: Check vport flags before entering the request to the work queue.

Signed-off-by Gal Rosen <[email protected]>

---
 drivers/scsi/scsi_transport_fc.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

drivers/scsi/scsi_transport_fc.c +12 ~~-12~~

index 5fd64e7..be96eb0 100644
@@ -1217,6 +1217,15 @@ store_fc_vport_delete(struct device *dev, struct device_attribute *attr,
 {
 	struct fc_vport *vport = transport_class_to_vport(dev);
 	struct Scsi_Host *shost = vport_to_shost(vport);
+	unsigned int flags;
+
+	spin_lock_irqsave(shost->host_lock, flags);
+	if (vport->flags & (FC_VPORT_DEL | FC_VPORT_CREATING)) {
+		spin_unlock_irqrestore(shost->host_lock, flags);
+		return -EBUSY;
+	}
+	vport->flags |= FC_VPORT_DELETING;
+	spin_unlock_irqrestore(shost->host_lock, flags);
 
 	fc_queue_work(shost, &vport->vport_delete_work);
 	return count;

@@ -1806,6 +1815,9 @@ store_fc_host_vport_delete(struct device *dev, struct device_attribute *attr,
 	list_for_each_entry(vport, &fc_host->vports, peers) {
 		if ((vport->channel == 0) &&
 		    (vport->port_name == wwpn) && (vport->node_name == wwnn)) {
+			if (vport->flags & (FC_VPORT_DEL | FC_VPORT_CREATING))
+				break;
+			vport->flags |= FC_VPORT_DELETING;
 			match = 1;
 			break;
 		}

@@ -3251,18 +3263,6 @@ fc_vport_terminate(struct fc_vport *vport)
 	unsigned long flags;
 	int stat;
 
-	spin_lock_irqsave(shost->host_lock, flags);
-	if (vport->flags & FC_VPORT_CREATING) {
-		spin_unlock_irqrestore(shost->host_lock, flags);
-		return -EBUSY;
-	}
-	if (vport->flags & (FC_VPORT_DEL)) {
-		spin_unlock_irqrestore(shost->host_lock, flags);
-		return -EALREADY;
-	}
-	vport->flags |= FC_VPORT_DELETING;
-	spin_unlock_irqrestore(shost->host_lock, flags);
-
 	if (i->f->vport_delete)
 		stat = i->f->vport_delete(vport);
 	else

Signature · 1 line

1.6.3.3

James Smart

2010-01-23 20:27 UTC #1 lore raw

Gal,

Thanks for correcting this issue. Patch looks good.

Signed-off-by James Smart <[email protected]>

Quoted 7 lines · Gal Rosen

Hi,
Attach proposed patch for the PROBLEM: Synchronization issue in scsi_transport_fc.
Gal Rosen.

Gal Rosen

author

2010-01-24 07:21 UTC #2 lore raw

James,

What is the process until it will enter to the code ? and for what version 2.6.26 or 2.6.33 ?

Thanks,
Gal.

Quoted 17 lines · James Smart

Gal,
Thanks for correcting this issue. Patch looks good.
Signed-off-by James Smart <[email protected]>
Quoted 7 lines · Gal Rosen
Hi,
Attach proposed patch for the PROBLEM: Synchronization issue in scsi_transport_fc.
Gal Rosen.

James Smart

2010-01-25 15:36 UTC #3 lore raw

James B controls the merge into upstream. He will most likely include the patch in scsi-rc-fixes-2.6, which will merge into one of the 2.6.33-rc? phases.

I assume the process, to get it into an already released kernel rev, is to petition the 2.6.<rev>-stable tree maintainer (usually GregKH) to pick it up.
You do need to be careful, as some changes are dependent on other things
that may have changed since that kernel revision. However, I don't think this change would have that problem.

-- james s

Quoted 25 lines · Gal Rosen

James,
What is the process until it will enter to the code ? and for what version 2.6.26 or 2.6.33 ?
Thanks,
Gal.
Quoted 16 lines · James Smart
Gal,
Thanks for correcting this issue. Patch looks good.
Signed-off-by James Smart <[email protected]>
Quoted 7 lines · Gal Rosen
Hi,
Attach proposed patch for the PROBLEM: Synchronization issue in scsi_transport_fc.
Gal Rosen.

Gal Rosen

author

2010-03-08 08:17 UTC #4 lore raw

Hello James,

Do you know why the patch still not included even in scsi-rc-fixes-2.6 ?

Can you push it ?

Thanks,
Gal.

Quoted 39 lines · James Smart

James B controls the merge into upstream. He will most likely include the patch in scsi-rc-fixes-2.6, which will merge into one of the 2.6.33-rc? phases.
I assume the process, to get it into an already released kernel rev, is to petition the 2.6.<rev>-stable tree maintainer (usually GregKH) to pick it up.
You do need to be careful, as some changes are dependent on other things
that may have changed since that kernel revision. However, I don't think this change would have that problem.
-- james s
Quoted 25 lines · Gal Rosen
James,
What is the process until it will enter to the code ? and for what version 2.6.26 or 2.6.33 ?
Thanks,
Gal.
Quoted 16 lines · James Smart
Gal,
Thanks for correcting this issue. Patch looks good.
Signed-off-by James Smart <[email protected]>
Quoted 7 lines · Gal Rosen
Hi,
Attach proposed patch for the PROBLEM: Synchronization issue in scsi_transport_fc.
Gal Rosen.

James Smart

2010-03-08 16:42 UTC #5 lore raw

James,

Can you pick up the patch ? http://marc.info/?l=linux-scsi&m=126406180628608&w=2

-- james s

Quoted 46 lines · Gal Rosen

Hello James,
Do you know why the patch still not included even in scsi-rc-fixes-2.6 ?
Can you push it ?
Thanks,
Gal.
Quoted 35 lines · James Smart
James B controls the merge into upstream. He will most likely include the patch in scsi-rc-fixes-2.6, which will merge into one of the 2.6.33-rc? phases.
I assume the process, to get it into an already released kernel rev, is to petition the 2.6.<rev>-stable tree maintainer (usually GregKH) to pick it up.
You do need to be careful, as some changes are dependent on other things
that may have changed since that kernel revision. However, I don't think this change would have that problem.
-- james s
Quoted 24 lines · Gal Rosen
James,
What is the process until it will enter to the code ? and for what version 2.6.26 or 2.6.33 ?
Thanks,
Gal.
Quoted 16 lines · James Smart
Gal,
Thanks for correcting this issue. Patch looks good.
Signed-off-by James Smart <[email protected]>
Quoted 7 lines · Gal Rosen
Hi,
Attach proposed patch for the PROBLEM: Synchronization issue in scsi_transport_fc.
Gal Rosen.

James Bottomley

2010-03-08 17:12 UTC #6 lore raw

In reply to Gal Rosen

drivers/scsi/scsi_transport_fc.c store_fc_vport_delete(struct device *dev, · line 1223

Quoted 13 lines · Gal Rosen

drivers/scsi/scsi_transport_fc.c +3 -0

index 5fd64e7..be96eb0 100644
@@ -1217,6 +1217,15 @@ store_fc_vport_delete(struct device *dev,
 struct device_attribute *attr,
 {
        struct fc_vport *vport = transport_class_to_vport(dev);
        struct Scsi_Host *shost = vport_to_shost(vport);
+       unsigned int flags;
+
+       spin_lock_irqsave(shost->host_lock, flags);

For a spinlock flags has to be unsigned long, not unsigned int ... but I can fix that up.

James