| Message ID | 20191111104522.99531-1-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi_dh_rdac: avoid crash during rescan | expand |
On 11/11/19 2:45 AM, Hannes Reinecke wrote: > During rescanning the device might already have been removed, so > we should drop the BUG_ON and just ignore the non-existing device. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/scsi/device_handler/scsi_dh_rdac.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c > index 5efc959493ec..33a71df5ee59 100644 > --- a/drivers/scsi/device_handler/scsi_dh_rdac.c > +++ b/drivers/scsi/device_handler/scsi_dh_rdac.c > @@ -424,8 +424,8 @@ static int check_ownership(struct scsi_device *sdev, struct rdac_dh_data *h) > rcu_read_lock(); > list_for_each_entry_rcu(tmp, &h->ctlr->dh_list, node) { > /* h->sdev should always be valid */ > - BUG_ON(!tmp->sdev); > - tmp->sdev->access_state = access_state; > + if (tmp->sdev) { > + tmp->sdev->access_state = access_state; > } > rcu_read_unlock(); > err = SCSI_DH_OK; Since rdac_bus_detach() may clear h->sdev concurrently with the above code I think READ_ONCE() is needed to make the above code safe. Has it been considered to change the kfree() call in rdac_bus_detach() into a kfree_rcu() call? I think otherwise the above list_for_each_entry_rcu() loop may trigger a use-after-free. Thanks, Bart.
On 11/11/19 5:07 PM, Bart Van Assche wrote: > On 11/11/19 2:45 AM, Hannes Reinecke wrote: >> During rescanning the device might already have been removed, so >> we should drop the BUG_ON and just ignore the non-existing device. >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> drivers/scsi/device_handler/scsi_dh_rdac.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c >> b/drivers/scsi/device_handler/scsi_dh_rdac.c >> index 5efc959493ec..33a71df5ee59 100644 >> --- a/drivers/scsi/device_handler/scsi_dh_rdac.c >> +++ b/drivers/scsi/device_handler/scsi_dh_rdac.c >> @@ -424,8 +424,8 @@ static int check_ownership(struct scsi_device >> *sdev, struct rdac_dh_data *h) >> rcu_read_lock(); >> list_for_each_entry_rcu(tmp, &h->ctlr->dh_list, node) { >> /* h->sdev should always be valid */ >> - BUG_ON(!tmp->sdev); >> - tmp->sdev->access_state = access_state; >> + if (tmp->sdev) { >> + tmp->sdev->access_state = access_state; >> } >> rcu_read_unlock(); >> err = SCSI_DH_OK; > > Since rdac_bus_detach() may clear h->sdev concurrently with the above > code I think READ_ONCE() is needed to make the above code safe. > > Has it been considered to change the kfree() call in rdac_bus_detach() > into a kfree_rcu() call? I think otherwise the above > list_for_each_entry_rcu() loop may trigger a use-after-free. > Good point. I'll have a look. Cheers, Hannes
On Mon, Nov 11, 2019 at 11:45:22AM +0100, Hannes Reinecke wrote: > During rescanning the device might already have been removed, so > we should drop the BUG_ON and just ignore the non-existing device. The device is also added to the list before sdev is set, which is rather silly, so that might be worth fixing as well.
diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c index 5efc959493ec..33a71df5ee59 100644 --- a/drivers/scsi/device_handler/scsi_dh_rdac.c +++ b/drivers/scsi/device_handler/scsi_dh_rdac.c @@ -424,8 +424,8 @@ static int check_ownership(struct scsi_device *sdev, struct rdac_dh_data *h) rcu_read_lock(); list_for_each_entry_rcu(tmp, &h->ctlr->dh_list, node) { /* h->sdev should always be valid */ - BUG_ON(!tmp->sdev); - tmp->sdev->access_state = access_state; + if (tmp->sdev) { + tmp->sdev->access_state = access_state; } rcu_read_unlock(); err = SCSI_DH_OK;
During rescanning the device might already have been removed, so we should drop the BUG_ON and just ignore the non-existing device. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/device_handler/scsi_dh_rdac.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)