* [PATCH] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state
@ 2016-03-24 9:56 Johannes Thumshirn
2016-03-24 15:42 ` Ewan D. Milne
0 siblings, 1 reply; 6+ messages in thread
From: Johannes Thumshirn @ 2016-03-24 9:56 UTC (permalink / raw)
To: Martin K. Petersen, James E.J. Bottomley
Cc: linux-scsi, linux-kernel, Hannes Reinecke, Johannes Thumshirn, stable
The target state machine only knows 'STARGET_DEL', which is set once
scsi_target_destroy() is called.
However, by that time the structure is still part of the __stargets
list of the SCSI host, so any concurrent invocation will see this as
a valid target, causing an access to freed memory.
This patch adds an intermediate state 'STARGET_REMOVE', which is set
as soon as the target is scheduled to be removed.
With this any concurrent invocation will be able to skip these
targets and avoid the above scenario.
Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
Fixes: 90a88d6ef 'scsi: fix soft lockup in scsi_remove_target() on module removal'
Cc: stable@vger.kernel.org
Reviewed-by: Hannes Reinecke <hare@suse.com>
---
drivers/scsi/scsi_scan.c | 1 +
drivers/scsi/scsi_sysfs.c | 2 ++
include/scsi/scsi_device.h | 1 +
3 files changed, 4 insertions(+)
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 6a82066..37459dfa 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -315,6 +315,7 @@ static void scsi_target_destroy(struct scsi_target *starget)
struct Scsi_Host *shost = dev_to_shost(dev->parent);
unsigned long flags;
+ BUG_ON(starget->state != STARGET_REMOVE);
starget->state = STARGET_DEL;
transport_destroy_device(dev);
spin_lock_irqsave(shost->host_lock, flags);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 00bc721..0df82e8 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1279,11 +1279,13 @@ 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 ||
starget == last_target)
continue;
if (starget->dev.parent == dev || &starget->dev == dev) {
kref_get(&starget->reap_ref);
last_target = starget;
+ starget->state = STARGET_REMOVE;
spin_unlock_irqrestore(shost->host_lock, flags);
__scsi_remove_target(starget);
scsi_target_reap(starget);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index f63a167..2bffaa6 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -240,6 +240,7 @@ scmd_printk(const char *, const struct scsi_cmnd *, const char *, ...);
enum scsi_target_state {
STARGET_CREATED = 1,
STARGET_RUNNING,
+ STARGET_REMOVE,
STARGET_DEL,
};
--
1.8.5.6
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state
2016-03-24 9:56 [PATCH] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state Johannes Thumshirn
@ 2016-03-24 15:42 ` Ewan D. Milne
2016-03-29 7:20 ` Johannes Thumshirn
2016-03-29 7:45 ` Christoph Hellwig
0 siblings, 2 replies; 6+ messages in thread
From: Ewan D. Milne @ 2016-03-24 15:42 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: Martin K. Petersen, James E.J. Bottomley, linux-scsi,
linux-kernel, Hannes Reinecke, stable
On Thu, 2016-03-24 at 10:56 +0100, Johannes Thumshirn wrote:
> The target state machine only knows 'STARGET_DEL', which is set once
> scsi_target_destroy() is called.
> However, by that time the structure is still part of the __stargets
> list of the SCSI host, so any concurrent invocation will see this as
> a valid target, causing an access to freed memory.
>
> This patch adds an intermediate state 'STARGET_REMOVE', which is set
> as soon as the target is scheduled to be removed.
> With this any concurrent invocation will be able to skip these
> targets and avoid the above scenario.
>
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> Fixes: 90a88d6ef 'scsi: fix soft lockup in scsi_remove_target() on module removal'
> Cc: stable@vger.kernel.org
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> ---
> drivers/scsi/scsi_scan.c | 1 +
> drivers/scsi/scsi_sysfs.c | 2 ++
> include/scsi/scsi_device.h | 1 +
> 3 files changed, 4 insertions(+)
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 6a82066..37459dfa 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -315,6 +315,7 @@ static void scsi_target_destroy(struct scsi_target *starget)
> struct Scsi_Host *shost = dev_to_shost(dev->parent);
> unsigned long flags;
>
> + BUG_ON(starget->state != STARGET_REMOVE);
> starget->state = STARGET_DEL;
> transport_destroy_device(dev);
> spin_lock_irqsave(shost->host_lock, flags);
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 00bc721..0df82e8 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1279,11 +1279,13 @@ 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 ||
> starget == last_target)
> continue;
> if (starget->dev.parent == dev || &starget->dev == dev) {
> kref_get(&starget->reap_ref);
> last_target = starget;
> + starget->state = STARGET_REMOVE;
> spin_unlock_irqrestore(shost->host_lock, flags);
> __scsi_remove_target(starget);
> scsi_target_reap(starget);
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index f63a167..2bffaa6 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -240,6 +240,7 @@ scmd_printk(const char *, const struct scsi_cmnd *, const char *, ...);
> enum scsi_target_state {
> STARGET_CREATED = 1,
> STARGET_RUNNING,
> + STARGET_REMOVE,
> STARGET_DEL,
> };
>
This looks fine. Do we still need 90a88d6ef (scsi: fix soft lockup in
scsi_remove_target() on module removal) or can that be reverted now,
since the STARGET_REMOVE state will allow the iteration to continue?
Reviewed-by: Ewan D. Milne <emilne@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state
2016-03-24 15:42 ` Ewan D. Milne
2016-03-29 7:20 ` Johannes Thumshirn
@ 2016-03-29 7:20 ` Johannes Thumshirn
1 sibling, 0 replies; 6+ messages in thread
From: Johannes Thumshirn @ 2016-03-29 7:20 UTC (permalink / raw)
To: emilne
Cc: Martin K. Petersen, James E.J. Bottomley, linux-scsi,
linux-kernel, Hannes Reinecke, stable
On Donnerstag, 24. März 2016 11:42:44 CEST Ewan D. Milne wrote:
> On Thu, 2016-03-24 at 10:56 +0100, Johannes Thumshirn wrote:
> > The target state machine only knows 'STARGET_DEL', which is set once
> > scsi_target_destroy() is called.
> > However, by that time the structure is still part of the __stargets
> > list of the SCSI host, so any concurrent invocation will see this as
> > a valid target, causing an access to freed memory.
> >
> > This patch adds an intermediate state 'STARGET_REMOVE', which is set
> > as soon as the target is scheduled to be removed.
> > With this any concurrent invocation will be able to skip these
> > targets and avoid the above scenario.
> >
> > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> > Fixes: 90a88d6ef 'scsi: fix soft lockup in scsi_remove_target() on module
> > removal' Cc: stable@vger.kernel.org
> > Reviewed-by: Hannes Reinecke <hare@suse.com>
> > ---
> >
> > drivers/scsi/scsi_scan.c | 1 +
> > drivers/scsi/scsi_sysfs.c | 2 ++
> > include/scsi/scsi_device.h | 1 +
> > 3 files changed, 4 insertions(+)
> >
> > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> > index 6a82066..37459dfa 100644
> > --- a/drivers/scsi/scsi_scan.c
> > +++ b/drivers/scsi/scsi_scan.c
> > @@ -315,6 +315,7 @@ static void scsi_target_destroy(struct scsi_target
> > *starget)>
> > struct Scsi_Host *shost = dev_to_shost(dev->parent);
> > unsigned long flags;
> >
> > + BUG_ON(starget->state != STARGET_REMOVE);
> >
> > starget->state = STARGET_DEL;
> > transport_destroy_device(dev);
> > spin_lock_irqsave(shost->host_lock, flags);
> >
> > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> > index 00bc721..0df82e8 100644
> > --- a/drivers/scsi/scsi_sysfs.c
> > +++ b/drivers/scsi/scsi_sysfs.c
> >
> > @@ -1279,11 +1279,13 @@ 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 ||
> >
> > starget == last_target)
> >
> > continue;
> >
> > if (starget->dev.parent == dev || &starget->dev == dev) {
> >
> > kref_get(&starget->reap_ref);
> > last_target = starget;
> >
> > + starget->state = STARGET_REMOVE;
> >
> > spin_unlock_irqrestore(shost->host_lock, flags);
> > __scsi_remove_target(starget);
> > scsi_target_reap(starget);
> >
> > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> > index f63a167..2bffaa6 100644
> > --- a/include/scsi/scsi_device.h
> > +++ b/include/scsi/scsi_device.h
> > @@ -240,6 +240,7 @@ scmd_printk(const char *, const struct scsi_cmnd *,
> > const char *, ...);>
> > enum scsi_target_state {
> >
> > STARGET_CREATED = 1,
> > STARGET_RUNNING,
> >
> > + STARGET_REMOVE,
> >
> > STARGET_DEL,
> >
> > };
>
> This looks fine. Do we still need 90a88d6ef (scsi: fix soft lockup in
> scsi_remove_target() on module removal) or can that be reverted now,
> since the STARGET_REMOVE state will allow the iteration to continue?
>
> Reviewed-by: Ewan D. Milne <emilne@redhat.com>
We've tested it on-top of 90a88d6ef, so I'm not quite sure.
Anyways, I've seen a mail from the 0-day bot regarding this patch, so I'll
have to check that one first.
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state
@ 2016-03-29 7:20 ` Johannes Thumshirn
0 siblings, 0 replies; 6+ messages in thread
From: Johannes Thumshirn @ 2016-03-29 7:20 UTC (permalink / raw)
To: emilne
Cc: Martin K. Petersen, James E.J. Bottomley, linux-scsi,
linux-kernel, Hannes Reinecke, stable
On Donnerstag, 24. M�rz 2016 11:42:44 CEST Ewan D. Milne wrote:
> On Thu, 2016-03-24 at 10:56 +0100, Johannes Thumshirn wrote:
> > The target state machine only knows 'STARGET_DEL', which is set once
> > scsi_target_destroy() is called.
> > However, by that time the structure is still part of the __stargets
> > list of the SCSI host, so any concurrent invocation will see this as
> > a valid target, causing an access to freed memory.
> >
> > This patch adds an intermediate state 'STARGET_REMOVE', which is set
> > as soon as the target is scheduled to be removed.
> > With this any concurrent invocation will be able to skip these
> > targets and avoid the above scenario.
> >
> > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> > Fixes: 90a88d6ef 'scsi: fix soft lockup in scsi_remove_target() on module
> > removal' Cc: stable@vger.kernel.org
> > Reviewed-by: Hannes Reinecke <hare@suse.com>
> > ---
> >
> > drivers/scsi/scsi_scan.c | 1 +
> > drivers/scsi/scsi_sysfs.c | 2 ++
> > include/scsi/scsi_device.h | 1 +
> > 3 files changed, 4 insertions(+)
> >
> > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> > index 6a82066..37459dfa 100644
> > --- a/drivers/scsi/scsi_scan.c
> > +++ b/drivers/scsi/scsi_scan.c
> > @@ -315,6 +315,7 @@ static void scsi_target_destroy(struct scsi_target
> > *starget)>
> > struct Scsi_Host *shost = dev_to_shost(dev->parent);
> > unsigned long flags;
> >
> > + BUG_ON(starget->state != STARGET_REMOVE);
> >
> > starget->state = STARGET_DEL;
> > transport_destroy_device(dev);
> > spin_lock_irqsave(shost->host_lock, flags);
> >
> > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> > index 00bc721..0df82e8 100644
> > --- a/drivers/scsi/scsi_sysfs.c
> > +++ b/drivers/scsi/scsi_sysfs.c
> >
> > @@ -1279,11 +1279,13 @@ 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 ||
> >
> > starget == last_target)
> >
> > continue;
> >
> > if (starget->dev.parent == dev || &starget->dev == dev) {
> >
> > kref_get(&starget->reap_ref);
> > last_target = starget;
> >
> > + starget->state = STARGET_REMOVE;
> >
> > spin_unlock_irqrestore(shost->host_lock, flags);
> > __scsi_remove_target(starget);
> > scsi_target_reap(starget);
> >
> > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> > index f63a167..2bffaa6 100644
> > --- a/include/scsi/scsi_device.h
> > +++ b/include/scsi/scsi_device.h
> > @@ -240,6 +240,7 @@ scmd_printk(const char *, const struct scsi_cmnd *,
> > const char *, ...);>
> > enum scsi_target_state {
> >
> > STARGET_CREATED = 1,
> > STARGET_RUNNING,
> >
> > + STARGET_REMOVE,
> >
> > STARGET_DEL,
> >
> > };
>
> This looks fine. Do we still need 90a88d6ef (scsi: fix soft lockup in
> scsi_remove_target() on module removal) or can that be reverted now,
> since the STARGET_REMOVE state will allow the iteration to continue?
>
> Reviewed-by: Ewan D. Milne <emilne@redhat.com>
We've tested it on-top of 90a88d6ef, so I'm not quite sure.
Anyways, I've seen a mail from the 0-day bot regarding this patch, so I'll
have to check that one first.
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state
@ 2016-03-29 7:20 ` Johannes Thumshirn
0 siblings, 0 replies; 6+ messages in thread
From: Johannes Thumshirn @ 2016-03-29 7:20 UTC (permalink / raw)
To: emilne
Cc: Martin K. Petersen, James E.J. Bottomley, linux-scsi,
linux-kernel, Hannes Reinecke, stable
On Donnerstag, 24. März 2016 11:42:44 CEST Ewan D. Milne wrote:
> On Thu, 2016-03-24 at 10:56 +0100, Johannes Thumshirn wrote:
> > The target state machine only knows 'STARGET_DEL', which is set once
> > scsi_target_destroy() is called.
> > However, by that time the structure is still part of the __stargets
> > list of the SCSI host, so any concurrent invocation will see this as
> > a valid target, causing an access to freed memory.
> >
> > This patch adds an intermediate state 'STARGET_REMOVE', which is set
> > as soon as the target is scheduled to be removed.
> > With this any concurrent invocation will be able to skip these
> > targets and avoid the above scenario.
> >
> > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> > Fixes: 90a88d6ef 'scsi: fix soft lockup in scsi_remove_target() on module
> > removal' Cc: stable@vger.kernel.org
> > Reviewed-by: Hannes Reinecke <hare@suse.com>
> > ---
> >
> > drivers/scsi/scsi_scan.c | 1 +
> > drivers/scsi/scsi_sysfs.c | 2 ++
> > include/scsi/scsi_device.h | 1 +
> > 3 files changed, 4 insertions(+)
> >
> > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> > index 6a82066..37459dfa 100644
> > --- a/drivers/scsi/scsi_scan.c
> > +++ b/drivers/scsi/scsi_scan.c
> > @@ -315,6 +315,7 @@ static void scsi_target_destroy(struct scsi_target
> > *starget)>
> > struct Scsi_Host *shost = dev_to_shost(dev->parent);
> > unsigned long flags;
> >
> > + BUG_ON(starget->state != STARGET_REMOVE);
> >
> > starget->state = STARGET_DEL;
> > transport_destroy_device(dev);
> > spin_lock_irqsave(shost->host_lock, flags);
> >
> > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> > index 00bc721..0df82e8 100644
> > --- a/drivers/scsi/scsi_sysfs.c
> > +++ b/drivers/scsi/scsi_sysfs.c
> >
> > @@ -1279,11 +1279,13 @@ 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 ||
> >
> > starget == last_target)
> >
> > continue;
> >
> > if (starget->dev.parent == dev || &starget->dev == dev) {
> >
> > kref_get(&starget->reap_ref);
> > last_target = starget;
> >
> > + starget->state = STARGET_REMOVE;
> >
> > spin_unlock_irqrestore(shost->host_lock, flags);
> > __scsi_remove_target(starget);
> > scsi_target_reap(starget);
> >
> > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> > index f63a167..2bffaa6 100644
> > --- a/include/scsi/scsi_device.h
> > +++ b/include/scsi/scsi_device.h
> > @@ -240,6 +240,7 @@ scmd_printk(const char *, const struct scsi_cmnd *,
> > const char *, ...);>
> > enum scsi_target_state {
> >
> > STARGET_CREATED = 1,
> > STARGET_RUNNING,
> >
> > + STARGET_REMOVE,
> >
> > STARGET_DEL,
> >
> > };
>
> This looks fine. Do we still need 90a88d6ef (scsi: fix soft lockup in
> scsi_remove_target() on module removal) or can that be reverted now,
> since the STARGET_REMOVE state will allow the iteration to continue?
>
> Reviewed-by: Ewan D. Milne <emilne@redhat.com>
We've tested it on-top of 90a88d6ef, so I'm not quite sure.
Anyways, I've seen a mail from the 0-day bot regarding this patch, so I'll
have to check that one first.
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state
2016-03-24 15:42 ` Ewan D. Milne
2016-03-29 7:20 ` Johannes Thumshirn
@ 2016-03-29 7:45 ` Christoph Hellwig
1 sibling, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2016-03-29 7:45 UTC (permalink / raw)
To: Ewan D. Milne
Cc: Johannes Thumshirn, Martin K. Petersen, James E.J. Bottomley,
linux-scsi, linux-kernel, Hannes Reinecke, stable
On Thu, Mar 24, 2016 at 11:42:44AM -0400, Ewan D. Milne wrote:
> This looks fine. Do we still need 90a88d6ef (scsi: fix soft lockup in
> scsi_remove_target() on module removal) or can that be reverted now,
> since the STARGET_REMOVE state will allow the iteration to continue?
Yes, that stale pointer check is something I'd really, really prefer
not to have around in the long run.
Otherwise the patch looks like the right direction to me.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-03-29 7:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-24 9:56 [PATCH] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state Johannes Thumshirn
2016-03-24 15:42 ` Ewan D. Milne
2016-03-29 7:20 ` Johannes Thumshirn
2016-03-29 7:20 ` Johannes Thumshirn
2016-03-29 7:20 ` Johannes Thumshirn
2016-03-29 7:45 ` Christoph Hellwig
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.