Re: [PATCH 4.19 042/101] scsi: scsi_dh_alua: Avoid crash during alua_bus_detach()

[Pavel Machek]

Hi!

> From: Hannes Reinecke <hare@xxxxxxx>
> 
> [ Upstream commit 5faf50e9e9fdc2117c61ff7e20da49cd6a29e0ca ]
> 
> 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 is 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.

I don't get it.

In the new version, h->sdev will never be NULL, because it is never
set to NULL. It is will be valid up-to the point when h is freed...

synchronize_rcu() should prevent race with alua_rtpg(), but BUG_ON()s
should stay, to catch any bogosity... Or maybe the if (!h->sdev) tests
should be simply removed. But it does not make sense to silently
continue.

Best regards,
								Pavel

> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -672,8 +672,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();
> @@ -719,7 +719,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)
> @@ -1160,7 +1161,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);
> @@ -1169,6 +1169,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);
>  }
>  

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Attachment: signature.asc
Description: Digital signature