Re: [PATCH] scsi: check for device state in __scsi_remove_target()

["Ewan D. Milne" <emilne@redhat.com>]

On Thu, 2017-12-14 at 17:10 -0500, Ewan D. Milne wrote:
> On Thu, 2017-12-14 at 10:02 +0100, Hannes Reinecke wrote:
> > On 12/14/2017 09:05 AM, Jason Yan wrote:
> > > 
> > > On 2017/12/14 6:23, Bart Van Assche wrote:
> > >> On Wed, 2017-12-13 at 14:21 +0100, Hannes Reinecke wrote:
> > >>> As it turned out device_get() doesn't use kref_get_unless_zero(),
> > >>> so we will be always getting a device pointer.
> > >>> So we need to check for the device state in __scsi_remove_target()
> > >>> to avoid tripping over deleted objects.
> > >>>
> > >>> Fixes: fbce4d9 ("scsi: fixup kernel warning during rmmod()")
> > >>
> > >> How about adding Reported-by: Jason Yan? See also
> > >> https://www.spinics.net/lists/linux-scsi/msg115295.html
> > >>
> > >> Anyway:
> > >>
> > >> Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>
> > >>
> > > 
> > > Seems the same as my patch.So how do we plan to fix this issue,
> > > pick this approach up or the approach James Bottomley suggested?
> > > I have sent a patch to change get_device() but Greg seems do not
> > > like this way.
> > > 
> > This is actually a real regression, which can be trivially exercised by
> > eg logging out from two connections to an iSCSI target.
> > (Our QA tripped across that one).
> > So I'd rather have to have it fixed reasonably soon.
> > 
> > While 'get_device' is IMO the 'correct' solution it surely warrants a
> > broader discussion, plus one would need to audit all callers to check
> > the return value. If we were going down that route we should probably
> > add a __must_check to get_device(), too.
> > But again, this will probably drag out for quite some time, and I'd
> > prefer to have the fix in the meantime.
> > 
> > Cheers,
> > 
> > Hannes
> 
> We have 2 reproducible test cases, this patch fixes one of them,
> which was a continually oscillating FC target port w/short dev_loss_tmo.
> I'm still waiting for a report on the iSCSI test.  The code looks good.
> We need to get some kind of fix for this sooner rather than later.
> 
> Reviewed-by: Ewan D. Milne <emilne@redhat.com>

Report here is that Hannes's patch fixes our failing iSCSI test also.
Martin/James, can we get this in please?