* [PATCH] scsi_dh_alua: avoid crash during alua_bus_detach()
@ 2020-09-24 10:45 Hannes Reinecke
[not found] ` <2F7D7601-D9C2-4FFD-AA59-65A243F16AA9@purestorage.com>
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Hannes Reinecke @ 2020-09-24 10:45 UTC (permalink / raw)
To: Martin K. Petersen
Cc: Christoph Hellwig, James Bottomley, Ewan D . Milne, linux-scsi,
Hannes Reinecke, Brian Bunker
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
[parent not found: <2F7D7601-D9C2-4FFD-AA59-65A243F16AA9@purestorage.com>]
* Re: [PATCH] scsi_dh_alua: avoid crash during alua_bus_detach()
[not found] ` <2F7D7601-D9C2-4FFD-AA59-65A243F16AA9@purestorage.com>
@ 2020-09-24 16:38 ` Brian Bunker
0 siblings, 0 replies; 5+ messages in thread
From: Brian Bunker @ 2020-09-24 16:38 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Martin K. Petersen, Christoph Hellwig, James Bottomley,
Ewan D . Milne, linux-scsi
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] scsi_dh_alua: avoid crash during alua_bus_detach()
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-26 22:01 ` Bart Van Assche
2020-09-28 21:02 ` Ewan D. Milne
2020-11-03 2:01 ` Martin K. Petersen
2 siblings, 1 reply; 5+ messages in thread
From: Bart Van Assche @ 2020-09-26 22:01 UTC (permalink / raw)
To: Hannes Reinecke, Martin K. Petersen
Cc: Christoph Hellwig, James Bottomley, Ewan D . Milne, linux-scsi,
Brian Bunker
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] scsi_dh_alua: avoid crash during alua_bus_detach()
2020-09-26 22:01 ` Bart Van Assche
@ 2020-09-28 21:02 ` Ewan D. Milne
0 siblings, 0 replies; 5+ messages in thread
From: Ewan D. Milne @ 2020-09-28 21:02 UTC (permalink / raw)
To: Bart Van Assche, Hannes Reinecke, Martin K. Petersen
Cc: Christoph Hellwig, James Bottomley, linux-scsi, Brian Bunker
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.
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] scsi_dh_alua: avoid crash during alua_bus_detach()
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-26 22:01 ` Bart Van Assche
@ 2020-11-03 2:01 ` Martin K. Petersen
2 siblings, 0 replies; 5+ messages in thread
From: Martin K. Petersen @ 2020-11-03 2:01 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Martin K . Petersen, linux-scsi, Christoph Hellwig,
Ewan D . Milne, James Bottomley, Brian Bunker
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-11-03 2:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2020-09-28 21:02 ` Ewan D. Milne
2020-11-03 2:01 ` Martin K. Petersen
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).