| Message ID | 20200924104559.26753-1-hare@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | scsi_dh_alua: avoid crash during alua_bus_detach() | expand |
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
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.
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. >
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
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); }
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(-)