[PATCH] scsi_dh_alua: avoid crash during alua_bus_detach()

[PATCH] scsi_dh_alua: avoid crash during alua_bus_detach()

[Hannes Reinecke]

alua_bus_detach() might be running concurrently with alua_rtpg_work(),
so we might trip over h->sdev == NULL and call BUG_ON().
The correct way of handling it would be to not set h->sdev to NULL
in alua_bus_detach(), and call rcu_synchronize() before the final
delete to ensure that all concurrent threads have left the critical
section.
Then we can get rid of the BUG_ON(), and replace it with a simple
if condition.

Cc: Brian Bunker <brian@purestorage.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index f32da0ca529e..308bda2e9c00 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -658,8 +658,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 					rcu_read_lock();
 					list_for_each_entry_rcu(h,
 						&tmp_pg->dh_list, node) {
-						/* h->sdev should always be valid */
-						BUG_ON(!h->sdev);
+						if (!h->sdev)
+							continue;
 						h->sdev->access_state = desc[0];
 					}
 					rcu_read_unlock();
@@ -705,7 +705,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 			pg->expiry = 0;
 			rcu_read_lock();
 			list_for_each_entry_rcu(h, &pg->dh_list, node) {
-				BUG_ON(!h->sdev);
+				if (!h->sdev)
+					continue;
 				h->sdev->access_state =
 					(pg->state & SCSI_ACCESS_STATE_MASK);
 				if (pg->pref)
@@ -1147,7 +1148,6 @@ static void alua_bus_detach(struct scsi_device *sdev)
 	spin_lock(&h->pg_lock);
 	pg = rcu_dereference_protected(h->pg, lockdep_is_held(&h->pg_lock));
 	rcu_assign_pointer(h->pg, NULL);
-	h->sdev = NULL;
 	spin_unlock(&h->pg_lock);
 	if (pg) {
 		spin_lock_irq(&pg->lock);
@@ -1156,6 +1156,7 @@ static void alua_bus_detach(struct scsi_device *sdev)
 		kref_put(&pg->kref, release_port_group);
 	}
 	sdev->handler_data = NULL;
+	synchronize_rcu();
 	kfree(h);
 }
 
-- 
2.16.4

Re: [PATCH] scsi_dh_alua: avoid crash during alua_bus_detach()

[Brian Bunker]

Yes. That looks good to me.

Thanks,
Brian

Brian Bunker
SW Eng
brian@purestorage.com

> On Sep 24, 2020, at 9:36 AM, Brian Bunker <brian@purestorage.com> wrote:
> 
> Yes. That looks good to me.
> 
> Thanks,
> Brian
> 
> Brian Bunker
> SW Eng
> brian@purestorage.com

Re: [PATCH] scsi_dh_alua: avoid crash during alua_bus_detach()

[Bart Van Assche]

On 2020-09-24 03:45, Hannes Reinecke wrote:
> alua_bus_detach() might be running concurrently with alua_rtpg_work(),
> so we might trip over h->sdev == NULL and call BUG_ON().
> The correct way of handling it would be to not set h->sdev to NULL
> in alua_bus_detach(), and call rcu_synchronize() before the final
> delete to ensure that all concurrent threads have left the critical
> section.
> Then we can get rid of the BUG_ON(), and replace it with a simple
> if condition.
> 
> Cc: Brian Bunker <brian@purestorage.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/device_handler/scsi_dh_alua.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index f32da0ca529e..308bda2e9c00 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -658,8 +658,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>  					rcu_read_lock();
>  					list_for_each_entry_rcu(h,
>  						&tmp_pg->dh_list, node) {
> -						/* h->sdev should always be valid */
> -						BUG_ON(!h->sdev);
> +						if (!h->sdev)
> +							continue;
>  						h->sdev->access_state = desc[0];
>  					}
>  					rcu_read_unlock();
> @@ -705,7 +705,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>  			pg->expiry = 0;
>  			rcu_read_lock();
>  			list_for_each_entry_rcu(h, &pg->dh_list, node) {
> -				BUG_ON(!h->sdev);
> +				if (!h->sdev)
> +					continue;
>  				h->sdev->access_state =
>  					(pg->state & SCSI_ACCESS_STATE_MASK);
>  				if (pg->pref)
> @@ -1147,7 +1148,6 @@ static void alua_bus_detach(struct scsi_device *sdev)
>  	spin_lock(&h->pg_lock);
>  	pg = rcu_dereference_protected(h->pg, lockdep_is_held(&h->pg_lock));
>  	rcu_assign_pointer(h->pg, NULL);
> -	h->sdev = NULL;
>  	spin_unlock(&h->pg_lock);
>  	if (pg) {
>  		spin_lock_irq(&pg->lock);
> @@ -1156,6 +1156,7 @@ static void alua_bus_detach(struct scsi_device *sdev)
>  		kref_put(&pg->kref, release_port_group);
>  	}
>  	sdev->handler_data = NULL;
> +	synchronize_rcu();
>  	kfree(h);
>  }

Hi Hannes,

Do you agree that the changes in alua_bus_detach() make the changes in
alua_rtpg() superfluous?

How about freezing command processing for 'sdev' while detaching a
device handler instead of inserting a synchronize_rcu() call in
alua_bus_detach()? I'm concerned that the alua_bus_detach() changes are
not sufficient to fix all possible races between detaching a device
handler and the following code from the SCSI error handler:

	if (sdev->handler && sdev->handler->check_sense) {
		int rc;

		rc = sdev->handler->check_sense(sdev, &sshdr);
		if (rc != SCSI_RETURN_NOT_HANDLED)
			return rc;
		/* handler does not care. Drop down to default handling */
	}

Thanks,

Bart.

Re: [PATCH] scsi_dh_alua: avoid crash during alua_bus_detach()

[Ewan D. Milne]

On Sat, 2020-09-26 at 15:01 -0700, Bart Van Assche wrote:
> On 2020-09-24 03:45, Hannes Reinecke wrote:
> > alua_bus_detach() might be running concurrently with
> > alua_rtpg_work(),
> > so we might trip over h->sdev == NULL and call BUG_ON().
> > The correct way of handling it would be to not set h->sdev to NULL
> > in alua_bus_detach(), and call rcu_synchronize() before the final
> > delete to ensure that all concurrent threads have left the critical
> > section.
> > Then we can get rid of the BUG_ON(), and replace it with a simple
> > if condition.
> > 
> > Cc: Brian Bunker <brian@purestorage.com>
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > ---
> >  drivers/scsi/device_handler/scsi_dh_alua.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
> > b/drivers/scsi/device_handler/scsi_dh_alua.c
> > index f32da0ca529e..308bda2e9c00 100644
> > --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> > +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> > @@ -658,8 +658,8 @@ static int alua_rtpg(struct scsi_device *sdev,
> > struct alua_port_group *pg)
> >  					rcu_read_lock();
> >  					list_for_each_entry_rcu(h,
> >  						&tmp_pg->dh_list, node)
> > {
> > -						/* h->sdev should
> > always be valid */
> > -						BUG_ON(!h->sdev);
> > +						if (!h->sdev)
> > +							continue;
> >  						h->sdev->access_state =
> > desc[0];
> >  					}
> >  					rcu_read_unlock();
> > @@ -705,7 +705,8 @@ static int alua_rtpg(struct scsi_device *sdev,
> > struct alua_port_group *pg)
> >  			pg->expiry = 0;
> >  			rcu_read_lock();
> >  			list_for_each_entry_rcu(h, &pg->dh_list, node)
> > {
> > -				BUG_ON(!h->sdev);
> > +				if (!h->sdev)
> > +					continue;
> >  				h->sdev->access_state =
> >  					(pg->state &
> > SCSI_ACCESS_STATE_MASK);
> >  				if (pg->pref)
> > @@ -1147,7 +1148,6 @@ static void alua_bus_detach(struct
> > scsi_device *sdev)
> >  	spin_lock(&h->pg_lock);
> >  	pg = rcu_dereference_protected(h->pg, lockdep_is_held(&h-
> > >pg_lock));
> >  	rcu_assign_pointer(h->pg, NULL);
> > -	h->sdev = NULL;
> >  	spin_unlock(&h->pg_lock);
> >  	if (pg) {
> >  		spin_lock_irq(&pg->lock);
> > @@ -1156,6 +1156,7 @@ static void alua_bus_detach(struct
> > scsi_device *sdev)
> >  		kref_put(&pg->kref, release_port_group);
> >  	}
> >  	sdev->handler_data = NULL;
> > +	synchronize_rcu();
> >  	kfree(h);
> >  }
> 
> Hi Hannes,
> 
> Do you agree that the changes in alua_bus_detach() make the changes
> in
> alua_rtpg() superfluous?

I agree that the "if (!h->sdev)   continue;" should not be needed in
alua_rtpg() if the h->sdev remains valid while in the list.

I'm a little concerned about adding the synchronize_rcu() as this is
called in the scsi_device_dev_release_usercontext() path, with a lot
of LUNs it could take a while to remove all the devices, see e.g.:

f983622ae605 scsi: core: Avoid calling synchronize_rcu() for each
               device in scsi_host_block()

It doesn't look like we ever NULL sdev->handler on detach even though
we do a module_put() on the DH.  But we have already called the
release() function so perhaps this doesn't cause a problem in
practice.

-Ewan

> 
> How about freezing command processing for 'sdev' while detaching a
> device handler instead of inserting a synchronize_rcu() call in
> alua_bus_detach()? I'm concerned that the alua_bus_detach() changes
> are
> not sufficient to fix all possible races between detaching a device
> handler and the following code from the SCSI error handler:
> 
> 	if (sdev->handler && sdev->handler->check_sense) {
> 		int rc;
> 
> 		rc = sdev->handler->check_sense(sdev, &sshdr);
> 		if (rc != SCSI_RETURN_NOT_HANDLED)
> 			return rc;
> 		/* handler does not care. Drop down to default handling
> */
> 	}
> 
> Thanks,
> 
> Bart.
> 

Re: [PATCH] scsi_dh_alua: avoid crash during alua_bus_detach()

[Martin K. Petersen]

On Thu, 24 Sep 2020 12:45:59 +0200, Hannes Reinecke wrote:

> alua_bus_detach() might be running concurrently with alua_rtpg_work(),
> so we might trip over h->sdev == NULL and call BUG_ON().
> The correct way of handling it would be to not set h->sdev to NULL
> in alua_bus_detach(), and call rcu_synchronize() before the final
> delete to ensure that all concurrent threads have left the critical
> section.
> Then we can get rid of the BUG_ON(), and replace it with a simple
> if condition.

Applied to 5.10/scsi-fixes, thanks!

[1/1] scsi: scsi_dh_alua: Avoid crash during alua_bus_detach()
      https://git.kernel.org/mkp/scsi/c/5faf50e9e9fd

-- 
Martin K. Petersen	Oracle Linux Engineering