* [PATCH] scsi: check for device state in __scsi_remove_target()
@ 2017-12-13 13:21 Hannes Reinecke
2017-12-13 22:23 ` Bart Van Assche
2017-12-19 3:37 ` Martin K. Petersen
0 siblings, 2 replies; 9+ messages in thread
From: Hannes Reinecke @ 2017-12-13 13:21 UTC (permalink / raw)
To: Martin K. Petersen
Cc: Christoph Hellwig, James Bottomley, linux-scsi, Hannes Reinecke,
Hannes Reinecke
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()")
Signed-off-by: Hannes Reinecke <hare@suse.com>
---
drivers/scsi/scsi_sysfs.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index cbc0fe2..a04678b 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1411,7 +1411,10 @@ static void __scsi_remove_target(struct scsi_target *starget)
* check.
*/
if (sdev->channel != starget->channel ||
- sdev->id != starget->id ||
+ sdev->id != starget->id)
+ continue;
+ if (sdev->sdev_state == SDEV_DEL ||
+ sdev->sdev_state == SDEV_CANCEL ||
!get_device(&sdev->sdev_gendev))
continue;
spin_unlock_irqrestore(shost->host_lock, flags);
--
1.8.5.6
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: check for device state in __scsi_remove_target()
2017-12-13 13:21 [PATCH] scsi: check for device state in __scsi_remove_target() Hannes Reinecke
@ 2017-12-13 22:23 ` Bart Van Assche
2017-12-14 8:05 ` Jason Yan
2017-12-19 3:37 ` Martin K. Petersen
1 sibling, 1 reply; 9+ messages in thread
From: Bart Van Assche @ 2017-12-13 22:23 UTC (permalink / raw)
To: hare, yanaijie, martin.petersen; +Cc: hch, james.bottomley, linux-scsi, hare
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: check for device state in __scsi_remove_target()
2017-12-13 22:23 ` Bart Van Assche
@ 2017-12-14 8:05 ` Jason Yan
2017-12-14 9:02 ` Hannes Reinecke
0 siblings, 1 reply; 9+ messages in thread
From: Jason Yan @ 2017-12-14 8:05 UTC (permalink / raw)
To: Bart Van Assche, hare, martin.petersen
Cc: hch, james.bottomley, linux-scsi, hare
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: check for device state in __scsi_remove_target()
2017-12-14 8:05 ` Jason Yan
@ 2017-12-14 9:02 ` Hannes Reinecke
2017-12-14 22:10 ` Ewan D. Milne
0 siblings, 1 reply; 9+ messages in thread
From: Hannes Reinecke @ 2017-12-14 9:02 UTC (permalink / raw)
To: Jason Yan, Bart Van Assche, hare, martin.petersen
Cc: hch, james.bottomley, linux-scsi
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
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.com +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: check for device state in __scsi_remove_target()
2017-12-14 9:02 ` Hannes Reinecke
@ 2017-12-14 22:10 ` Ewan D. Milne
2017-12-18 14:38 ` Ewan D. Milne
0 siblings, 1 reply; 9+ messages in thread
From: Ewan D. Milne @ 2017-12-14 22:10 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Jason Yan, Bart Van Assche, hare, martin.petersen, hch,
james.bottomley, linux-scsi
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: check for device state in __scsi_remove_target()
2017-12-14 22:10 ` Ewan D. Milne
@ 2017-12-18 14:38 ` Ewan D. Milne
0 siblings, 0 replies; 9+ messages in thread
From: Ewan D. Milne @ 2017-12-18 14:38 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Jason Yan, Bart Van Assche, hare, martin.petersen, hch,
james.bottomley, linux-scsi
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: check for device state in __scsi_remove_target()
2017-12-13 13:21 [PATCH] scsi: check for device state in __scsi_remove_target() Hannes Reinecke
2017-12-13 22:23 ` Bart Van Assche
@ 2017-12-19 3:37 ` Martin K. Petersen
2018-01-16 16:11 ` Bart Van Assche
1 sibling, 1 reply; 9+ messages in thread
From: Martin K. Petersen @ 2017-12-19 3:37 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Martin K. Petersen, Christoph Hellwig, James Bottomley,
linux-scsi, Hannes Reinecke
Hannes,
> 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.
Applied to 4.15/scsi-fixes. Thanks!
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: check for device state in __scsi_remove_target()
2017-12-19 3:37 ` Martin K. Petersen
@ 2018-01-16 16:11 ` Bart Van Assche
2018-01-17 4:39 ` Martin K. Petersen
0 siblings, 1 reply; 9+ messages in thread
From: Bart Van Assche @ 2018-01-16 16:11 UTC (permalink / raw)
To: hare, martin.petersen; +Cc: hch, james.bottomley, linux-scsi, hare
On Mon, 2017-12-18 at 22:37 -0500, Martin K. Petersen wrote:
> Hannes,
>
> > 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.
>
> Applied to 4.15/scsi-fixes. Thanks!
Hello Martin,
Since that patch fixes an issue that was introduced in kernel v4.14 but did
not have a "Cc: stable" tag, should this patch be sent to Greg for inclusion
in the kernel v4.14.x series?
Thanks,
Bart.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: check for device state in __scsi_remove_target()
2018-01-16 16:11 ` Bart Van Assche
@ 2018-01-17 4:39 ` Martin K. Petersen
0 siblings, 0 replies; 9+ messages in thread
From: Martin K. Petersen @ 2018-01-17 4:39 UTC (permalink / raw)
To: Bart Van Assche
Cc: hare, martin.petersen, hch, james.bottomley, linux-scsi, hare
Bart,
>> Applied to 4.15/scsi-fixes. Thanks!
>
> Since that patch fixes an issue that was introduced in kernel v4.14
> but did not have a "Cc: stable" tag, should this patch be sent to Greg
> for inclusion in the kernel v4.14.x series?
Yes. Hannes?
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-01-17 4:40 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-13 13:21 [PATCH] scsi: check for device state in __scsi_remove_target() Hannes Reinecke
2017-12-13 22:23 ` Bart Van Assche
2017-12-14 8:05 ` Jason Yan
2017-12-14 9:02 ` Hannes Reinecke
2017-12-14 22:10 ` Ewan D. Milne
2017-12-18 14:38 ` Ewan D. Milne
2017-12-19 3:37 ` Martin K. Petersen
2018-01-16 16:11 ` Bart Van Assche
2018-01-17 4:39 ` Martin K. Petersen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.