linux-kernel - Re: [PATCH 4.19 042/101] scsi: scsi_dh_alua: Avoid crash during alua_bus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20201118221748.GC23840@amd>
Date:   Wed, 18 Nov 2020 23:17:49 +0100
From:   Pavel Machek <pavel@...x.de>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Brian Bunker <brian@...estorage.com>,
        Jitendra Khasdev <jitendra.khasdev@...cle.com>,
        Hannes Reinecke <hare@...e.de>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Sasha Levin <sashal@...nel.org>
Subject: Re: [PATCH 4.19 042/101] scsi: scsi_dh_alua: Avoid crash during
 alua_bus_detach()

Hi!

> From: Hannes Reinecke <hare@...e.de>
> 
> [ 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

Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)