[PATCH for-2.6.39] [SCSI] scsi_dh_emc: fix panic when handling multiple path failures

[PATCH for-2.6.39] [SCSI] scsi_dh_emc: fix panic when handling multiple path failures

[Mike Snitzer]

From: Eddie Williams <eddie.williams@steeleye.com>

Do not sdev_printk() a scsi_device that was, or is being, deleted.

If multiple failures occur (e.g. FC switch with multiple paths fails),
and both the active path and next path is failed in the process, the
scsi_device associated with the next path can be accessed after it was
deleted.

Signed-off-by: Eddie Williams <eddie.williams@steeleye.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/scsi/device_handler/scsi_dh_emc.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
index 48441f6..670f059 100644
--- a/drivers/scsi/device_handler/scsi_dh_emc.c
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -537,14 +537,14 @@ static int clariion_activate(struct scsi_device *sdev,
 
 	result = clariion_send_inquiry(sdev, csdev);
 	if (result != SCSI_DH_OK)
-		goto done;
+		goto out;
 
 	if (csdev->lun_state == CLARIION_LUN_OWNED)
 		goto done;
 
 	result = send_trespass_cmd(sdev, csdev);
 	if (result != SCSI_DH_OK)
-		goto done;
+		goto out;
 	sdev_printk(KERN_INFO, sdev,"%s: %s trespass command sent\n",
 		    CLARIION_NAME,
 		    csdev->flags&CLARIION_SHORT_TRESPASS?"short":"long" );
@@ -552,7 +552,7 @@ static int clariion_activate(struct scsi_device *sdev,
 	/* Update status */
 	result = clariion_send_inquiry(sdev, csdev);
 	if (result != SCSI_DH_OK)
-		goto done;
+		goto out;
 
 done:
 	sdev_printk(KERN_INFO, sdev,
@@ -561,10 +561,12 @@ done:
 		    csdev->port, lun_state[csdev->lun_state],
 		    csdev->default_sp + 'A');
 
+out:
 	if (fn)
 		fn(data, result);
 	return 0;
 }
+
 /*
  * params - parameters in the following format
  *      "no_of_params\0param1\0param2\0param3\0...\0"
-- 
1.7.3.4

Re: [PATCH for-2.6.39] [SCSI] scsi_dh_emc: fix panic when handling multiple path failures

[James Bottomley]

On Mon, 2011-03-21 at 15:57 -0400, Mike Snitzer wrote:
> From: Eddie Williams <eddie.williams@steeleye.com>
> 
> Do not sdev_printk() a scsi_device that was, or is being, deleted.
> 
> If multiple failures occur (e.g. FC switch with multiple paths fails),
> and both the active path and next path is failed in the process, the
> scsi_device associated with the next path can be accessed after it was
> deleted.

This treats the symptom, not the cause, doesn't it?  Why do we have a
reference to an ungot instance of an sdev here ... and should it have
had a reference taken on it?

James


> Signed-off-by: Eddie Williams <eddie.williams@steeleye.com>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> ---
>  drivers/scsi/device_handler/scsi_dh_emc.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
> index 48441f6..670f059 100644
> --- a/drivers/scsi/device_handler/scsi_dh_emc.c
> +++ b/drivers/scsi/device_handler/scsi_dh_emc.c
> @@ -537,14 +537,14 @@ static int clariion_activate(struct scsi_device *sdev,
>  
>  	result = clariion_send_inquiry(sdev, csdev);
>  	if (result != SCSI_DH_OK)
> -		goto done;
> +		goto out;
>  
>  	if (csdev->lun_state == CLARIION_LUN_OWNED)
>  		goto done;
>  
>  	result = send_trespass_cmd(sdev, csdev);
>  	if (result != SCSI_DH_OK)
> -		goto done;
> +		goto out;
>  	sdev_printk(KERN_INFO, sdev,"%s: %s trespass command sent\n",
>  		    CLARIION_NAME,
>  		    csdev->flags&CLARIION_SHORT_TRESPASS?"short":"long" );
> @@ -552,7 +552,7 @@ static int clariion_activate(struct scsi_device *sdev,
>  	/* Update status */
>  	result = clariion_send_inquiry(sdev, csdev);
>  	if (result != SCSI_DH_OK)
> -		goto done;
> +		goto out;
>  
>  done:
>  	sdev_printk(KERN_INFO, sdev,
> @@ -561,10 +561,12 @@ done:
>  		    csdev->port, lun_state[csdev->lun_state],
>  		    csdev->default_sp + 'A');
>  
> +out:
>  	if (fn)
>  		fn(data, result);
>  	return 0;
>  }
> +
>  /*
>   * params - parameters in the following format
>   *      "no_of_params\0param1\0param2\0param3\0...\0"

Re: [PATCH for-2.6.39] [SCSI] scsi_dh_emc: fix panic when handling multiple path failures

[Mike Snitzer]

On Mon, Mar 21 2011 at  4:02pm -0400,
James Bottomley <James.Bottomley@suse.de> wrote:

> On Mon, 2011-03-21 at 15:57 -0400, Mike Snitzer wrote:
> > From: Eddie Williams <eddie.williams@steeleye.com>
> > 
> > Do not sdev_printk() a scsi_device that was, or is being, deleted.
> > 
> > If multiple failures occur (e.g. FC switch with multiple paths fails),
> > and both the active path and next path is failed in the process, the
> > scsi_device associated with the next path can be accessed after it was
> > deleted.
> 
> This treats the symptom, not the cause, doesn't it?  Why do we have a
> reference to an ungot instance of an sdev here ... and should it have
> had a reference taken on it?

Stands to reason, and to be fair Eddie thought that might be the case
too -- I should have shared as much.  I'll take a closer look (quite
busy this week but...).

Finding the root cause in the near-term would be ideal.  Short of that
this "fix" isn't unreasonable.  Only problem with taking it would be if
someone like me didn't know to actively chase the real problem with this
"fix" reverted.  Whereby papering over the real issue.

Anyway, it's now on my plate to sort out.  No idea if I'll do so by the
close of the merge window.  But maybe a real fix to this can go in after
the window closes?

I guess time will tell, thanks.
Mike

Re: [PATCH for-2.6.39] [SCSI] scsi_dh_emc: fix panic when handling multiple path failures

[James Bottomley]

On Mon, 2011-03-21 at 16:38 -0400, Mike Snitzer wrote:
> Anyway, it's now on my plate to sort out.  No idea if I'll do so by the
> close of the merge window.  But maybe a real fix to this can go in after
> the window closes?

It's a bug fix: of course it can.

James

Re: [PATCH for-2.6.39] [SCSI] scsi_dh_emc: fix panic when handling multiple path failures

[Mike Snitzer]

On Mon, Mar 21 2011 at  4:44pm -0400,
James Bottomley <James.Bottomley@suse.de> wrote:

> On Mon, 2011-03-21 at 16:38 -0400, Mike Snitzer wrote:
> > Anyway, it's now on my plate to sort out.  No idea if I'll do so by the
> > close of the merge window.  But maybe a real fix to this can go in after
> > the window closes?
> 
> It's a bug fix: of course it can.

Right, so given scsi_dh_activate()'s {get,put}_device() that wraps the
call to scsi_dh->activate (aka clariion_activate) I'm not seeing how
accessing sdev->sdev_gendev (via sdev_printk) could result in a panic.
So I'll need to dig deeper (hopefully with Eddie's assistance)...

But now that I look it at scsi_dh_activate's 'err' path, it would appear
that we don't drop the reference (put_device) before returning 'err'.  I
unfotunately had a role in the offending change (commit: db422318)
getting upstream.  /me ducks

Looks to me like we need this:


From: Mike Snitzer <snitzer@redhat.com>
Subject: [SCSI] scsi_dh: fix reference counting in scsi_dh_activate error path

Commit db422318cbca55168cf965f655471dbf8be82433 ([SCSI] scsi_dh:
propagate SCSI device deletion) introduced a regression where the device
reference is not dropped prior to scsi_dh_activate's early return from
the error path.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Cc: stable@kernel.org
---
 drivers/scsi/device_handler/scsi_dh.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index 564e6ec..0119b81 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -394,12 +394,14 @@ int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
 	unsigned long flags;
 	struct scsi_device *sdev;
 	struct scsi_device_handler *scsi_dh = NULL;
+	struct device *dev = NULL;
 
 	spin_lock_irqsave(q->queue_lock, flags);
 	sdev = q->queuedata;
 	if (sdev && sdev->scsi_dh_data)
 		scsi_dh = sdev->scsi_dh_data->scsi_dh;
-	if (!scsi_dh || !get_device(&sdev->sdev_gendev) ||
+	dev = get_device(&sdev->sdev_gendev);
+	if (!scsi_dh || !dev ||
 	    sdev->sdev_state == SDEV_CANCEL ||
 	    sdev->sdev_state == SDEV_DEL)
 		err = SCSI_DH_NOSYS;
@@ -410,12 +412,13 @@ int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
 	if (err) {
 		if (fn)
 			fn(data, err);
-		return err;
+		goto out;
 	}
 
 	if (scsi_dh->activate)
 		err = scsi_dh->activate(sdev, fn, data);
-	put_device(&sdev->sdev_gendev);
+out:
+	put_device(dev);
 	return err;
 }
 EXPORT_SYMBOL_GPL(scsi_dh_activate);

Re: [PATCH for-2.6.39] [SCSI] scsi_dh_emc: fix panic when handling multiple path failures

[Eddie Williams]

oops, blasted google mail converted it to html...

On Mon, Mar 21, 2011 at 10:33 PM, Eddie Williams
<Eddie.Williams@us.sios.com> wrote:
>
> OK, I will give it a test in the morning...
> Eddie
>
> On Mon, Mar 21, 2011 at 7:16 PM, Mike Snitzer <snitzer@redhat.com> wrote:
>>
>> On Mon, Mar 21 2011 at  4:44pm -0400,
>> James Bottomley <James.Bottomley@suse.de> wrote:
>>
>> > On Mon, 2011-03-21 at 16:38 -0400, Mike Snitzer wrote:
>> > > Anyway, it's now on my plate to sort out.  No idea if I'll do so by the
>> > > close of the merge window.  But maybe a real fix to this can go in after
>> > > the window closes?
>> >
>> > It's a bug fix: of course it can.
>>
>> Right, so given scsi_dh_activate()'s {get,put}_device() that wraps the
>> call to scsi_dh->activate (aka clariion_activate) I'm not seeing how
>> accessing sdev->sdev_gendev (via sdev_printk) could result in a panic.
>> So I'll need to dig deeper (hopefully with Eddie's assistance)...
>>
>> But now that I look it at scsi_dh_activate's 'err' path, it would appear
>> that we don't drop the reference (put_device) before returning 'err'.  I
>> unfotunately had a role in the offending change (commit: db422318)
>> getting upstream.  /me ducks
>>
>> Looks to me like we need this:
>>
>>
>> From: Mike Snitzer <snitzer@redhat.com>
>> Subject: [SCSI] scsi_dh: fix reference counting in scsi_dh_activate error path
>>
>> Commit db422318cbca55168cf965f655471dbf8be82433 ([SCSI] scsi_dh:
>> propagate SCSI device deletion) introduced a regression where the device
>> reference is not dropped prior to scsi_dh_activate's early return from
>> the error path.
>>
>> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
>> Cc: stable@kernel.org
>> ---
>>  drivers/scsi/device_handler/scsi_dh.c |    9 ++++++---
>>  1 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
>> index 564e6ec..0119b81 100644
>> --- a/drivers/scsi/device_handler/scsi_dh.c
>> +++ b/drivers/scsi/device_handler/scsi_dh.c
>> @@ -394,12 +394,14 @@ int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
>>        unsigned long flags;
>>        struct scsi_device *sdev;
>>        struct scsi_device_handler *scsi_dh = NULL;
>> +       struct device *dev = NULL;
>>
>>        spin_lock_irqsave(q->queue_lock, flags);
>>        sdev = q->queuedata;
>>        if (sdev && sdev->scsi_dh_data)
>>                scsi_dh = sdev->scsi_dh_data->scsi_dh;
>> -       if (!scsi_dh || !get_device(&sdev->sdev_gendev) ||
>> +       dev = get_device(&sdev->sdev_gendev);
>> +       if (!scsi_dh || !dev ||
>>            sdev->sdev_state == SDEV_CANCEL ||
>>            sdev->sdev_state == SDEV_DEL)
>>                err = SCSI_DH_NOSYS;
>> @@ -410,12 +412,13 @@ int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
>>        if (err) {
>>                if (fn)
>>                        fn(data, err);
>> -               return err;
>> +               goto out;
>>        }
>>
>>        if (scsi_dh->activate)
>>                err = scsi_dh->activate(sdev, fn, data);
>> -       put_device(&sdev->sdev_gendev);
>> +out:
>> +       put_device(dev);
>>        return err;
>>  }
>>  EXPORT_SYMBOL_GPL(scsi_dh_activate);
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-2.6.39] [SCSI] scsi_dh_emc: fix panic when handling multiple path failures

[Mike Snitzer]

On Mon, Mar 21 2011 at  7:16pm -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> > It's a bug fix: of course it can.
> 
> Right, so given scsi_dh_activate()'s {get,put}_device() that wraps the
> call to scsi_dh->activate (aka clariion_activate) I'm not seeing how
> accessing sdev->sdev_gendev (via sdev_printk) could result in a panic.
> So I'll need to dig deeper (hopefully with Eddie's assistance)...

Hi James,

I checked with Eddie and the sdev_printk() panic he saw no longer occurs
when commit db422318 is in place.  But he also tested with the patch I
provided below.

I still feel this fix is needed and would like to get your feedback on
this (relative to 2.6.39):

> But now that I look at scsi_dh_activate's 'err' path, it would appear
> that we don't drop the reference (put_device) before returning 'err'.  I
> unfotunately had a role in the offending change (commit: db422318)
> getting upstream.  /me ducks
> 
> Looks to me like we need this:
> 
> 
> From: Mike Snitzer <snitzer@redhat.com>
> Subject: [SCSI] scsi_dh: fix reference counting in scsi_dh_activate error path
> 
> Commit db422318cbca55168cf965f655471dbf8be82433 ([SCSI] scsi_dh:
> propagate SCSI device deletion) introduced a regression where the device
> reference is not dropped prior to scsi_dh_activate's early return from
> the error path.
> 
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> Cc: stable@kernel.org
> ---
>  drivers/scsi/device_handler/scsi_dh.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
> index 564e6ec..0119b81 100644
> --- a/drivers/scsi/device_handler/scsi_dh.c
> +++ b/drivers/scsi/device_handler/scsi_dh.c
> @@ -394,12 +394,14 @@ int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
>  	unsigned long flags;
>  	struct scsi_device *sdev;
>  	struct scsi_device_handler *scsi_dh = NULL;
> +	struct device *dev = NULL;
>  
>  	spin_lock_irqsave(q->queue_lock, flags);
>  	sdev = q->queuedata;
>  	if (sdev && sdev->scsi_dh_data)
>  		scsi_dh = sdev->scsi_dh_data->scsi_dh;
> -	if (!scsi_dh || !get_device(&sdev->sdev_gendev) ||
> +	dev = get_device(&sdev->sdev_gendev);
> +	if (!scsi_dh || !dev ||
>  	    sdev->sdev_state == SDEV_CANCEL ||
>  	    sdev->sdev_state == SDEV_DEL)
>  		err = SCSI_DH_NOSYS;
> @@ -410,12 +412,13 @@ int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
>  	if (err) {
>  		if (fn)
>  			fn(data, err);
> -		return err;
> +		goto out;
>  	}
>  
>  	if (scsi_dh->activate)
>  		err = scsi_dh->activate(sdev, fn, data);
> -	put_device(&sdev->sdev_gendev);
> +out:
> +	put_device(dev);
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(scsi_dh_activate);
> 

Re: [PATCH for-2.6.39] [SCSI] scsi_dh_emc: fix panic when handling multiple path failures

[James Bottomley]

On Wed, 2011-03-23 at 17:58 -0400, Mike Snitzer wrote:
> On Mon, Mar 21 2011 at  7:16pm -0400,
> Mike Snitzer <snitzer@redhat.com> wrote:
> 
> > > It's a bug fix: of course it can.
> > 
> > Right, so given scsi_dh_activate()'s {get,put}_device() that wraps the
> > call to scsi_dh->activate (aka clariion_activate) I'm not seeing how
> > accessing sdev->sdev_gendev (via sdev_printk) could result in a panic.
> > So I'll need to dig deeper (hopefully with Eddie's assistance)...
> 
> Hi James,
> 
> I checked with Eddie and the sdev_printk() panic he saw no longer occurs
> when commit db422318 is in place.  But he also tested with the patch I
> provided below.
> 
> I still feel this fix is needed and would like to get your feedback on
> this (relative to 2.6.39):

So what about the fix you propose in

Message-id: <20110321231657.GA12902@redhat.com>

?  That looks to be the correct one.

James

Re: [PATCH for-2.6.39] [SCSI] scsi_dh_emc: fix panic when handling multiple path failures

[Mike Snitzer]

On Wed, Mar 23 2011 at  6:03pm -0400,
James Bottomley <James.Bottomley@suse.de> wrote:

> On Wed, 2011-03-23 at 17:58 -0400, Mike Snitzer wrote:
> > On Mon, Mar 21 2011 at  7:16pm -0400,
> > Mike Snitzer <snitzer@redhat.com> wrote:
> > 
> > > > It's a bug fix: of course it can.
> > > 
> > > Right, so given scsi_dh_activate()'s {get,put}_device() that wraps the
> > > call to scsi_dh->activate (aka clariion_activate) I'm not seeing how
> > > accessing sdev->sdev_gendev (via sdev_printk) could result in a panic.
> > > So I'll need to dig deeper (hopefully with Eddie's assistance)...
> > 
> > Hi James,
> > 
> > I checked with Eddie and the sdev_printk() panic he saw no longer occurs
> > when commit db422318 is in place.  But he also tested with the patch I
> > provided below.
> > 
> > I still feel this fix is needed and would like to get your feedback on
> > this (relative to 2.6.39):
> 
> So what about the fix you propose in
> 
> Message-id: <20110321231657.GA12902@redhat.com>
> 
> ?  That looks to be the correct one.

Yes, that is the correct fix (that is the same patch I quoted in my mail
preceeding this one).

We'd need it in 2.6.38.y too.

Thanks,
Mike