Re: work queue of scsi fc transports should be serialized

[Martin Wilck <mwilck@suse.com>]

On Sat, 2017-05-20 at 08:25 +0000, Dashi DS1 Cao wrote:
> On Fri, 2017-05-19 at 09:36 +0000, Dashi DS1 Cao wrote:
> > It seems there is a race of multiple "fc_starget_delete" of the
> > same 
> > rport, thus of the same SCSI host. The race leads to the race of 
> > scsi_remove_target and it cannot be prevented by the code snippet 
> > alone, even of the most recent
> > version:
> >         spin_lock_irqsave(shost->host_lock, flags);
> >         list_for_each_entry(starget, &shost->__targets, siblings) {
> >                 if (starget->state == STARGET_DEL ||
> >                     starget->state == STARGET_REMOVE)
> >                         continue;
> > If there is a possibility that the starget is under deletion(state
> > == 
> > STARGET_DEL), it should be possible that list_next_entry(starget, 
> > siblings) could cause a read access violation.
> > Hello Dashi,
> > Something else must be going on. From scsi_remove_target():
> > restart:
> > 	spin_lock_irqsave(shost->host_lock, flags);
> > 	list_for_each_entry(starget, &shost->__targets, siblings) {
> > 		if (starget->state == STARGET_DEL ||
> > 		    starget->state == STARGET_REMOVE)
> > 			continue;
> > 		if (starget->dev.parent == dev || &starget->dev == dev)
> > {
> > 			kref_get(&starget->reap_ref);
> > 			starget->state = STARGET_REMOVE;
> > 			spin_unlock_irqrestore(shost->host_lock,
> > flags);
> > 			__scsi_remove_target(starget);
> > 			scsi_target_reap(starget);
> > 			goto restart;
> > 		}
> > 	}
> > 	spin_unlock_irqrestore(shost->host_lock, flags);
> > In other words, before scsi_remove_target() decides to call
> > __scsi_remove_target(), it changes the target state into
> > STARGET_REMOVE while holding the host lock. 
> > This means that scsi_remove_target() won't
> > call __scsi_remove_target() twice and also that it won't invoke
> > list_next_entry(starget, siblings) after starget has been 
> > freed.
> > Bart.
> 
> In the crashes of Suse 12 sp1, the root cause is the deletion of a
> list node without holding the lock:
>         spin_lock_irqsave(shost->host_lock, flags);
>         list_for_each_entry_safe(starget, tmp, &shost->__targets,
> siblings) {
>                 if (starget->state == STARGET_DEL)
>                         continue;
>                 if (starget->dev.parent == dev || &starget->dev ==
> dev) {
>                         /* assuming new targets arrive at the end */
>                         kref_get(&starget->reap_ref);
>                         spin_unlock_irqrestore(shost->host_lock,
> flags);
> 
>                         __scsi_remove_target(starget);
>                         list_move_tail(&starget->siblings,
> &reap_list);  --this deletion from shost->__targets list is done
> without the lock.
>                         spin_lock_irqsave(shost->host_lock, flags);
>                  }
>           }
>           spin_unlock_irqrestore(shost->host_lock, flags);

I believe this is fixed in SLES12-SP1 kernel 3.12.53-60.30.1, with the
following patch:

* Mon Jan 18 2016 jthumshirn@suse.de
- scsi: restart list search after unlock in scsi_remove_target
  (bsc#944749, bsc#959257).
- Delete
  patches.fixes/0001-SCSI-Fix-hard-lockup-in-scsi_remove_target.patch.
- commit 2490876

Regards,
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)