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 <[email protected]>
Fixes: 90a88d6ef 'scsi: fix soft lockup in scsi_remove_target() on module removal'
Cc: [email protected]
Reviewed-by: Hannes Reinecke <[email protected]>
---
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
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 <[email protected]>
> Fixes: 90a88d6ef 'scsi: fix soft lockup in scsi_remove_target() on module removal'
> Cc: [email protected]
> Reviewed-by: Hannes Reinecke <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> > Fixes: 90a88d6ef 'scsi: fix soft lockup in scsi_remove_target() on module
> > removal' Cc: [email protected]
> > Reviewed-by: Hannes Reinecke <[email protected]>
> > ---
> >
> > 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 <[email protected]>
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
[email protected] +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
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.