[PATCH] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state

[PATCH] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state

[Johannes Thumshirn]

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

Re: [PATCH] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state

[Ewan D. Milne]

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>

Re: [PATCH] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state

[Johannes Thumshirn]

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

Re: [PATCH] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state

[Johannes Thumshirn]

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

Re: [PATCH] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state

[Johannes Thumshirn]

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

Re: [PATCH] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state

[Christoph Hellwig]

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.