[PATCH] SCSI: Synchronization issue while deleting vport

[PATCH] SCSI: Synchronization issue while deleting vport

[Gal Rosen]

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

Hi,

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

Gal Rosen.

[-- Attachment #2: 0001-Synchronization-issue-while-deleting-vport.patch --]
[-- Type: text/x-patch, Size: 2756 bytes --]

From ed24557e2f98f9617d083853fd2cb40f93e1c486 Mon Sep 17 00:00:00 2001
From: Gal Rosen <galr@storwize.com>
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 <galr@storwize.com>
---
 drivers/scsi/scsi_transport_fc.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 5fd64e7..be96eb0 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -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
-- 
1.6.3.3

Re: [PATCH] SCSI: Synchronization issue while deleting vport

[James Smart]

Gal,

Thanks for correcting this issue.  Patch looks good.

Signed-off-by: James Smart <james.smart@emulex.com>



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

Re: [PATCH] SCSI: Synchronization issue while deleting vport

[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.
On Fri, 2010-01-22 at 21:00 -0500, James Smart wrote:
> Gal,
> 
> Thanks for correcting this issue.  Patch looks good.
> 
> Signed-off-by: James Smart <james.smart@emulex.com>
> 
> 
> 
> Gal Rosen wrote:
> > Hi,
> > 
> > Attach proposed patch for the PROBLEM: Synchronization issue in
> > scsi_transport_fc.
> > 
> > Gal Rosen.
> > 
> 

Re: [PATCH] SCSI: Synchronization issue while deleting vport

[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



Gal Rosen wrote:
> 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.
> On Fri, 2010-01-22 at 21:00 -0500, James Smart wrote:
>> Gal,
>>
>> Thanks for correcting this issue.  Patch looks good.
>>
>> Signed-off-by: James Smart <james.smart@emulex.com>
>>
>>
>>
>> Gal Rosen wrote:
>>> Hi,
>>>
>>> Attach proposed patch for the PROBLEM: Synchronization issue in
>>> scsi_transport_fc.
>>>
>>> Gal Rosen.
>>>
> 

Re: [PATCH] SCSI: Synchronization issue while deleting vport

[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.

On Mon, 2010-01-25 at 10:35 -0500, James Smart wrote:
> 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
> 
> 
> 
> Gal Rosen wrote:
> > 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.
> > On Fri, 2010-01-22 at 21:00 -0500, James Smart wrote:
> >> Gal,
> >>
> >> Thanks for correcting this issue.  Patch looks good.
> >>
> >> Signed-off-by: James Smart <james.smart@emulex.com>
> >>
> >>
> >>
> >> Gal Rosen wrote:
> >>> Hi,
> >>>
> >>> Attach proposed patch for the PROBLEM: Synchronization issue in
> >>> scsi_transport_fc.
> >>>
> >>> Gal Rosen.
> >>>
> > 

Re: [PATCH] SCSI: Synchronization issue while deleting vport

[James Smart]

James,

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

-- james s


Gal Rosen wrote:
> Hello James,
> 
> Do you know why the patch still not included even in scsi-rc-fixes-2.6 ?
> 
> Can you push it ?
> 
> Thanks,
> Gal.
> 
> On Mon, 2010-01-25 at 10:35 -0500, James Smart wrote:
>> 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
>>
>>
>>
>> Gal Rosen wrote:
>>> 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.
>>> On Fri, 2010-01-22 at 21:00 -0500, James Smart wrote:
>>>> Gal,
>>>>
>>>> Thanks for correcting this issue.  Patch looks good.
>>>>
>>>> Signed-off-by: James Smart <james.smart@emulex.com>
>>>>
>>>>
>>>>
>>>> Gal Rosen wrote:
>>>>> Hi,
>>>>>
>>>>> Attach proposed patch for the PROBLEM: Synchronization issue in
>>>>> scsi_transport_fc.
>>>>>
>>>>> Gal Rosen.
>>>>>
> 

Re: [PATCH] SCSI: Synchronization issue while deleting vport

[James Bottomley]

On Thu, 2010-01-21 at 10:15 +0200, Gal Rosen wrote:
> diff --git a/drivers/scsi/scsi_transport_fc.c
> b/drivers/scsi/scsi_transport_fc.c
> index 5fd64e7..be96eb0 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -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