[PATCH 4/5] SCSI: fix bad method call in scsi_target_destroy

[PATCH 4/5] SCSI: fix bad method call in scsi_target_destroy

[Alan Stern]

This patch (as1336) fixes a bug in scsi_target_destroy().  It calls
the host template's target_destroy method even if the target_alloc
method failed.  (Also, the target_destroy method is called inside the
scope of the host lock, which is unnecessary and probably a mistake.)

A new flag is added to struct scsi_target to remember whether or not
the target_alloc has succeeded.  There also are a couple of minor
whitespace formatting improvements.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

Index: usb-2.6/include/scsi/scsi_device.h
===================================================================
--- usb-2.6.orig/include/scsi/scsi_device.h
+++ usb-2.6/include/scsi/scsi_device.h
@@ -242,6 +242,7 @@ struct scsi_target {
 	unsigned int		single_lun:1;	/* Indicates we should only
 						 * allow I/O to one of the luns
 						 * for the device at a time. */
+	unsigned int		did_target_alloc:1;
 	unsigned int		pdt_1f_for_no_lun;	/* PDT = 0x1f */
 						/* means no lun present */
 	/* commands actually active on LLD. protected by host lock. */
Index: usb-2.6/drivers/scsi/scsi_scan.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_scan.c
+++ usb-2.6/drivers/scsi/scsi_scan.c
@@ -332,10 +332,10 @@ static void scsi_target_destroy(struct s
 	struct Scsi_Host *shost = dev_to_shost(dev->parent);
 	unsigned long flags;
 
+	if (starget->did_target_alloc && shost->hostt->target_destroy)
+		shost->hostt->target_destroy(starget);
 	transport_destroy_device(dev);
 	spin_lock_irqsave(shost->host_lock, flags);
-	if (shost->hostt->target_destroy)
-		shost->hostt->target_destroy(starget);
 	list_del_init(&starget->siblings);
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	put_device(dev);
@@ -442,14 +442,16 @@ static struct scsi_target *scsi_alloc_ta
 	if (shost->hostt->target_alloc) {
 		error = shost->hostt->target_alloc(starget);
 
-		if(error) {
-			dev_printk(KERN_ERR, dev, "target allocation failed, error %d\n", error);
+		if (error) {
+			dev_printk(KERN_ERR, dev,
+				"target allocation failed, error %d\n", error);
 			/* don't want scsi_target_reap to do the final
 			 * put because it will be under the host lock */
 			scsi_target_destroy(starget);
 			return NULL;
 		}
 	}
+	starget->did_target_alloc = 1;
 	get_device(dev);
 
 	return starget;

Re: [PATCH 4/5] SCSI: fix bad method call in scsi_target_destroy

[James Bottomley]

On Fri, 2010-02-12 at 12:14 -0500, Alan Stern wrote:
> This patch (as1336) fixes a bug in scsi_target_destroy().  It calls
> the host template's target_destroy method even if the target_alloc
> method failed.  (Also, the target_destroy method is called inside the
> scope of the host lock, which is unnecessary and probably a mistake.)

OK, so this isn't a mistake.  The act of create/destroy on the target
and removing it from the lists has to be atomic for devices which use
fixed slots (most SPI cards).  If the target is still in the list after
destroy, it could end up getting spuriously reused.  Conversely, if you
remove it from the list and then destroy, we could end up getting target
alloc for a new target called *before* the target_destroy of the old
one.

> A new flag is added to struct scsi_target to remember whether or not
> the target_alloc has succeeded.  There also are a couple of minor
> whitespace formatting improvements.

I don't really see the need for this.  None of the users assumes the
target was created ... if they do allocations, they all check before
unwinding.  If there is a case for needing this, it should be part of
the target state flags.

James

Re: [PATCH 4/5] SCSI: fix bad method call in scsi_target_destroy

[Alan Stern]

On Fri, 19 Feb 2010, James Bottomley wrote:

> On Fri, 2010-02-12 at 12:14 -0500, Alan Stern wrote:
> > This patch (as1336) fixes a bug in scsi_target_destroy().  It calls
> > the host template's target_destroy method even if the target_alloc
> > method failed.  (Also, the target_destroy method is called inside the
> > scope of the host lock, which is unnecessary and probably a mistake.)
> 
> OK, so this isn't a mistake.  The act of create/destroy on the target
> and removing it from the lists has to be atomic for devices which use
> fixed slots (most SPI cards).  If the target is still in the list after
> destroy, it could end up getting spuriously reused.  Conversely, if you
> remove it from the list and then destroy, we could end up getting target
> alloc for a new target called *before* the target_destroy of the old
> one.

If the rest of the system is designed properly, targets won't get 
spuriously reused.  Of course, that's a big "if" -- but it's more 
relevant to the patch 5/5 discussion than to this one.

If you would prefer, I can revise this patch leaving the target_destroy
method call within the spinlocked region.

> > A new flag is added to struct scsi_target to remember whether or not
> > the target_alloc has succeeded.  There also are a couple of minor
> > whitespace formatting improvements.
> 
> I don't really see the need for this.  None of the users assumes the
> target was created ... if they do allocations, they all check before
> unwinding.  If there is a case for needing this, it should be part of
> the target state flags.

Maybe none of the current users make this assumption, but future users
might.  It's only natural to assume that the core won't call
your target_destroy function if target_alloc returned an error.  And 
neither of these methods is mentioned in 
Documentation/scsi/scsi_mid_low_api.txt, so the unnatural calling 
sequence isn't documented.

Some time ago I did post a patch adding an new state (STARGET_NEW) in
order to record whether or not target_alloc had failed.  I can't find
your response in the email archives, but I vaguely recall you saying
that adding a new state to the state machine wasn't an appropriate
approach.

What do you think is the best way to handle this?

Alan Stern

Re: [PATCH 4/5] SCSI: fix bad method call in scsi_target_destroy

[James Bottomley]

On Fri, 2010-02-19 at 14:53 -0500, Alan Stern wrote:
> On Fri, 19 Feb 2010, James Bottomley wrote:
> 
> > On Fri, 2010-02-12 at 12:14 -0500, Alan Stern wrote:
> > > This patch (as1336) fixes a bug in scsi_target_destroy().  It calls
> > > the host template's target_destroy method even if the target_alloc
> > > method failed.  (Also, the target_destroy method is called inside the
> > > scope of the host lock, which is unnecessary and probably a mistake.)
> > 
> > OK, so this isn't a mistake.  The act of create/destroy on the target
> > and removing it from the lists has to be atomic for devices which use
> > fixed slots (most SPI cards).  If the target is still in the list after
> > destroy, it could end up getting spuriously reused.  Conversely, if you
> > remove it from the list and then destroy, we could end up getting target
> > alloc for a new target called *before* the target_destroy of the old
> > one.
> 
> If the rest of the system is designed properly, targets won't get 
> spuriously reused.  Of course, that's a big "if" -- but it's more 
> relevant to the patch 5/5 discussion than to this one.
> 
> If you would prefer, I can revise this patch leaving the target_destroy
> method call within the spinlocked region.
> 
> > > A new flag is added to struct scsi_target to remember whether or not
> > > the target_alloc has succeeded.  There also are a couple of minor
> > > whitespace formatting improvements.
> > 
> > I don't really see the need for this.  None of the users assumes the
> > target was created ... if they do allocations, they all check before
> > unwinding.  If there is a case for needing this, it should be part of
> > the target state flags.
> 
> Maybe none of the current users make this assumption, but future users
> might.  It's only natural to assume that the core won't call
> your target_destroy function if target_alloc returned an error.  And 
> neither of these methods is mentioned in 
> Documentation/scsi/scsi_mid_low_api.txt, so the unnatural calling 
> sequence isn't documented.

So documenting it would be fine ...

> Some time ago I did post a patch adding an new state (STARGET_NEW) in
> order to record whether or not target_alloc had failed.  I can't find
> your response in the email archives, but I vaguely recall you saying
> that adding a new state to the state machine wasn't an appropriate
> approach.
> 
> What do you think is the best way to handle this?

Just add documentation ... there's no real need for a new state or a
flag.

James

Re: [PATCH 4/5] SCSI: fix bad method call in scsi_target_destroy

[Alan Stern]

On Fri, 12 Mar 2010, James Bottomley wrote:

> > Some time ago I did post a patch adding an new state (STARGET_NEW) in
> > order to record whether or not target_alloc had failed.  I can't find
> > your response in the email archives, but I vaguely recall you saying
> > that adding a new state to the state machine wasn't an appropriate
> > approach.
> > 
> > What do you think is the best way to handle this?
> 
> Just add documentation ... there's no real need for a new state or a
> flag.

Okay.  I'll put it off for now... in fact, I had more or less reached
the conclusion that this patch wasn't worth bothering with any further.

The 5/5 patch is more important, though.  The race doesn't need to be
fixed in the way I did it; another approach would be for
scsi_alloc_target() to busy-wait if the old target it finds has not yet
gone through the transport_setup_device() and
shost->hostt->target_alloc() procedures.  Waiting on the scan_mutex
seems more straightforward, though.

And it looks like there needs to be one more patch in addition.  The
logic in scsi_alloc_target() is wrong.  When deciding whether or not
the old target it found is dying, it needs to test reap_ref rather than
found_target->state, because the state is changed outside the
spinlocked region in scsi_target_reap().  Or the state change could be 
moved inside the spinlocked region; that would work also.

Alan Stern