From: Bart Van Assche <bvanassche@acm.org>
To: Hannes Reinecke <hare@suse.de>,
"Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>,
James Bottomley <james.bottomley@hansenpartnership.com>,
"Ewan D . Milne" <emilne@redhat.com>,
linux-scsi@vger.kernel.org, Brian Bunker <brian@purestorage.com>
Subject: Re: [PATCH] scsi_dh_alua: avoid crash during alua_bus_detach()
Date: Sat, 26 Sep 2020 15:01:46 -0700 [thread overview]
Message-ID: <2175d8e0-88fa-a9eb-5d50-46f0eed402cf@acm.org> (raw)
In-Reply-To: <20200924104559.26753-1-hare@suse.de>
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.
next prev parent reply other threads:[~2020-09-26 22:01 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-24 10:45 [PATCH] scsi_dh_alua: avoid crash during alua_bus_detach() Hannes Reinecke
[not found] ` <2F7D7601-D9C2-4FFD-AA59-65A243F16AA9@purestorage.com>
2020-09-24 16:38 ` Brian Bunker
2020-09-26 22:01 ` Bart Van Assche [this message]
2020-09-28 21:02 ` Ewan D. Milne
2020-11-03 2:01 ` Martin K. Petersen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2175d8e0-88fa-a9eb-5d50-46f0eed402cf@acm.org \
--to=bvanassche@acm.org \
--cc=brian@purestorage.com \
--cc=emilne@redhat.com \
--cc=hare@suse.de \
--cc=hch@lst.de \
--cc=james.bottomley@hansenpartnership.com \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).